Bug 1474033 - Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8
authorLina Cambridge <lina@yakshaving.ninja>
Mon, 09 Jul 2018 14:12:41 +0000
changeset 425492 a4e025f18962c37d5a3ae389dada0b8b699c8d2f
parent 425491 c7bd2557255a7b57fb94294d5c4a55ed85c190c3
child 425493 9912325cd0737296ecdd10d4cfc810bcafcd519d
push id34255
push useraciure@mozilla.com
push dateMon, 09 Jul 2018 21:58:13 +0000
treeherdermozilla-central@8e3c84df8f67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8
bugs1474033
milestone63.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 1474033 - Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8 This patch unifies `updateBookmark` and `moveBookmark`. `update` already handles the Sync change counter, and `move` specially optimizes for moves. We can consolidate the two by reusing the queries from `move` in `update`, moving its extra `setAncestorsLastModified` call to `PlacesUtils.bookmarks.update`, and removing its transaction and wrapper. Differential Revision: https://phabricator.services.mozilla.com/D2016
services/sync/tests/unit/test_bookmark_tracker.js
toolkit/components/places/Bookmarks.jsm
--- a/services/sync/tests/unit/test_bookmark_tracker.js
+++ b/services/sync/tests/unit/test_bookmark_tracker.js
@@ -873,16 +873,82 @@ add_task(async function test_onLivemarkD
     await verifyTrackedItems(["menu", livemark.guid]);
     Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE);
   } finally {
     _("Clean up.");
     await cleanup();
   }
 });
 
