Bug 1310941 - Fix `BookmarksTracker.onItemChanged` arguments to avoid triggering syncs for remote changes. r=tcsc
MozReview-Commit-ID: K1OcL5m6opv
--- 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");
}