Bug 1437040 - Remove synchronous Bookmarks::GetItemIndex. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 09 Feb 2018 15:23:21 +0100
changeset 753816 3a17c1f81aa4375a4a111ed2d17ffad241a77d81
parent 753815 3ee38289dac8838fe848f7234d75f3cef5d3dbc7
push id98685
push usermak77@bonardo.net
push dateMon, 12 Feb 2018 13:47:25 +0000
reviewersstandard8
bugs1437040
milestone60.0a1
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