Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 18 Oct 2016 08:26:43 -0700
changeset 318550 5cd0ba0180c388c978d66ae7d6a0e8fe103460c8
parent 318549 2069ba1153a5d300cc72dc9a6bf80717b62a04cb
child 318551 dda934940e4dcf79c681cf14627bf6303223ca5c
push id82960
push usercbook@mozilla.com
push dateWed, 19 Oct 2016 15:04:04 +0000
treeherdermozilla-inbound@c48c53e3492e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc
bugs1310941
milestone52.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 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");
     }