Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI. r?mak draft
authorMark Banner <standard8@mozilla.com>
Fri, 23 Mar 2018 17:32:37 +0000
changeset 772602 49f820ef221d593e372f49df5ba7d9a5cb78a291
parent 772522 f0d13136b3580182df241674cf97a3fbf1543c2a
push id103980
push userbmo:standard8@mozilla.com
push dateMon, 26 Mar 2018 15:50:23 +0000
reviewersmak
bugs1432435
milestone61.0a1
Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI. r?mak MozReview-Commit-ID: GSQOoSoeibq
browser/components/places/tests/browser/browser_library_views_liveupdate.js
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/nsNavBookmarks.h
toolkit/components/places/tests/legacy/test_bookmarks.js
--- a/browser/components/places/tests/browser/browser_library_views_liveupdate.js
+++ b/browser/components/places/tests/browser/browser_library_views_liveupdate.js
@@ -3,228 +3,195 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /**
  * Tests Library Left pane view for liveupdate.
  */
 
 let gLibrary = null;
 
+add_task(async function setup() {
+  gLibrary = await promiseLibrary();
+
+  await PlacesUtils.bookmarks.eraseEverything();
+
+  registerCleanupFunction(async () => {
+    await PlacesUtils.bookmarks.eraseEverything();
+    await promiseLibraryClosed(gLibrary);
+  });
+});
+
 async function testInFolder(folderGuid, prefix) {
   let addedBookmarks = [];
-  let item = await PlacesUtils.bookmarks.insert({
+
+  let item = await insertAndCheckItem({
     parentGuid: folderGuid,
     title: `${prefix}1`,
     url: `http://${prefix}1.mozilla.org/`,
   });
   item.title = `${prefix}1_edited`;
-  await PlacesUtils.bookmarks.update(item);
+  await updateAndCheckItem(item);
   addedBookmarks.push(item);
 
-  item = await PlacesUtils.bookmarks.insert({
+  item = await insertAndCheckItem({
     parentGuid: folderGuid,
     title: `${prefix}2`,
     url: "place:",
-  });
+  }, 0);
 
   item.title = `${prefix}2_edited`;
-  await PlacesUtils.bookmarks.update(item);
+  await updateAndCheckItem(item, 0);
   addedBookmarks.push(item);
 
-  item = await PlacesUtils.bookmarks.insert({
+  item = await insertAndCheckItem({
     parentGuid: folderGuid,
     type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
   });
   addedBookmarks.push(item);
 
-  item = await PlacesUtils.bookmarks.insert({
+  item = await insertAndCheckItem({
     parentGuid: folderGuid,
     title: `${prefix}f`,
     type: PlacesUtils.bookmarks.TYPE_FOLDER,
-  });
+  }, 1);
 
   item.title = `${prefix}f_edited`;
-  await PlacesUtils.bookmarks.update(item);
+  await updateAndCheckItem(item, 1);
+
+  item.index = 0;
+  await updateAndCheckItem(item, 0);
   addedBookmarks.push(item);
 
-  item = await PlacesUtils.bookmarks.insert({
-    parentGuid: item.guid,
+  let folderGuid1 = item.guid;
+
+  item = await insertAndCheckItem({
+    parentGuid: folderGuid1,
     title: `${prefix}f1`,
     url: `http://${prefix}f1.mozilla.org/`,
   });
   addedBookmarks.push(item);
 
+  item = await insertAndCheckItem({
+    parentGuid: folderGuid1,
+    title: `${prefix}f12`,
+    url: `http://${prefix}f12.mozilla.org/`,
+  });
+  addedBookmarks.push(item);
+
   item.index = 0;
-  await PlacesUtils.bookmarks.update(item);
+  await updateAndCheckItem(item);
 
   return addedBookmarks;
 }
 
 add_task(async function test() {
-  // This test takes quite some time, and timeouts frequently, so we require
-  // more time to run.
-  // See Bug 525610.
-  requestLongerTimeout(2);
-
-  // Open Library, we will check the left pane.
-  gLibrary = await promiseLibrary();
-
-  // Add observers.
-  PlacesUtils.bookmarks.addObserver(bookmarksObserver);
-  PlacesUtils.annotations.addObserver(bookmarksObserver);
   let addedBookmarks = [];
 
-  // MENU
-  ok(true, "*** Acting on menu bookmarks");
+  info("*** Acting on menu bookmarks");
   addedBookmarks = addedBookmarks.concat(await testInFolder(PlacesUtils.bookmarks.menuGuid, "bm"));
 
-  // TOOLBAR
-  ok(true, "*** Acting on toolbar bookmarks");
+  info("*** Acting on toolbar bookmarks");
   addedBookmarks = addedBookmarks.concat(await testInFolder(PlacesUtils.bookmarks.toolbarGuid, "tb"));
 
-  // UNSORTED
-  ok(true, "*** Acting on unsorted bookmarks");
+  info("*** Acting on unsorted bookmarks");
   addedBookmarks = addedBookmarks.concat(await testInFolder(PlacesUtils.bookmarks.unfiledGuid, "ub"));
 
-  // Remove all added bookmarks.
-  for (let i = 0; i < addedBookmarks.length; i++) {
-    // If we remove an item after its containing folder has been removed,
-    // this will throw, but we can ignore that.
-    try {
-      await PlacesUtils.bookmarks.remove(addedBookmarks[i]);
-    } catch (ex) {}
+  // Remove bookmarks in reverse order, so that the effects are correct.
+  for (let i = addedBookmarks.length - 1; i >= 0; i--) {
+    await removeAndCheckItem(addedBookmarks[i]);
+  }
+});
+
+async function insertAndCheckItem(itemData, expectedIndex) {
+  let item = await PlacesUtils.bookmarks.insert(itemData);
+
+  let [node, index, title] = getNodeForTreeItem(item.guid, gLibrary.PlacesOrganizer._places);
+  // Left pane should not be updated for normal bookmarks or separators.
+  switch (itemData.type || PlacesUtils.bookmarks.TYPE_BOOKMARK) {
+    case PlacesUtils.bookmarks.TYPE_BOOKMARK:
+      let uriString = itemData.url;
+      let isQuery = uriString.substr(0, 6) == "place:";
+      if (isQuery) {
+        Assert.ok(node, "Should have a new query in the left pane.");
+        break;
+      }
+      // Fallthrough if this isn't a query
+    case PlacesUtils.bookmarks.TYPE_SEPARATOR:
+      Assert.ok(!node, "Should not have added a bookmark or separator to the left pane.");
+      break;
+    default:
+      Assert.ok(node, "Should have added a new node in the left pane for a folder.");
+  }
+
+  if (node) {
+    Assert.equal(title, itemData.title, "Should have the correct title");
+    Assert.equal(index, expectedIndex, "Should have the expected index");
   }
 
-  // Remove observers.
-  PlacesUtils.bookmarks.removeObserver(bookmarksObserver);
-  PlacesUtils.annotations.removeObserver(bookmarksObserver);
+  return item;
+}
+
+async function updateAndCheckItem(newItemData, expectedIndex) {
+  await PlacesUtils.bookmarks.update(newItemData);
+
+  let [node, index, title] = getNodeForTreeItem(newItemData.guid, gLibrary.PlacesOrganizer._places);
 
-  await promiseLibraryClosed(gLibrary);
-});
+  // Left pane should not be updated for normal bookmarks or separators.
+  switch (newItemData.type || PlacesUtils.bookmarks.TYPE_BOOKMARK) {
+    case PlacesUtils.bookmarks.TYPE_BOOKMARK:
+      let isQuery = newItemData.url.protocol == "place:";
+      if (isQuery) {
+        Assert.ok(node, "Should be able to find the updated node");
+        break;
+      }
+      // Fallthrough if this isn't a query
+    case PlacesUtils.bookmarks.TYPE_SEPARATOR:
+      Assert.ok(!node, "Should not be able to find the updated node");
+      break;
+    default:
+      Assert.ok(node, "Should be able to find the updated node");
+  }
+
+  if (node) {
+    Assert.equal(title, newItemData.title, "Should have the correct title");
+    Assert.equal(index, expectedIndex, "Should have the expected index");
+  }
+}
+
+async function removeAndCheckItem(itemData) {
+  await PlacesUtils.bookmarks.remove(itemData);
+  let [node, ] = getNodeForTreeItem(itemData.guid, gLibrary.PlacesOrganizer._places);
+  Assert.ok(!node, "Should not be able to find the removed node");
+}
 
 /**
- * The observer is where magic happens, for every change we do it will look for
- * nodes positions in the affected views.
- */
-let bookmarksObserver = {
-  QueryInterface: XPCOMUtils.generateQI([
-    Ci.nsINavBookmarkObserver,
-    Ci.nsIAnnotationObserver
-  ]),
-
-  // nsIAnnotationObserver
-  onItemAnnotationSet() {},
-  onItemAnnotationRemoved() {},
-  onPageAnnotationSet() {},
-  onPageAnnotationRemoved() {},
-
-  // nsINavBookmarkObserver
-  onItemAdded: function PSB_onItemAdded(aItemId, aFolderId, aIndex, aItemType,
-                                        aURI) {
-    let node = null;
-    let index = null;
-    [node, index] = getNodeForTreeItem(aItemId, gLibrary.PlacesOrganizer._places);
-    // Left pane should not be updated for normal bookmarks or separators.
-    switch (aItemType) {
-      case PlacesUtils.bookmarks.TYPE_BOOKMARK:
-        let uriString = aURI.spec;
-        let isQuery = uriString.substr(0, 6) == "place:";
-        if (isQuery) {
-          isnot(node, null, "Found new Places node in left pane");
-          ok(index >= 0, "Node is at index " + index);
-          break;
-        }
-        // Fallback to separator case if this is not a query.
-      case PlacesUtils.bookmarks.TYPE_SEPARATOR:
-        is(node, null, "New Places node not added in left pane");
-        break;
-      default:
-        isnot(node, null, "Found new Places node in left pane");
-        ok(index >= 0, "Node is at index " + index);
-    }
-  },
-
-  onItemRemoved: function PSB_onItemRemoved(aItemId, aFolder, aIndex) {
-    let node = null;
-    [node, ] = getNodeForTreeItem(aItemId, gLibrary.PlacesOrganizer._places);
-    is(node, null, "Places node not found in left pane");
-  },
-
-  onItemMoved(aItemId,
-                        aOldFolderId, aOldIndex,
-                        aNewFolderId, aNewIndex, aItemType) {
-    let node = null;
-    let index = null;
-    [node, index] = getNodeForTreeItem(aItemId, gLibrary.PlacesOrganizer._places);
-    // Left pane should not be updated for normal bookmarks or separators.
-    switch (aItemType) {
-      case PlacesUtils.bookmarks.TYPE_BOOKMARK:
-        let uriString = PlacesUtils.bookmarks.getBookmarkURI(aItemId).spec;
-        let isQuery = uriString.substr(0, 6) == "place:";
-        if (isQuery) {
-          isnot(node, null, "Found new Places node in left pane");
-          ok(index >= 0, "Node is at index " + index);
-          break;
-        }
-        // Fallback to separator case if this is not a query.
-      case PlacesUtils.bookmarks.TYPE_SEPARATOR:
-        is(node, null, "New Places node not added in left pane");
-        break;
-      default:
-        isnot(node, null, "Found new Places node in left pane");
-        ok(index >= 0, "Node is at index " + index);
-    }
-  },
-
-  onBeginUpdateBatch: function PSB_onBeginUpdateBatch() {},
-  onEndUpdateBatch: function PSB_onEndUpdateBatch() {},
-  onItemVisited() {},
-  onItemChanged: function PSB_onItemChanged(aItemId, aProperty,
-                                            aIsAnnotationProperty, aNewValue) {
-    if (aProperty == "title") {
-      let validator = function(aTreeRowIndex) {
-        let tree = gLibrary.PlacesOrganizer._places;
-        let cellText = tree.view.getCellText(aTreeRowIndex,
-                                             tree.columns.getColumnAt(0));
-        return cellText == aNewValue;
-      };
-      let [node, , valid] = getNodeForTreeItem(aItemId, gLibrary.PlacesOrganizer._places, validator);
-      if (node) // Only visible nodes.
-        ok(valid, "Title cell value has been correctly updated");
-    }
-  }
-};
-
-
-/**
- * Get places node and index for an itemId in a tree view.
+ * Get places node, index and cell text for a guid in a tree view.
  *
- * @param aItemId
- *        item id of the item to search.
+ * @param aItemGuid
+ *        item guid of the item to search.
  * @param aTree
  *        Tree to search in.
- * @param aValidator [optional]
- *        function to check row validity if found.  Defaults to {return true;}.
- * @returns [node, index, valid] or [null, null, false] if not found.
+ * @returns [node, index, cellText] or [null, null, ""] if not found.
  */
-function getNodeForTreeItem(aItemId, aTree, aValidator) {
+function getNodeForTreeItem(aItemGuid, aTree) {
 
   function findNode(aContainerIndex) {
     if (aTree.view.isContainerEmpty(aContainerIndex))
-      return [null, null, false];
+      return [null, null, ""];
 
     // The rowCount limit is just for sanity, but we will end looping when
     // we have checked the last child of this container or we have found node.
     for (let i = aContainerIndex + 1; i < aTree.view.rowCount; i++) {
       let node = aTree.view.nodeForTreeIndex(i);
 
-      if (node.itemId == aItemId) {
+      if (node.bookmarkGuid == aItemGuid) {
         // Minus one because we want relative index inside the container.
-        let valid = aValidator ? aValidator(i) : true;
-        return [node, i - aTree.view.getParentIndex(i) - 1, valid];
+        let tree = gLibrary.PlacesOrganizer._places;
+        let cellText = tree.view.getCellText(i, tree.columns.getColumnAt(0));
+        return [node, i - aContainerIndex - 1, cellText];
       }
 
       if (PlacesUtils.nodeIsFolder(node)) {
         // Open container.
         aTree.view.toggleOpenState(i);
         // Search inside it.
         let foundNode = findNode(i);
         // Close container.
@@ -233,25 +200,25 @@ function getNodeForTreeItem(aItemId, aTr
         if (foundNode[0] != null)
           return foundNode;
       }
 
       // We have finished walking this container.
       if (!aTree.view.hasNextSibling(aContainerIndex + 1, i))
         break;
     }
-    return [null, null, false];
+    return [null, null, ""];
   }
 
   // Root node is hidden, so we need to manually walk the first level.
   for (let i = 0; i < aTree.view.rowCount; i++) {
     // Open container.
     aTree.view.toggleOpenState(i);
     // Search inside it.
     let foundNode = findNode(i);
     // Close container.
     aTree.view.toggleOpenState(i);
     // Return node if found.
     if (foundNode[0] != null)
       return foundNode;
   }
-  return [null, null, false];
+  return [null, null, ""];
 }
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -455,29 +455,21 @@ interface nsINavBookmarksService : nsISu
    *       any other method that changes an item property, but we will send
    *       the corresponding itemChanged notification instead.
    */
   void setItemLastModified(in long long aItemId,
                            in PRTime aLastModified,
                            [optional] in unsigned short aSource);
 
   /**
-   * Get the URI for a bookmark item.
-   */
-  nsIURI getBookmarkURI(in long long aItemId);
-
-  /**
-
-  /**
    * Get the parent folder's id for an item.
    */
   long long getFolderIdForItem(in long long aItemId);
 
   /**
-
    * Adds a bookmark observer. If ownsWeak is false, the bookmark service will
    * keep an owning reference to the observer.  If ownsWeak is true, then
    * aObserver must implement nsISupportsWeakReference, and the bookmark
    * service will keep a weak reference to the observer.
    */
   void addObserver(in nsINavBookmarkObserver observer,
                    [optional] in boolean ownsWeak);
 
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -1579,17 +1579,17 @@ nsNavBookmarks::GetItemTitle(int64_t aIt
   nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
 
   _title = bookmark.title;
   return NS_OK;
 }
 
 
-NS_IMETHODIMP
+nsresult
 nsNavBookmarks::GetBookmarkURI(int64_t aItemId,
                                nsIURI** _URI)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_URI);
 
   BookmarkData bookmark;
   nsresult rv = FetchItemInfo(aItemId, bookmark);
--- a/toolkit/components/places/nsNavBookmarks.h
+++ b/toolkit/components/places/nsNavBookmarks.h
@@ -114,16 +114,18 @@ public:
   typedef mozilla::places::BookmarkStatementId BookmarkStatementId;
 
   nsresult OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
                    int64_t aSessionId, int64_t aReferringId,
                    uint32_t aTransitionType, const nsACString& aGUID,
                    bool aHidden, uint32_t aVisitCount,
                    uint32_t aTyped, const nsAString& aLastKnownTitle);
 
+  nsresult GetBookmarkURI(int64_t aItemId, nsIURI** _URI);
+
   nsresult ResultNodeForContainer(int64_t aID,
                                   nsNavHistoryQueryOptions* aOptions,
                                   nsNavHistoryResultNode** aNode);
 
   // Find all the children of a folder, using the given query and options.
   // For each child, a ResultNode is created and added to |children|.
   // The results are ordered by folder position.
   nsresult QueryFolderChildren(int64_t aFolderId,
--- a/toolkit/components/places/tests/legacy/test_bookmarks.js
+++ b/toolkit/components/places/tests/legacy/test_bookmarks.js
@@ -119,17 +119,16 @@ add_task(async function test_bookmarks()
   Assert.ok(beforeInsert > 0);
 
   let newId = bs.insertBookmark(testRoot, uri("http://google.com/"),
                                 bs.DEFAULT_INDEX, "");
   Assert.equal(bookmarksObserver._itemAddedId, newId);
   Assert.equal(bookmarksObserver._itemAddedParent, testRoot);
   Assert.equal(bookmarksObserver._itemAddedIndex, testStartIndex);
   Assert.ok(bookmarksObserver._itemAddedURI.equals(uri("http://google.com/")));
-  Assert.equal(bs.getBookmarkURI(newId).spec, "http://google.com/");
 
   // after just inserting, modified should not be set
   let lastModified = PlacesUtils.toPRTime((await PlacesUtils.bookmarks.fetch(
     await PlacesUtils.promiseItemGuid(newId))).lastModified);
 
   // The time before we set the title, in microseconds.
   let beforeSetTitle = Date.now() * 1000;
   Assert.ok(beforeSetTitle >= beforeInsert);
@@ -304,37 +303,25 @@ add_task(async function test_bookmarks()
   let newId10 = bs.insertBookmark(testRoot, uri("http://foo10.com/"),
                                   bs.DEFAULT_INDEX, "");
 
   // Workaround possible VM timers issues moving lastModified and dateAdded
   // to the past.
   lastModified -= 1000;
   bs.setItemLastModified(newId10, lastModified);
 
-  // test getBookmarkURI
-  let newId11 = bs.insertBookmark(testRoot, uri("http://foo10.com/"),
-                                  bs.DEFAULT_INDEX, "");
-  let bmURI = bs.getBookmarkURI(newId11);
-  Assert.equal("http://foo10.com/", bmURI.spec);
-
-  // test getBookmarkURI with non-bookmark items
-  try {
-    bs.getBookmarkURI(testRoot);
-    do_throw("getBookmarkURI() should throw for non-bookmark items!");
-  } catch (ex) {}
-
   // insert a bookmark with title ZZZXXXYYY and then search for it.
   // this test confirms that we can find bookmarks that we haven't visited
   // (which are "hidden") and that we can find by title.
   // see bug #369887 for more details
   let newId13 = bs.insertBookmark(testRoot, uri("http://foobarcheese.com/"),
                                   bs.DEFAULT_INDEX, "");
   Assert.equal(bookmarksObserver._itemAddedId, newId13);
   Assert.equal(bookmarksObserver._itemAddedParent, testRoot);
-  Assert.equal(bookmarksObserver._itemAddedIndex, 7);
+  Assert.equal(bookmarksObserver._itemAddedIndex, 6);
 
   // set bookmark title
   bs.setItemTitle(newId13, "ZZZXXXYYY");
   Assert.equal(bookmarksObserver._itemChangedId, newId13);
   Assert.equal(bookmarksObserver._itemChangedProperty, "title");
   Assert.equal(bookmarksObserver._itemChangedValue, "ZZZXXXYYY");
 
   // check if setting an item annotation triggers onItemChanged