Bug 1432435 - Remove synchronous Bookmarks::getBookmarkURI. r=mak

MozReview-Commit-ID: GSQOoSoeibq

--HG--
extra : rebase_source : 836cba69b085e8c3751b79f1fdf32eb023881369

browser/components/places/tests/browser/browser_library_views_liveupdate.js

@@ -8,218 +8,185 @@
 
 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]);
   }
-
-  // Remove observers.
-  PlacesUtils.bookmarks.removeObserver(bookmarksObserver);
-  PlacesUtils.annotations.removeObserver(bookmarksObserver);
-
-  await promiseLibraryClosed(gLibrary);
 });
 
-/**
- * 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
-  ]),
+async function insertAndCheckItem(itemData, expectedIndex) {
+  let item = await PlacesUtils.bookmarks.insert(itemData);
 
-  // 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");
+  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;
-      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");
-    }
+      }
+      // 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");
+  }
+
+  return item;
+}
+
+async function updateAndCheckItem(newItemData, expectedIndex) {
+  await PlacesUtils.bookmarks.update(newItemData);
+
+  let [node, index, title] = getNodeForTreeItem(newItemData.guid, gLibrary.PlacesOrganizer._places);
+
+  // 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");
+}
 
 /**
- * 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)) {
@@ -238,7 +205,7 @@ function getNodeForTreeItem(aItemId, aTree, aValidator) {
       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.
@@ -253,5 +220,5 @@ function getNodeForTreeItem(aItemId, aTree, aValidator) {
     if (foundNode[0] != null)
       return foundNode;
   }
-  return [null, null, false];
+  return [null, null, ""];
 }

toolkit/components/places/nsINavBookmarksService.idl

@@ -459,20 +459,12 @@ interface nsINavBookmarksService : nsISupports
                            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

toolkit/components/places/nsNavBookmarks.cpp

@@ -1584,7 +1584,7 @@ nsNavBookmarks::GetItemTitle(int64_t aItemId,
 }
 
 
-NS_IMETHODIMP
+nsresult
 nsNavBookmarks::GetBookmarkURI(int64_t aItemId,
                                nsIURI** _URI)
 {

toolkit/components/places/nsNavBookmarks.h

@@ -119,6 +119,8 @@ public:
                    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);

toolkit/components/places/tests/legacy/test_bookmarks.js

@@ -124,7 +124,6 @@ add_task(async function test_bookmarks() {
   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(
@@ -309,18 +308,6 @@ add_task(async function test_bookmarks() {
   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.
@@ -329,7 +316,7 @@ add_task(async function test_bookmarks() {
                                   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");