Bug 1432437 - Remove synchronous changeBookmarkURI. r=standard8 draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 24 Jan 2018 12:13:02 +0100
changeset 747570 84958fcaaacdcdd9a400a546b14eb49fc897752c
parent 747564 329bfa4b804a69d67cd0018d70bbf4cc970f4a64
push id96938
push usermak77@bonardo.net
push dateFri, 26 Jan 2018 11:20:20 +0000
reviewersstandard8
bugs1432437
milestone60.0a1
Bug 1432437 - Remove synchronous changeBookmarkURI. r=standard8 MozReview-Commit-ID: Jy76eKCN7wp
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
toolkit/components/places/tests/legacy/test_changeBookmarkURI.js
toolkit/components/places/tests/legacy/xpcshell.ini
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
@@ -594,36 +594,37 @@ add_task(async function test_onItemChang
 
 add_task(async function test_onItemChanged_changeBookmarkURI() {
   _("Changes to bookmark URIs should be tracked");
 
   try {
     await stopTracking();
 
     _("Insert a bookmark");
-    let fx_id = PlacesUtils.bookmarks.insertBookmark(
-      PlacesUtils.bookmarks.bookmarksMenuFolder,
-      CommonUtils.makeURI("http://getfirefox.com"),
-      PlacesUtils.bookmarks.DEFAULT_INDEX,
-      "Get Firefox!");
-    let fx_guid = await engine._store.GUIDForId(fx_id);
-    _(`Firefox GUID: ${fx_guid}`);
+    let bm = await PlacesUtils.bookmarks.insert({
+      parentGuid: PlacesUtils.bookmarks.menuGuid,
+      url: "http://getfirefox.com",
+      title: "Get Firefox!"
+    });
+    _(`Firefox GUID: ${bm.guid}`);
 
     _("Set a tracked annotation to make sure we only notify once");
+    let id = await PlacesUtils.promiseItemId(bm.guid);
     PlacesUtils.annotations.setItemAnnotation(
-      fx_id, PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO, "A test description", 0,
+      id, PlacesSyncUtils.bookmarks.DESCRIPTION_ANNO, "A test description", 0,
       PlacesUtils.annotations.EXPIRE_NEVER);
 
     await startTracking();
 
     _("Change the bookmark's URI");
-    PlacesUtils.bookmarks.changeBookmarkURI(fx_id,
-      CommonUtils.makeURI("https://www.mozilla.org/firefox"));
-    await verifyTrackedItems([fx_guid]);
-    Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
+    bm.url = "https://www.mozilla.org/firefox";
+    bm = await PlacesUtils.bookmarks.update(bm);
+
+    await verifyTrackedItems([bm.guid]);
+    Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
 
 add_task(async function test_onItemTagged() {
   _("Items tagged using the synchronous API should be tracked");
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -601,25 +601,16 @@ interface nsINavBookmarksService : nsISu
    *
    *  @throws If aNewIndex is out of bounds.
    */
   void setItemIndex(in long long aItemId,
                     in long aNewIndex,
                     [optional] in unsigned short aSource);
 
   /**
-   * Change the bookmarked URI for a bookmark.
-   * This changes which "place" the bookmark points at,
-   * which means all annotations, etc are carried along.
-   */
-  void changeBookmarkURI(in long long aItemId,
-                         in nsIURI aNewURI,
-                         [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
@@ -2344,106 +2344,16 @@ nsNavBookmarks::FetchFolderInfo(int64_t 
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
 
 
 NS_IMETHODIMP
-nsNavBookmarks::ChangeBookmarkURI(int64_t aBookmarkId, nsIURI* aNewURI,
-                                  uint16_t aSource)
-{
-  NS_ENSURE_ARG_MIN(aBookmarkId, 1);
-  NS_ENSURE_ARG(aNewURI);
-
-  BookmarkData bookmark;
-  nsresult rv = FetchItemInfo(aBookmarkId, bookmark);
-  NS_ENSURE_SUCCESS(rv, rv);
-  NS_ENSURE_ARG(bookmark.type == TYPE_BOOKMARK);
-
-  mozStorageTransaction transaction(mDB->MainConn(), false);
-
-  int64_t tagsRootId = TagsRootId();
-  bool isTagging = bookmark.grandParentId == tagsRootId;
-  int64_t syncChangeDelta = DetermineSyncChangeDelta(aSource);
-
-  nsNavHistory* history = nsNavHistory::GetHistoryService();
-  NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
-  int64_t newPlaceId;
-  nsAutoCString newPlaceGuid;
-  rv = history->GetOrCreateIdForPage(aNewURI, &newPlaceId, newPlaceGuid);
-  NS_ENSURE_SUCCESS(rv, rv);
-  if (!newPlaceId)
-    return NS_ERROR_INVALID_ARG;
-
-  nsCOMPtr<mozIStorageStatement> statement = mDB->GetStatement(
-    "UPDATE moz_bookmarks SET "
-     "fk = :page_id, lastModified = :date, "
-     "syncChangeCounter = syncChangeCounter + :delta "
-    "WHERE id = :item_id "
-  );
-  NS_ENSURE_STATE(statement);
-  mozStorageStatementScoper scoper(statement);
-
-  rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), newPlaceId);
-  NS_ENSURE_SUCCESS(rv, rv);
-  bookmark.lastModified = RoundedPRNow();
-  rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("date"),
-                                  bookmark.lastModified);
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("item_id"), bookmark.id);
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = statement->BindInt64ByName(NS_LITERAL_CSTRING("delta"), syncChangeDelta);
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = statement->Execute();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  if (isTagging) {
-    // For consistency with the tagging service behavior, changing a tag entry's
-    // URL bumps the change counter for bookmarks with the old and new URIs.
-    rv = AddSyncChangesForBookmarksWithURL(bookmark.url, syncChangeDelta);
-    NS_ENSURE_SUCCESS(rv, rv);
-    rv = AddSyncChangesForBookmarksWithURI(aNewURI, syncChangeDelta);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
-  rv = transaction.Commit();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  rv = history->UpdateFrecency(newPlaceId);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // Upon changing the URI for a bookmark, update the frecency for the old
-  // place as well.
-  rv = history->UpdateFrecency(bookmark.placeId);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  nsAutoCString spec;
-  rv = aNewURI->GetSpec(spec);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                   nsINavBookmarkObserver,
-                   OnItemChanged(bookmark.id,
-                                 NS_LITERAL_CSTRING("uri"),
-                                 false,
-                                 spec,
-                                 bookmark.lastModified,
-                                 bookmark.type,
-                                 bookmark.parentId,
-                                 bookmark.guid,
-                                 bookmark.parentGuid,
-                                 bookmark.url,
-                                 aSource));
-  return NS_OK;
-}
-
-
-NS_IMETHODIMP
 nsNavBookmarks::GetFolderIdForItem(int64_t aItemId, int64_t* _parentId)
 {
   NS_ENSURE_ARG_MIN(aItemId, 1);
   NS_ENSURE_ARG_POINTER(_parentId);
 
   BookmarkData bookmark;
   nsresult rv = FetchItemInfo(aItemId, bookmark);
   NS_ENSURE_SUCCESS(rv, rv);
--- a/toolkit/components/places/tests/legacy/test_bookmarks.js
+++ b/toolkit/components/places/tests/legacy/test_bookmarks.js
@@ -420,46 +420,30 @@ add_task(async function test_bookmarks()
 
   // Workaround possible VM timers issues moving lastModified and dateAdded
   // to the past.
   lastModified -= 1000;
   bs.setItemLastModified(newId10, lastModified);
   dateAdded -= 1000;
   bs.setItemDateAdded(newId10, dateAdded);
 
-  bs.changeBookmarkURI(newId10, uri("http://foo11.com/"));
-
-  // check that lastModified is set after we change the bookmark uri
-  lastModified2 = bs.getItemLastModified(newId10);
-  info("test changeBookmarkURI");
-  info("dateAdded = " + dateAdded);
-  info("lastModified = " + lastModified);
-  info("lastModified2 = " + lastModified2);
-  Assert.ok(is_time_ordered(lastModified, lastModified2));
-  Assert.ok(is_time_ordered(dateAdded, lastModified2));
-
-  Assert.equal(bookmarksObserver._itemChangedId, newId10);
-  Assert.equal(bookmarksObserver._itemChangedProperty, "uri");
-  Assert.equal(bookmarksObserver._itemChangedValue, "http://foo11.com/");
-  Assert.equal(bookmarksObserver._itemChangedOldValue, "http://foo10.com/");
-
   // test getBookmarkURI
-  let newId11 = bs.insertBookmark(testRoot, uri("http://foo11.com/"),
+  let newId11 = bs.insertBookmark(testRoot, uri("http://foo10.com/"),
                                   bs.DEFAULT_INDEX, "");
   let bmURI = bs.getBookmarkURI(newId11);
-  Assert.equal("http://foo11.com/", bmURI.spec);
+  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://foo11.com/"), 1, "");
+  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/"),
deleted file mode 100644
--- a/toolkit/components/places/tests/legacy/test_changeBookmarkURI.js
+++ /dev/null
@@ -1,62 +0,0 @@
-/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
-/* vim:set ts=2 sw=2 sts=2 et: */
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-// Get bookmark service
-var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
-            getService(Ci.nsINavBookmarksService);
-
-/**
- * Ensures that the Places APIs recognize that aBookmarkedUri is bookmarked
- * via aBookmarkId and that aUnbookmarkedUri is not bookmarked at all.
- *
- * @param aBookmarkId
- *        an item ID whose corresponding URI is aBookmarkedUri
- * @param aBookmarkedUri
- *        a bookmarked URI that has a corresponding item ID aBookmarkId
- * @param aUnbookmarkedUri
- *        a URI that is not currently bookmarked at all
- */
-async function checkUris(aBookmarkId, aBookmarkedUri, aUnbookmarkedUri) {
-  // Ensure that aBookmarkedUri equals some URI that is bookmarked
-  let bm = await PlacesUtils.bookmarks.fetch({url: aBookmarkedUri});
-
-  Assert.notEqual(bm, null);
-  Assert.ok(uri(bm.url).equals(aBookmarkedUri));
-
-  // Ensure that the URI corresponding to aBookmarkId equals aBookmarkedUri
-  Assert.ok(bmsvc.getBookmarkURI(aBookmarkId).equals(aBookmarkedUri));
-
-  // Ensure that aUnbookmarkedUri does not equal any URI that is bookmarked
-  bm = await PlacesUtils.bookmarks.fetch({url: aUnbookmarkedUri});
-  Assert.equal(bm, null);
-}
-
-// main
-add_task(async function() {
-  // Create a folder
-  var folderId = bmsvc.createFolder(bmsvc.toolbarFolder,
-                                    "test",
-                                    bmsvc.DEFAULT_INDEX);
-
-  // Create 2 URIs
-  var uri1 = uri("http://www.dogs.com/");
-  var uri2 = uri("http://www.cats.com/");
-
-  // Bookmark the first one
-  var bookmarkId = bmsvc.insertBookmark(folderId,
-                                        uri1,
-                                        bmsvc.DEFAULT_INDEX,
-                                        "Dogs");
-
-  // uri1 is bookmarked via bookmarkId, uri2 is not
-  await checkUris(bookmarkId, uri1, uri2);
-
-  // Change the URI of the bookmark to uri2
-  bmsvc.changeBookmarkURI(bookmarkId, uri2);
-
-  // uri2 is now bookmarked via bookmarkId, uri1 is not
-  await checkUris(bookmarkId, uri2, uri1);
-});
--- a/toolkit/components/places/tests/legacy/xpcshell.ini
+++ b/toolkit/components/places/tests/legacy/xpcshell.ini
@@ -3,10 +3,9 @@
 
 [DEFAULT]
 head = head_legacy.js
 firefox-appdir = browser
 
 [test_418643_removeFolderChildren.js]
 [test_bookmarks.js]
 [test_bookmarks_setNullTitle.js]
-[test_changeBookmarkURI.js]
 [test_protectRoots.js]
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -698,38 +698,37 @@ add_task(async function test_pullChanges
       "Tagging service should update cache after updating tag folder");
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
       [firstItem.recordId, secondItem.recordId].sort(),
       "Should include tagged bookmarks after updating tag folder");
     await setChangesSynced(changes);
   }
 
-  info("Change tag entry URI using Bookmarks.changeBookmarkURI");
+  info("Change tag entry URL using Bookmarks.update");
   {
-    let tagId = PlacesUtils.bookmarks.getIdForItemAt(tagFolderId, 0);
-    PlacesUtils.bookmarks.changeBookmarkURI(tagId, uri("https://bugzilla.org"));
+    let tagGuid = await PlacesUtils.promiseItemGuid(
+      PlacesUtils.bookmarks.getIdForItemAt(tagFolderId, 0));
+    await PlacesUtils.bookmarks.update({
+      guid: tagGuid,
+      url: "https://bugzilla.org",
+    });
     let changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
       [firstItem.recordId, secondItem.recordId, untaggedItem.recordId].sort(),
       "Should include tagged bookmarks after changing tag entry URI");
     assertTagForURLs("tricky", ["https://bugzilla.org/", "https://mozilla.org/"],
       "Should remove tag entry for old URI");
     await setChangesSynced(changes);
-  }
-
-  info("Change tag entry URL using Bookmarks.update");
-  {
-    let tagGuid = await PlacesUtils.promiseItemGuid(
-      PlacesUtils.bookmarks.getIdForItemAt(tagFolderId, 0));
+
     await PlacesUtils.bookmarks.update({
       guid: tagGuid,
       url: "https://example.com",
     });
-    let changes = await PlacesSyncUtils.bookmarks.pullChanges();
+    changes = await PlacesSyncUtils.bookmarks.pullChanges();
     deepEqual(Object.keys(changes).sort(),
       [untaggedItem.recordId].sort(),
       "Should include tagged bookmarks after changing tag entry URL");
     assertTagForURLs("tricky", ["https://example.com/", "https://mozilla.org/"],
       "Should remove tag entry for old URL");
     await setChangesSynced(changes);
   }