Bug 539517 - remove synchronous Bookmarks::SetItemIndex API. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 08 Feb 2018 18:45:00 +0100
changeset 752974 7ac31306185573f6f94bcef4e89257d6a2ea2c2d
parent 752971 a38aa91eb3db3611c7e75d98992efc20abcea2fd
push id98431
push usermak77@bonardo.net
push dateFri, 09 Feb 2018 10:23:27 +0000
reviewersstandard8
bugs539517
milestone60.0a1
Bug 539517 - remove synchronous Bookmarks::SetItemIndex API. r=standard8 MozReview-Commit-ID: 20kwLPazqPC
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/nsINavBookmarksService.idl
toolkit/components/places/nsNavBookmarks.cpp
toolkit/components/places/tests/unit/test_sync_utils.js
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -1213,18 +1213,18 @@ add_task(async function test_async_onIte
     _(`Mozilla GUID: ${mozBmk.guid}`);
 
     await startTracking();
 
     _("Reorder bookmarks");
     await PlacesUtils.bookmarks.reorder(PlacesUtils.bookmarks.menuGuid,
       [mozBmk.guid, fxBmk.guid, tbBmk.guid]);
 
-    // As with setItemIndex, we should only track the folder if we reorder
-    // its children, but we should bump the score for every changed item.
+    // We only track the folder if we reorder its children, but we should
+    // bump the score for every changed item.
     await verifyTrackedItems(["menu"]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
 
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -556,37 +556,16 @@ interface nsINavBookmarksService : nsISu
   nsIURI getBookmarkURI(in long long aItemId);
 
   /**
    * Get the index for an item.
    */
   long getItemIndex(in long long aItemId);
 
   /**
-   * Changes the index for a item. This method does not change the indices of
-   * any other items in the same folder, so ensure that the new index does not
-   * already exist, or change the index of other items accordingly, otherwise
-   * the indices will become corrupted.
-   *
-   * WARNING: This is API is intended for scenarios such as folder sorting,
-   *          where the caller manages the indices of *all* items in the folder.
-   *          You must always ensure each index is unique after a reordering.
-   *
-   *  @param aItemId    The id of the item to modify
-   *  @param aNewIndex  The new index
-   *  @param aSource    The optional change source, forwarded to all bookmark
-   *                    observers. Defaults to SOURCE_DEFAULT.
-   *
-   *  @throws If aNewIndex is out of bounds.
-   */
-  void setItemIndex(in long long aItemId,
-                    in long aNewIndex,
-                    [optional] in unsigned short aSource);
-
-  /**
    * Get the parent folder's id for an item.
    */
   long long getFolderIdForItem(in long long aItemId);
 
   /**
    * Associates the given keyword with the given bookmark.
    *
    * Use an empty keyword to clear the keyword associated with the URI.
--- a/toolkit/components/places/nsNavBookmarks.cpp
+++ b/toolkit/components/places/nsNavBookmarks.cpp
@@ -2352,107 +2352,16 @@ nsNavBookmarks::GetItemIndex(int64_t aIt
     *_index = -1;
     return NS_OK;
   }
 
   *_index = bookmark.position;
   return NS_OK;
 }
 
-NS_IMETHODIMP
-nsNavBookmarks::SetItemIndex(int64_t aItemId,
-                             int32_t aNewIndex,
-                             uint16_t aSource)
-{
-  NS_ENSURE_ARG_MIN(aItemId, 1);
-  NS_ENSURE_ARG_MIN(aNewIndex, 0);
-
-  BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aItemId, bookmark);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // Ensure we are not going out of range.
-  int32_t folderCount;
-  int64_t grandParentId;
-  nsAutoCString folderGuid;
-  rv = FetchFolderInfo(bookmark.parentId, &folderCount, folderGuid, &grandParentId);
-  NS_ENSURE_SUCCESS(rv, rv);
-  NS_ENSURE_TRUE(aNewIndex < folderCount, NS_ERROR_INVALID_ARG);
-  // Check the parent's guid is the expected one.
-  MOZ_ASSERT(bookmark.parentGuid == folderGuid);
-
-  mozStorageTransaction transaction(mDB->MainConn(), false);
-
-  {
-    nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
-      "UPDATE moz_bookmarks SET "
-       "position = :item_index "
-      "WHERE id = :item_id"
-    );
-    NS_ENSURE_STATE(stmt);
-    mozStorageStatementScoper scoper(stmt);
-
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), aItemId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->BindInt32ByName(NS_LITERAL_CSTRING("item_index"), aNewIndex);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = stmt->Execute();
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  int64_t syncChangeDelta = DetermineSyncChangeDelta(aSource);
-
-  {
-    // Sync stores child indices in the parent's record, so we only need to
-    // bump the parent's change counter.
-    nsCOMPtr<mozIStorageStatement> stmt = mDB->GetStatement(
-      "UPDATE moz_bookmarks SET "
-       "syncChangeCounter = syncChangeCounter + :delta "
-      "WHERE id = :parent_id"
-    );
-    NS_ENSURE_STATE(stmt);
-    mozStorageStatementScoper scoper(stmt);
-
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("parent_id"),
-                               bookmark.parentId);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("delta"),
-                               syncChangeDelta);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = stmt->Execute();
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  rv = AdjustSeparatorsSyncCounter(bookmark.parentId, aNewIndex, syncChangeDelta);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  rv = PreventSyncReparenting(bookmark);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  rv = transaction.Commit();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                   nsINavBookmarkObserver,
-                   OnItemMoved(bookmark.id,
-                               bookmark.parentId,
-                               bookmark.position,
-                               bookmark.parentId,
-                               aNewIndex,
-                               bookmark.type,
-                               bookmark.guid,
-                               bookmark.parentGuid,
-                               bookmark.parentGuid,
-                               aSource));
-
-  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/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -1781,31 +1781,16 @@ add_task(async function test_set_orphan_
   info("Verify synced orphan annos match");
   {
     let orphanGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
       SYNC_PARENT_ANNO, nonexistentRecordId);
     deepEqual(orphanGuids.sort(), [fxBmk.recordId, tbBmk.recordId].sort(),
       "Orphaned bookmarks should match before changing indices");
   }
 
-  info("Set synced orphan indices");
-  {
-    let fxId = await recordIdToId(fxBmk.recordId);
-    let tbId = await recordIdToId(tbBmk.recordId);
-    PlacesUtils.bookmarks.runInBatchMode(_ => {
-      PlacesUtils.bookmarks.setItemIndex(fxId, 1);
-      PlacesUtils.bookmarks.setItemIndex(tbId, 0);
-    }, null);
-    await PlacesTestUtils.promiseAsyncUpdates();
-    let orphanGuids = await PlacesSyncUtils.bookmarks.fetchGuidsWithAnno(
-      SYNC_PARENT_ANNO, nonexistentRecordId);
-    deepEqual(orphanGuids, [],
-      "Should remove orphan annos after updating indices");
-  }
-
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_unsynced_orphans() {
   let nonexistentRecordId = makeGuid();
   let newBmk = await PlacesSyncUtils.bookmarks.insert({
     kind: "bookmark",