+add_task(async function test_async_onItemMoved_moveToFolder() {
+  _("Items moved via `moveToFolder` should be tracked");
+
+  try {
+    await tracker.stop();
+
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.menuGuid,
+      children: [{
+        guid: "bookmarkAAAA",
+        title: "A",
+        url: "http://example.com/a",
+      }, {
+        guid: "bookmarkBBBB",
+        title: "B",
+        url: "http://example.com/b",
+      }, {
+        guid: "bookmarkCCCC",
+        title: "C",
+        url: "http://example.com/c",
+      }, {
+        guid: "bookmarkDDDD",
+        title: "D",
+        url: "http://example.com/d",
+      }],
+    });
+    await PlacesUtils.bookmarks.insertTree({
+      guid: PlacesUtils.bookmarks.toolbarGuid,
+      children: [{
+        guid: "bookmarkEEEE",
+        title: "E",
+        url: "http://example.com/e",
+      }],
+    });
+
+    await startTracking();
+
+    _("Move (A B D) to the toolbar");
+    await PlacesUtils.bookmarks.moveToFolder(
+      ["bookmarkAAAA", "bookmarkBBBB", "bookmarkDDDD"],
+      PlacesUtils.bookmarks.toolbarGuid,
+      PlacesUtils.bookmarks.DEFAULT_INDEX);
+
+    // Moving multiple bookmarks between two folders should track the old
+    // folder, new folder, and moved bookmarks.
+    await verifyTrackedItems(["menu", "toolbar", "bookmarkAAAA",
+      "bookmarkBBBB", "bookmarkDDDD"]);
+    Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 3);
+    await resetTracker();
+
+    _("Reorder toolbar children: (D A B E)");
+    await PlacesUtils.bookmarks.moveToFolder(
+      ["bookmarkDDDD", "bookmarkAAAA", "bookmarkBBBB"],
+      PlacesUtils.bookmarks.toolbarGuid,
+      0);
+
+    // Reordering bookmarks in a folder should only track the folder, not the
+    // bookmarks.
+    await verifyTrackedItems(["toolbar"]);
+    Assert.equal(tracker.score, SCORE_INCREMENT_XLARGE * 2);
+  } finally {
+    _("Clean up.");
+    await cleanup();
+  }
+});
+
 add_task(async function test_async_onItemMoved_update() {
   _("Items moved via the asynchronous API should be tracked");
 
   try {
     await tracker.stop();
 
     await PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -704,17 +704,29 @@ var Bookmarks = Object.freeze({
              updateInfo.index = parent._childCount;
 
             // Fix the index when moving within the same container.
             if (parent.guid == item.parentGuid)
                updateInfo.index--;
           }
         }
 
-        let updatedItem = await updateBookmark(updateInfo, item, parent);
+        let syncChangeDelta =
+          PlacesSyncUtils.bookmarks.determineSyncChangeDelta(info.source);
+
+        let updatedItem = await db.executeTransaction(async function() {
+          let updatedItem = await updateBookmark(db, updateInfo, item,
+                                                 item.index, parent,
+                                                 syncChangeDelta);
+          if (parent) {
+            await setAncestorsLastModified(db, parent.guid, updatedItem.lastModified,
+                                           syncChangeDelta);
+          }
+          return updatedItem;
+        });
 
         if (item.type == this.TYPE_BOOKMARK &&
             item.url.href != updatedItem.url.href) {
           // ...though we don't wait for the calculation.
           updateFrecency(db, [item.url]).catch(Cu.reportError);
           updateFrecency(db, [updatedItem.url]).catch(Cu.reportError);
         }
 
@@ -848,17 +860,17 @@ var Bookmarks = Object.freeze({
       source = this.SOURCES.DEFAULT;
     }
 
     return (async () => {
       let updateInfos = [];
       let syncChangeDelta =
         PlacesSyncUtils.bookmarks.determineSyncChangeDelta(source);
 
-      await PlacesUtils.withConnectionWrapper("Bookmarks.jsm: update", async db => {
+      await PlacesUtils.withConnectionWrapper("Bookmarks.jsm: moveToFolder", async db => {
         const lastModified = new Date();
 
         let targetParentGuid = parentGuid || undefined;
 
         for (let guid of guids) {
           // Ensure the item exists.
           let existingItem = await fetchBookmark({ guid }, false, db);
           if (!existingItem)
@@ -930,22 +942,21 @@ var Bookmarks = Object.freeze({
 
               // If this is moving within the same folder, then we need to drop the
               // index by one to compensate for "removing" it, then re-inserting.
               if (info.existingItem.parentGuid == newParent.guid) {
                 index--;
               }
             }
 
-            info.updatedItem = await moveBookmark(db,
+            info.updatedItem = await updateBookmark(db,
+              { lastModified, index },
               info.existingItem,
               info.currIndex,
               newParent,
-              index,
-              lastModified,
               syncChangeDelta);
 
             // For items moving within the same folder, we have to keep track
             // of their indexes. Otherwise we run the risk of not correctly
             // updating the indexes of other items in the folder.
             // This section simulates the database write in moveBookmark, which
             // allows us to avoid re-reading the database.
             if (info.existingItem.parentGuid == newParent.guid) {
@@ -1495,268 +1506,179 @@ function notify(observers, notification,
     try {
       observer[notification](...args);
     } catch (ex) {}
   }
 }
 
 // Update implementation.
 
-function updateBookmark(info, item, newParent) {
-  return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: updateBookmark",
-    async function(db) {
-
-    let tuples = new Map();
-    tuples.set("lastModified", { value: PlacesUtils.toPRTime(info.lastModified) });
-    if (info.hasOwnProperty("title"))
-      tuples.set("title", { value: info.title,
-                            fragment: `title = NULLIF(:title, "")` });
-    if (info.hasOwnProperty("dateAdded"))
-      tuples.set("dateAdded", { value: PlacesUtils.toPRTime(info.dateAdded) });
-
-    await db.executeTransaction(async function() {
-      let isTagging = item._grandParentId == PlacesUtils.tagsFolderId;
-      let syncChangeDelta =
-        PlacesSyncUtils.bookmarks.determineSyncChangeDelta(info.source);
-
-      if (info.hasOwnProperty("url")) {
-        // Ensure a page exists in moz_places for this URL.
-        await maybeInsertPlace(db, info.url);
-        // Update tuples for the update query.
-        tuples.set("url", { value: info.url.href,
-                            fragment: "fk = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url)" });
-      }
-
-      let newIndex = info.hasOwnProperty("index") ? info.index : item.index;
-      if (newParent) {
-        // For simplicity, update the index regardless.
-        tuples.set("position", { value: newIndex });
-
-        if (newParent.guid == item.parentGuid) {
-          // Moving inside the original container.
-          // When moving "up", add 1 to each index in the interval.
-          // Otherwise when moving down, we subtract 1.
-          // Only the parent needs a sync change, which is handled in
-          // `setAncestorsLastModified`.
-          let sign = newIndex < item.index ? +1 : -1;
-          await db.executeCached(
-            `UPDATE moz_bookmarks SET position = position + :sign
-             WHERE parent = :newParentId
-               AND position BETWEEN :lowIndex AND :highIndex
-            `, { sign, newParentId: newParent._id,
-                 lowIndex: Math.min(item.index, newIndex),
-                 highIndex: Math.max(item.index, newIndex) });
-        } else {
-          // Moving across different containers. In this case, both parents and
-          // the child need sync changes. `setAncestorsLastModified` handles the
-          // parents; the `needsSyncChange` check below handles the child.
-          tuples.set("parent", { value: newParent._id} );
-          await db.executeCached(
-            `UPDATE moz_bookmarks SET position = position + :sign
-             WHERE parent = :oldParentId
-               AND position >= :oldIndex
-            `, { sign: -1, oldParentId: item._parentId, oldIndex: item.index });
-          await db.executeCached(
-            `UPDATE moz_bookmarks SET position = position + :sign
-             WHERE parent = :newParentId
-               AND position >= :newIndex
-            `, { sign: +1, newParentId: newParent._id, newIndex });
-
-          await setAncestorsLastModified(db, item.parentGuid, info.lastModified,
-                                         syncChangeDelta);
-        }
-        await setAncestorsLastModified(db, newParent.guid, info.lastModified,
-                                       syncChangeDelta);
-      }
-
-      if (syncChangeDelta) {
-        // Sync stores child indices in the parent's record, so we only bump the
-        // item's counter if we're updating at least one more property in
-        // addition to the index, last modified time, and dateAdded.
-        let sizeThreshold = 1;
-        if (info.hasOwnProperty("index") && info.index != item.index) {
-          ++sizeThreshold;
-        }
-        if (tuples.has("dateAdded")) {
-          ++sizeThreshold;
-        }
-        let needsSyncChange = tuples.size > sizeThreshold;
-        if (needsSyncChange) {
-          tuples.set("syncChangeDelta", { value: syncChangeDelta,
-                                          fragment: "syncChangeCounter = syncChangeCounter + :syncChangeDelta" });
-        }
-      }
-
-      if (isTagging) {
-        // If we're updating a tag entry, bump the sync change counter for
-        // bookmarks with the tagged URL.
-        await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
-          db, item.url, syncChangeDelta);
-        if (info.hasOwnProperty("url")) {
-          // Changing the URL of a tag entry is equivalent to untagging the
-          // old URL and tagging the new one, so we bump the change counter
-          // for the new URL here.
-          await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
-            db, info.url, syncChangeDelta);
-        }
-      }
-
-      let isChangingTagFolder = item._parentId == PlacesUtils.tagsFolderId;
-      if (isChangingTagFolder) {
-        // If we're updating a tag folder (for example, changing a tag's title),
-        // bump the change counter for all tagged bookmarks.
-        await addSyncChangesForBookmarksInFolder(db, item, syncChangeDelta);
-      }
-
-      await db.executeCached(
-        `UPDATE moz_bookmarks
-         SET ${Array.from(tuples.keys()).map(v => tuples.get(v).fragment || `${v} = :${v}`).join(", ")}
-         WHERE guid = :guid
-        `, Object.assign({ guid: info.guid },
-                         [...tuples.entries()].reduce((p, c) => { p[c[0]] = c[1].value; return p; }, {})));
-
-      if (newParent) {
-        if (newParent.guid == item.parentGuid) {
-          // Mark all affected separators as changed
-          // Also bumps the change counter if the item itself is a separator
-          const startIndex = Math.min(newIndex, item.index);
-          await adjustSeparatorsSyncCounter(db, newParent._id, startIndex, syncChangeDelta);
-        } else {
-          // Mark all affected separators as changed
-          await adjustSeparatorsSyncCounter(db, item._parentId, item.index, syncChangeDelta);
-          await adjustSeparatorsSyncCounter(db, newParent._id, newIndex, syncChangeDelta);
-        }
-        // Remove the Sync orphan annotation from reparented items. We don't
-        // notify annotation observers about this because this is a temporary,
-        // internal anno that's only used by Sync.
-        await db.executeCached(
-          `DELETE FROM moz_items_annos
-           WHERE anno_attribute_id = (SELECT id FROM moz_anno_attributes
-                                      WHERE name = :orphanAnno) AND
-                 item_id = :id`,
-          { orphanAnno: PlacesSyncUtils.bookmarks.SYNC_PARENT_ANNO,
-            id: item._id });
-      }
-    });
-
-    // If the parent changed, update related non-enumerable properties.
-    let additionalParentInfo = {};
-    if (newParent) {
-      Object.defineProperty(additionalParentInfo, "_parentId",
-                            { value: newParent._id, enumerable: false });
-      Object.defineProperty(additionalParentInfo, "_grandParentId",
-                            { value: newParent._parentId, enumerable: false });
-    }
-
-    let updatedItem = mergeIntoNewObject(item, info, additionalParentInfo);
-
-    return updatedItem;
-  });
-}
-
 /**
- * Moves a single bookmark in the database.
+ * Updates a single bookmark in the database. This should be called from within
+ * a transaction.
  *
  * @param {Object} db The pre-existing database connection.
+ * @param {Object} info A bookmark-item structure with new properties.
  * @param {Object} item A bookmark-item structure representing the existing bookmark.
+ * @param {Integer} oldIndex The index of the item in the old parent.
  * @param {Object} newParent The new parent folder (note: this may be the same as)
  *                           the existing folder.
- * @param {Object} index The new index to move the item to.
- * @param {Date} lastModified The date to set for the modification times.
  * @param {Integer} syncChangeDelta The change delta to be applied.
- * @param {Boolean} defaultIndexRequested Set to true to indicate the default index
- *                                        was requested.
  */
-async function moveBookmark(db, item, oldIndex, newParent, newIndex, lastModified,
-                            syncChangeDelta, defaultIndexRequested) {
+async function updateBookmark(db, info, item, oldIndex, newParent, syncChangeDelta) {
   let tuples = new Map();
-  tuples.set("lastModified", { value: PlacesUtils.toPRTime(lastModified) });
-  item.lastModified = lastModified;
+  tuples.set("lastModified", { value: PlacesUtils.toPRTime(info.lastModified) });
+  if (info.hasOwnProperty("title")) {
+    tuples.set("title", { value: info.title,
+                          fragment: `title = NULLIF(:title, "")` });
+  }
+  if (info.hasOwnProperty("dateAdded")) {
+    tuples.set("dateAdded", { value: PlacesUtils.toPRTime(info.dateAdded) });
+  }
 
-  // For simplicity, update the index regardless.
-  tuples.set("position", { value: newIndex });
+  if (info.hasOwnProperty("url")) {
+    // Ensure a page exists in moz_places for this URL.
+    await maybeInsertPlace(db, info.url);
+    // Update tuples for the update query.
+    tuples.set("url", { value: info.url.href,
+                        fragment: "fk = (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url)" });
+  }
+
+  let newIndex = info.hasOwnProperty("index") ? info.index : item.index;
+  if (newParent) {
+    // For simplicity, update the index regardless.
+    tuples.set("position", { value: newIndex });
 
-  // For moving within the same parent, we've already updated the indexes.
-  if (newParent.guid == item.parentGuid) {
-      // Moving inside the original container.
-      // When moving "up", add 1 to each index in the interval.
-      // Otherwise when moving down, we subtract 1.
-      // Only the parent needs a sync change, which is handled in
-      // `setAncestorsLastModified`.
+    // For moving within the same parent, we've already updated the indexes.
+    if (newParent.guid == item.parentGuid) {
+        // Moving inside the original container.
+        // When moving "up", add 1 to each index in the interval.
+        // Otherwise when moving down, we subtract 1.
+        // Only the parent needs a sync change, which is handled in
+        // `setAncestorsLastModified`.
+        await db.executeCached(
+          `UPDATE moz_bookmarks
+           SET position = CASE WHEN :newIndex < :currIndex
+             THEN position + 1
+             ELSE position - 1
+           END
+           WHERE parent = :newParentId
+             AND position BETWEEN :lowIndex AND :highIndex
+          `, { newIndex, currIndex: oldIndex, newParentId: newParent._id,
+               lowIndex: Math.min(oldIndex, newIndex),
+               highIndex: Math.max(oldIndex, newIndex) });
+    } else {
+      // Moving across different containers. In this case, both parents and
+      // the child need sync changes. `setAncestorsLastModified`, below and in
+      // `update` and `moveToFolder`, handles the parents. The `needsSyncChange`
+      // check below handles the child.
+      tuples.set("parent", { value: newParent._id} );
       await db.executeCached(
-        `UPDATE moz_bookmarks
-         SET position = CASE WHEN :newIndex < :currIndex
-           THEN position + 1
-           ELSE position - 1
-         END
+        `UPDATE moz_bookmarks SET position = position - 1
+         WHERE parent = :oldParentId
+           AND position >= :oldIndex
+        `, { oldParentId: item._parentId, oldIndex });
+      await db.executeCached(
+        `UPDATE moz_bookmarks SET position = position + 1
          WHERE parent = :newParentId
-           AND position BETWEEN :lowIndex AND :highIndex
-        `, { newIndex, currIndex: oldIndex, newParentId: newParent._id,
-             lowIndex: Math.min(oldIndex, newIndex),
-             highIndex: Math.max(oldIndex, newIndex) });
-  } else {
-    // Moving across different containers. In this case, both parents and
-    // the child need sync changes. `setAncestorsLastModified` handles the
-    // parents; the `needsSyncChange` check below handles the child.
-    tuples.set("parent", { value: newParent._id} );
-    await db.executeCached(
-      `UPDATE moz_bookmarks SET position = position - 1
-       WHERE parent = :oldParentId
-         AND position >= :oldIndex
-      `, { oldParentId: item._parentId, oldIndex });
-    await db.executeCached(
-      `UPDATE moz_bookmarks SET position = position + 1
-       WHERE parent = :newParentId
-         AND position >= :newIndex
-      `, { newParentId: newParent._id, newIndex });
+           AND position >= :newIndex
+        `, { newParentId: newParent._id, newIndex });
+
+      await setAncestorsLastModified(db, item.parentGuid, info.lastModified,
+                                     syncChangeDelta);
+    }
+  }
+
+  if (syncChangeDelta) {
+    // Sync stores child indices in the parent's record, so we only bump the
+    // item's counter if we're updating at least one more property in
+    // addition to the index, last modified time, and dateAdded.
+    let sizeThreshold = 1;
+    if (newIndex != oldIndex) {
+      ++sizeThreshold;
+    }
+    if (tuples.has("dateAdded")) {
+      ++sizeThreshold;
+    }
+    let needsSyncChange = tuples.size > sizeThreshold;
+    if (needsSyncChange) {
+      tuples.set("syncChangeDelta", { value: syncChangeDelta,
+                                      fragment: "syncChangeCounter = syncChangeCounter + :syncChangeDelta" });
+    }
+  }
 
-    await setAncestorsLastModified(db, item.parentGuid, lastModified,
-                                   syncChangeDelta);
+  let isTagging = item._grandParentId == PlacesUtils.tagsFolderId;
+  if (isTagging) {
+    // If we're updating a tag entry, bump the sync change counter for
+    // bookmarks with the tagged URL.
+    await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
+      db, item.url, syncChangeDelta);
+    if (info.hasOwnProperty("url")) {
+      // Changing the URL of a tag entry is equivalent to untagging the
+      // old URL and tagging the new one, so we bump the change counter
+      // for the new URL here.
+      await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
+        db, info.url, syncChangeDelta);
+    }
+  }
+
+  let isChangingTagFolder = item._parentId == PlacesUtils.tagsFolderId;
+  if (isChangingTagFolder && syncChangeDelta) {
+    // If we're updating a tag folder (for example, changing a tag's title),
+    // bump the change counter for all tagged bookmarks.
+    await db.executeCached(`
+      UPDATE moz_bookmarks SET
+        syncChangeCounter = syncChangeCounter + :syncChangeDelta
+      WHERE type = :type AND
+            fk = (SELECT fk FROM moz_bookmarks WHERE parent = :parent)
+      `,
+      { syncChangeDelta, type: Bookmarks.TYPE_BOOKMARK, parent: item._id });
   }
 
   await db.executeCached(
     `UPDATE moz_bookmarks
      SET ${Array.from(tuples.keys()).map(v => tuples.get(v).fragment || `${v} = :${v}`).join(", ")}
      WHERE guid = :guid
     `, Object.assign({ guid: item.guid },
                      [...tuples.entries()].reduce((p, c) => { p[c[0]] = c[1].value; return p; }, {})));
 
-  if (newParent.guid == item.parentGuid) {
-    // Mark all affected separators as changed
-    // Also bumps the change counter if the item itself is a separator
-    const startIndex = Math.min(newIndex, oldIndex);
-    await adjustSeparatorsSyncCounter(db, newParent._id, startIndex, syncChangeDelta);
-  } else {
-    // Mark all affected separators as changed
-    await adjustSeparatorsSyncCounter(db, item._parentId, oldIndex, syncChangeDelta);
-    await adjustSeparatorsSyncCounter(db, newParent._id, newIndex, syncChangeDelta);
+  if (newParent) {
+    if (newParent.guid == item.parentGuid) {
+      // Mark all affected separators as changed
+      // Also bumps the change counter if the item itself is a separator
+      const startIndex = Math.min(newIndex, oldIndex);
+      await adjustSeparatorsSyncCounter(db, newParent._id, startIndex, syncChangeDelta);
+    } else {
+      // Mark all affected separators as changed
+      await adjustSeparatorsSyncCounter(db, item._parentId, oldIndex, syncChangeDelta);
+      await adjustSeparatorsSyncCounter(db, newParent._id, newIndex, syncChangeDelta);
+    }
+    // Remove the Sync orphan annotation from reparented items. We don't
+    // notify annotation observers about this because this is a temporary,
+    // internal anno that's only used by Sync.
+    await db.executeCached(
+      `DELETE FROM moz_items_annos
+       WHERE anno_attribute_id = (SELECT id FROM moz_anno_attributes
+                                  WHERE name = :orphanAnno) AND
+             item_id = :id`,
+      { orphanAnno: PlacesSyncUtils.bookmarks.SYNC_PARENT_ANNO,
+        id: item._id });
   }
-  // Remove the Sync orphan annotation from reparented items. We don't
-  // notify annotation observers about this because this is a temporary,
-  // internal anno that's only used by Sync.
-  await db.executeCached(
-    `DELETE FROM moz_items_annos
-     WHERE anno_attribute_id = (SELECT id FROM moz_anno_attributes
-                                WHERE name = :orphanAnno) AND
-           item_id = :id`,
-    { orphanAnno: PlacesSyncUtils.bookmarks.SYNC_PARENT_ANNO,
-      id: item._id });
 
   // If the parent changed, update related non-enumerable properties.
-  let additionalParentInfo = {
-    index: newIndex,
-    parentGuid: newParent.guid
-  };
+  let additionalParentInfo = {};
+  if (newParent) {
+    additionalParentInfo.parentGuid = newParent.guid;
+    Object.defineProperty(additionalParentInfo, "_parentId",
+                          { value: newParent._id, enumerable: false });
+    Object.defineProperty(additionalParentInfo, "_grandParentId",
+                          { value: newParent._parentId, enumerable: false });
+  }
 
-  Object.defineProperty(additionalParentInfo, "_parentId",
-                        { value: newParent._id, enumerable: false });
-  Object.defineProperty(additionalParentInfo, "_grandParentId",
-                        { value: newParent._parentId, enumerable: false });
-
-  return mergeIntoNewObject(item, additionalParentInfo);
+  return mergeIntoNewObject(item, info, additionalParentInfo);
 }
 
 // Insert implementation.
 
 function insertBookmark(item, parent) {
   return PlacesUtils.withConnectionWrapper("Bookmarks.jsm: insertBookmark",
     async function(db) {
 
@@ -2819,31 +2741,16 @@ var addSyncChangesForRemovedTagFolders =
     let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
     if (isUntagging) {
       await PlacesSyncUtils.bookmarks.addSyncChangesForBookmarksWithURL(
         db, item.url, syncChangeDelta);
     }
   }
 };
 
-// Bumps the change counter for all bookmarked URLs within `folders`.
-// This is used to update tagged bookmarks when changing a tag folder.
-function addSyncChangesForBookmarksInFolder(db, folder, syncChangeDelta) {
-  if (!syncChangeDelta) {
-    return Promise.resolve();
-  }
-  return db.execute(`
-    UPDATE moz_bookmarks SET
-      syncChangeCounter = syncChangeCounter + :syncChangeDelta
-    WHERE type = :type AND
-          fk = (SELECT fk FROM moz_bookmarks WHERE parent = :parent)
-    `,
-    { syncChangeDelta, type: Bookmarks.TYPE_BOOKMARK, parent: folder._id });
-}
-
 function adjustSeparatorsSyncCounter(db, parentId, startIndex, syncChangeDelta) {
   if (!syncChangeDelta) {
     return Promise.resolve();
   }
 
   return db.executeCached(`
     UPDATE moz_bookmarks
     SET syncChangeCounter = syncChangeCounter + :delta