Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc draft
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 18 Oct 2016 08:26:43 -0700
changeset 426590 610d395625a0c4841298331163bc55b89bf7fb31
parent 426145 c22f0df11dd8dc7671017fb517f5a9deb3c6fce8
child 534223 8c953a01edd1ff7d962f8541efa711ac460bf842
push id32754
push userbmo:kcambridge@mozilla.com
push dateTue, 18 Oct 2016 20:41:02 +0000
reviewerstcsc
bugs1310941
milestone52.0a1
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc MozReview-Commit-ID: K1OcL5m6opv
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -1105,17 +1105,18 @@ BookmarksTracker.prototype = {
     }
   },
 
   // This method is oddly structured, but the idea is to return as quickly as
   // possible -- this handler gets called *every time* a bookmark changes, for
   // *each change*.
   onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value,
                                             lastModified, itemType, parentId,
-                                            guid, parentGuid, source) {
+                                            guid, parentGuid, oldValue,
+                                            source) {
     if (IGNORED_SOURCES.includes(source)) {
       return;
     }
 
     if (isAnno && (ANNOS_TO_TRACK.indexOf(property) == -1))
       // Ignore annotations except for the ones that we sync.
       return;
 
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -1,36 +1,44 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/PlacesSyncUtils.jsm");
 Cu.import("resource://gre/modules/BookmarkJSONUtils.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/bookmarks.js");
 Cu.import("resource://services-sync/service.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://testing-common/services/sync/utils.js");
 Cu.import("resource://gre/modules/Promise.jsm");
 
 initTestLogging("Trace");
 
 Service.engineManager.register(BookmarksEngine);
 
 add_task(function* test_change_during_sync() {
   _("Ensure that we track changes made during a sync.");
 
-  let engine = new BookmarksEngine(Service);
-  let store  = engine._store;
+  let engine  = new BookmarksEngine(Service);
+  let store   = engine._store;
+  let tracker = engine._tracker;
   let server = serverForFoo(engine);
   new SyncTestingInfrastructure(server.server);
 
   let collection = server.user("foo").collection("bookmarks");
 
+  let bz_id = PlacesUtils.bookmarks.insertBookmark(
+    PlacesUtils.bookmarksMenuFolderId, Utils.makeURI("https://bugzilla.mozilla.org/"),
+    PlacesUtils.bookmarks.DEFAULT_INDEX, "Bugzilla");
+  let bz_guid = yield PlacesUtils.promiseItemGuid(bz_id);
+    _(`Bugzilla GUID: ${bz_guid}`);
+
   Svc.Obs.notify("weave:engine:start-tracking");
 
   try {
     let folder1_id = PlacesUtils.bookmarks.createFolder(
       PlacesUtils.bookmarks.toolbarFolder, "Folder 1", 0);
     let folder1_guid = store.GUIDForId(folder1_id);
     _(`Folder GUID: ${folder1_guid}`);
 
@@ -40,16 +48,27 @@ add_task(function* test_change_during_sy
     let bmk1_guid = store.GUIDForId(bmk1_id);
     _(`Thunderbird GUID: ${bmk1_guid}`);
 
     // Sync is synchronous, so, to simulate a bookmark change made during a
     // sync, we create a server record that adds a bookmark as a side effect.
     let bmk2_guid = "get-firefox1";
     let bmk3_id = -1;
     {
+      // An existing record changed on the server that should not trigger
+      // another sync when applied.
+      let changedRecord = new Bookmark("bookmarks", bz_guid);
+      changedRecord.bmkUri = "https://bugzilla.mozilla.org/";
+      changedRecord.description = "New description";
+      changedRecord.title = "Bugzilla";
+      changedRecord.tags = ["new", "tags"];
+      changedRecord.parentName = "Bookmarks Toolbar";
+      changedRecord.parentid = PlacesUtils.bookmarks.toolbarGuid;
+      collection.insert(bz_guid, encryptPayload(changedRecord.cleartext));
+
       let localRecord = new Bookmark("bookmarks", bmk2_guid);
       localRecord.bmkUri        = "http://getfirefox.com/";
       localRecord.description   = "Firefox is awesome.";
       localRecord.title         = "Get Firefox!";
       localRecord.tags          = ["firefox", "awesome", "browser"];
       localRecord.keyword       = "awesome";
       localRecord.loadInSidebar = false;
       localRecord.parentName    = "Folder 1";
@@ -68,17 +87,22 @@ add_task(function* test_change_during_sy
 
     {
       let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
       let childGuids = tree.children.map(child => child.guid);
       deepEqual(childGuids, [bmk1_guid], "Folder should have 1 child before first sync");
     }
 
     _("Perform first sync");
-    yield sync_engine_and_validate_telem(engine, false);
+    {
+      let changes = engine.pullNewChanges();
+      deepEqual(changes.ids().sort(), [folder1_guid, bmk1_guid, "toolbar"].sort(),
+        "Should track bookmark and folder created before first sync");
+      yield sync_engine_and_validate_telem(engine, false);
+    }
 
     let bmk2_id = store.idForGUID(bmk2_guid);
     let bmk3_guid = store.GUIDForId(bmk3_id);
     _(`Mozilla GUID: ${bmk3_guid}`);
     {
       equal(store.GUIDForId(bmk2_id), bmk2_guid,
         "Remote bookmark should be applied during first sync");
       ok(bmk3_id > -1,
@@ -88,19 +112,22 @@ add_task(function* test_change_during_sy
 
       let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
       let childGuids = tree.children.map(child => child.guid);
       deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid],
         "Folder should have 3 children after first sync");
     }
 
     _("Perform second sync");
-    yield sync_engine_and_validate_telem(engine, false);
+    {
+      let changes = engine.pullNewChanges();
+      deepEqual(changes.ids().sort(), [bmk3_guid, folder1_guid].sort(),
+        "Should track bookmark added during last sync and its parent");
+      yield sync_engine_and_validate_telem(engine, false);
 
-    {
       ok(collection.wbo(bmk3_guid),
         "Bookmark created during first sync should be uploaded during second sync");
 
       let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid);
       let childGuids = tree.children.map(child => child.guid);
       deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid],
         "Folder should have same children after second sync");
     }