Bug 1437040 - Remove synchronous Bookmarks::GetItemIndex. r=standard8
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 09 Feb 2018 15:23:21 +0100
changeset 403377 48b37a1bc455b6986b75335fbbba2c58642ff381
parent 403376 f0e9bf0f4eef518bd2da3139022754eb7dff81ce
child 403378 98d71952c33ad1a5a0f2ac7f231206b8f62367d5
push id33432
push useraciure@mozilla.com
push dateMon, 12 Feb 2018 22:07:25 +0000
treeherdermozilla-central@a0afa134baa3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstandard8
bugs1437040
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1437040 - Remove synchronous Bookmarks::GetItemIndex. r=standard8 MozReview-Commit-ID: GpNkBI1Qe42
browser/components/places/PlacesUIUtils.jsm
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/tests/legacy/test_bookmarks.js
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -880,18 +880,18 @@ this.PlacesUIUtils = {
         // This will throw if the annotation is an orphan.
         bs.removeItem(aItemId);
       } catch (e) { /* orphan anno */ }
     }
 
     // Returns true if item really exists, false otherwise.
     function itemExists(aItemId) {
       try {
-        let index = bs.getItemIndex(aItemId);
-        return index > -1;
+        bs.getFolderIdForItem(aItemId);
+        return true;
       } catch (e) {
         return false;
       }
     }
 
     // Get all items marked as being the left pane folder.
     let items = as.getItemsWithAnnotation(this.ORGANIZER_FOLDER_ANNO);
     if (items.length > 1) {
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -455,19 +455,20 @@ add_task(async function test_onItemAdded
     let syncBmkGUID = await PlacesUtils.promiseItemGuid(syncBmkID);
     await verifyTrackedItems([syncFolderGUID, syncBmkGUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
 
     await resetTracker();
     await startTracking();
 
     _("Insert a separator using the sync API");
+    let index = (await PlacesUtils.bookmarks.fetch(syncFolderGUID)).index;
     let syncSepID = PlacesUtils.bookmarks.insertSeparator(
       PlacesUtils.bookmarks.bookmarksMenuFolder,
-      PlacesUtils.bookmarks.getItemIndex(syncFolderID));
+      index);
     let syncSepGUID = await PlacesUtils.promiseItemGuid(syncSepID);
     await verifyTrackedItems(["menu", syncSepGUID]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -525,19 +525,16 @@ interface nsINavBookmarksService : nsISu
                            [optional] in unsigned short aSource);
 
   /**
    * Get the URI for a bookmark item.
    */
   nsIURI getBookmarkURI(in long long aItemId);
 
   /**
-   * Get the index for an item.
-   */
-  long getItemIndex(in long long aItemId);
 
   /**
    * Get the parent folder's id for an item.
    */
   long long getFolderIdForItem(in long long aItemId);
 
   /**
    * Associates the given keyword with the given bookmark.
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -2242,33 +2242,16 @@ nsNavBookmarks::GetBookmarksForURI(nsIUR
     NS_ENSURE_SUCCESS(rv, rv);
 
     NS_ENSURE_TRUE(aBookmarks.AppendElement(bookmark), NS_ERROR_OUT_OF_MEMORY);
   }
 
   return NS_OK;
 }
 
-NS_IMETHODIMP
-nsNavBookmarks::GetItemIndex(int64_t aItemId, int32_t* _index)
-{
-  NS_ENSURE_ARG_MIN(aItemId, 1);
-  NS_ENSURE_ARG_POINTER(_index);
-
-  BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark);
-  // With respect to the API.
-  if (NS_FAILED(rv)) {
-    *_index = -1;
-    return NS_OK;
-  }
-
-  *_index = bookmark.position;
-  return NS_OK;
-}
 
 
 NS_IMETHODIMP
 nsNavBookmarks::SetKeywordForBookmark(int64_t aBookmarkId,
                                       const nsAString& aUserCasedKeyword,
                                       uint16_t aSource)
 {
   NS_ENSURE_ARG_MIN(aBookmarkId, 1);
--- a/toolkit/components/places/tests/legacy/test_bookmarks.js
+++ b/toolkit/components/places/tests/legacy/test_bookmarks.js
@@ -108,19 +108,16 @@ add_task(async function test_bookmarks()
   let testRoot = bs.createFolder(root, "places bookmarks xpcshell tests",
                                  bs.DEFAULT_INDEX);
   Assert.equal(bookmarksObserver._itemAddedId, testRoot);
   Assert.equal(bookmarksObserver._itemAddedParent, root);
   Assert.equal(bookmarksObserver._itemAddedIndex, bmStartIndex);
   Assert.equal(bookmarksObserver._itemAddedURI, null);
   let testStartIndex = 0;
 
-  // test getItemIndex for folders
-  Assert.equal(bs.getItemIndex(testRoot), bmStartIndex);
-
   // insert a bookmark.
   // the time before we insert, in microseconds
   let beforeInsert = Date.now() * 1000;
   Assert.ok(beforeInsert > 0);
 
   let newId = bs.insertBookmark(testRoot, uri("http://google.com/"),
                                 bs.DEFAULT_INDEX, "");
   Assert.equal(bookmarksObserver._itemAddedId, newId);
@@ -166,19 +163,16 @@ add_task(async function test_bookmarks()
     bs.getItemTitle(-3);
     do_throw("getItemTitle accepted bad input");
   } catch (ex) {}
 
   // get the folder that the bookmark is in
   let folderId = bs.getFolderIdForItem(newId);
   Assert.equal(folderId, testRoot);
 
-  // test getItemIndex for bookmarks
-  Assert.equal(bs.getItemIndex(newId), testStartIndex);
-
   // create a folder at a specific index
   let workFolder = bs.createFolder(testRoot, "Work", 0);
   Assert.equal(bookmarksObserver._itemAddedId, workFolder);
   Assert.equal(bookmarksObserver._itemAddedParent, testRoot);
   Assert.equal(bookmarksObserver._itemAddedIndex, 0);
   Assert.equal(bookmarksObserver._itemAddedURI, null);
 
   Assert.equal(bs.getItemTitle(workFolder), "Work");
@@ -257,33 +251,31 @@ add_task(async function test_bookmarks()
   bs.moveItem(workFolder, homeFolder, bs.DEFAULT_INDEX);
   Assert.equal(bookmarksObserver._itemMovedId, workFolder);
   Assert.equal(bookmarksObserver._itemMovedOldParent, testRoot);
   Assert.equal(bookmarksObserver._itemMovedOldIndex, 0);
   Assert.equal(bookmarksObserver._itemMovedNewParent, homeFolder);
   Assert.equal(bookmarksObserver._itemMovedNewIndex, 1);
 
   // test that the new index is properly stored
-  Assert.equal(bs.getItemIndex(workFolder), 1);
   Assert.equal(bs.getFolderIdForItem(workFolder), homeFolder);
 
   // XXX expose FolderCount, and check that the old parent has one less child?
   Assert.equal(getChildCount(testRoot), oldParentCC - 1);
 
   // move item, appending, to different folder
   bs.moveItem(newId5, testRoot, bs.DEFAULT_INDEX);
   Assert.equal(bookmarksObserver._itemMovedId, newId5);
   Assert.equal(bookmarksObserver._itemMovedOldParent, homeFolder);
   Assert.equal(bookmarksObserver._itemMovedOldIndex, 0);
   Assert.equal(bookmarksObserver._itemMovedNewParent, testRoot);
   Assert.equal(bookmarksObserver._itemMovedNewIndex, 3);
 
   // test get folder's index
   let tmpFolder = bs.createFolder(testRoot, "tmp", 2);
-  Assert.equal(bs.getItemIndex(tmpFolder), 2);
 
   // test setKeywordForBookmark
   let kwTestItemId = bs.insertBookmark(testRoot, uri("http://keywordtest.com"),
                                        bs.DEFAULT_INDEX, "");
   bs.setKeywordForBookmark(kwTestItemId, "bar");
 
   // test getKeywordForBookmark
   let k = bs.getKeywordForBookmark(kwTestItemId);
@@ -397,30 +389,25 @@ add_task(async function test_bookmarks()
   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) {}
 
-  // test getItemIndex
-  let newId12 = bs.insertBookmark(testRoot, uri("http://foo10.com/"), 1, "");
-  let bmIndex = bs.getItemIndex(newId12);
-  Assert.equal(1, bmIndex);
-
   // 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, 11);
+  Assert.equal(bookmarksObserver._itemAddedIndex, 10);
 
   // 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