Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc, a=gchang
authorKit Cambridge <kit@yakshaving.ninja>
Tue, 18 Oct 2016 08:26:43 -0700
changeset 356200 66d78c1bcadcb7bbfc8f5f5158ad32a750ecf21c
parent 356199 9995bef459e957c131af9366741fadede7d1481d
child 356201 c152fb1bc58813c7c88ae26be43c308b5c330544
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, gchang
bugs1310941
milestone51.0a2
Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc, a=gchang 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
@@ -1220,17 +1220,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,37 +1,45 @@
 /* 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/bookmark_utils.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}`);
 
@@ -41,16 +49,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";
@@ -69,17 +88,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,
@@ -89,19 +113,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");
     }