Bug 1474033 - Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8 a=lizzard
authorLina Cambridge <lina@yakshaving.ninja>
Mon, 09 Jul 2018 14:12:41 +0000
changeset 478012 6aedd84a5388b511ba15f64e6efeaaec09079c58
parent 478011 fff218cba849af85e392ca103061b453c581fc87
child 478013 1f9ce3126115f82fa8440a311e175e0771183425
push id9499
push userarchaeopteryx@coole-files.de
push dateThu, 19 Jul 2018 06:52:33 +0000
treeherdermozilla-beta@ce18b96ec82b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersStandard8, lizzard
bugs1474033
milestone62.0
Bug 1474033 - Ensure `PlacesUtils.bookmarks.moveToFolder` bumps the Sync change counter for items moved between different parents. r=Standard8 a=lizzard
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
@@ -908,16 +908,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
@@ -1699,16 +1699,27 @@ async function moveBookmark(db, item, ol
        WHERE parent = :newParentId
          AND position >= :newIndex
       `, { newParentId: newParent._id, newIndex });
 
     await setAncestorsLastModified(db, item.parentGuid, 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 and last modified time.
+    let needsSyncChange = tuples.size > 2;
+    if (needsSyncChange) {
+      tuples.set("syncChangeDelta", { value: syncChangeDelta,
+                                      fragment: "syncChangeCounter = syncChangeCounter + :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: item.guid },
                      [...tuples.entries()].reduce((p, c) => { p[c[0]] = c[1].value; return p; }, {})));
 
   if (newParent.guid == item.parentGuid) {