Skip to content

Commit

Permalink
Bug 1299338 - Replace ignoreAll with change source checks in the Sy…
Browse files Browse the repository at this point in the history 
…nc bookmarks engine. r=markh

MozReview-Commit-ID: DAsTlQbFanK
  • Loading branch information
Kit Cambridge committed Sep 6, 2016
1 parent 429235b commit 14accfe
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 28 deletions.
66 changes: 40 additions & 26 deletions services/sync/modules/engines/bookmarks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ const ANNOS_TO_TRACK = [BookmarkAnnos.DESCRIPTION_ANNO, BookmarkAnnos.SIDEBAR_AN

const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
const FOLDER_SORTINDEX = 1000000;
const { SOURCE_SYNC } = Ci.nsINavBookmarksService;
const {
SOURCE_SYNC,
SOURCE_IMPORT,
SOURCE_IMPORT_REPLACE,
} = Ci.nsINavBookmarksService;

// Maps Sync record property names to `PlacesSyncUtils` bookmark properties.
const RECORD_PROPS_TO_BOOKMARK_PROPS = {
Expand All @@ -42,6 +46,12 @@ const RECORD_PROPS_TO_BOOKMARK_PROPS = {
feedUri: "feed",
};

// The tracker ignores changes made by bookmark import and restore, and
// changes made by Sync. We don't need to exclude `SOURCE_IMPORT`, but both
// import and restore fire `bookmarks-restore-*` observer notifications, and
// the tracker doesn't currently distinguish between the two.
const IGNORED_SOURCES = [SOURCE_SYNC, SOURCE_IMPORT, SOURCE_IMPORT_REPLACE];

this.PlacesItem = function PlacesItem(collection, id, type) {
CryptoWrapper.call(this, collection, id);
this.type = type || "item";
Expand Down Expand Up @@ -377,9 +387,7 @@ BookmarksEngine.prototype = {
SyncEngine.prototype._processIncoming.call(this, newitems);
} finally {
// Reorder children.
this._tracker.ignoreAll = true;
this._store._orderChildren();
this._tracker.ignoreAll = false;
delete this._store._childrenToOrder;
}
},
Expand Down Expand Up @@ -905,6 +913,16 @@ function BookmarksTracker(name, engine) {
BookmarksTracker.prototype = {
__proto__: Tracker.prototype,

//`_ignore` checks the change source for each observer notification, so we
// don't want to let the engine ignore all changes during a sync.
get ignoreAll() {
return false;
},

// Define an empty setter so that the engine doesn't throw a `TypeError`
// setting a read-only property.
set ignoreAll(value) {},

startTracking: function() {
PlacesUtils.bookmarks.addObserver(this, true);
Svc.Obs.add("bookmarks-restore-begin", this);
Expand All @@ -925,11 +943,9 @@ BookmarksTracker.prototype = {
switch (topic) {
case "bookmarks-restore-begin":
this._log.debug("Ignoring changes from importing bookmarks.");
this.ignoreAll = true;
break;
case "bookmarks-restore-success":
this._log.debug("Tracking all items on successful import.");
this.ignoreAll = false;

this._log.debug("Restore succeeded: wiping server and other clients.");
this.engine.service.resetClient([this.name]);
Expand All @@ -938,7 +954,6 @@ BookmarksTracker.prototype = {
break;
case "bookmarks-restore-failed":
this._log.debug("Tracking all items on failed import.");
this.ignoreAll = false;
break;
}
},
Expand Down Expand Up @@ -978,11 +993,15 @@ BookmarksTracker.prototype = {
* Item under consideration to ignore
* @param folder (optional)
* Folder of the item being changed
* @param guid
* Places GUID of the item being changed
* @param source
* A change source constant from `nsINavBookmarksService::SOURCE_*`.
*/
_ignore: function BMT__ignore(itemId, folder, guid) {
// Ignore unconditionally if the engine tells us to.
if (this.ignoreAll)
_ignore: function BMT__ignore(itemId, folder, guid, source) {
if (IGNORED_SOURCES.includes(source)) {
return true;
}

// Get the folder id if we weren't given one.
if (folder == null) {
Expand Down Expand Up @@ -1018,18 +1037,19 @@ BookmarksTracker.prototype = {

onItemAdded: function BMT_onItemAdded(itemId, folder, index,
itemType, uri, title, dateAdded,
guid, parentGuid) {
if (this._ignore(itemId, folder, guid))
guid, parentGuid, source) {
if (this._ignore(itemId, folder, guid, source)) {
return;
}

this._log.trace("onItemAdded: " + itemId);
this._add(itemId, guid);
this._add(folder, parentGuid);
},

onItemRemoved: function (itemId, parentId, index, type, uri,
guid, parentGuid) {
if (this._ignore(itemId, parentId, guid)) {
guid, parentGuid, source) {
if (this._ignore(itemId, parentId, guid, source)) {
return;
}

Expand All @@ -1049,9 +1069,6 @@ BookmarksTracker.prototype = {
if (all.length == 0)
return;

// Disable handling of notifications while changing the mobile query
this.ignoreAll = true;

let mobile = find(BookmarkAnnos.MOBILE_ANNO);
let queryURI = Utils.makeURI("place:folder=" + BookmarkSpecialIds.mobile);
let title = Str.sync.get("mobile.label");
Expand All @@ -1073,20 +1090,14 @@ BookmarksTracker.prototype = {
else if (PlacesUtils.bookmarks.getItemTitle(mobile[0]) != title) {
PlacesUtils.bookmarks.setItemTitle(mobile[0], title, SOURCE_SYNC);
}

this.ignoreAll = false;
},

// 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) {
// Quicker checks first.
if (this.ignoreAll)
return;

guid, parentGuid, source) {
if (isAnno && (ANNOS_TO_TRACK.indexOf(property) == -1))
// Ignore annotations except for the ones that we sync.
return;
Expand All @@ -1095,8 +1106,9 @@ BookmarksTracker.prototype = {
if (property == "favicon")
return;

if (this._ignore(itemId, parentId, guid))
if (this._ignore(itemId, parentId, guid, source)) {
return;
}

this._log.trace("onItemChanged: " + itemId +
(", " + property + (isAnno? " (anno)" : "")) +
Expand All @@ -1106,9 +1118,11 @@ BookmarksTracker.prototype = {

onItemMoved: function BMT_onItemMoved(itemId, oldParent, oldIndex,
newParent, newIndex, itemType,
guid, oldParentGuid, newParentGuid) {
if (this._ignore(itemId, newParent, guid))
guid, oldParentGuid, newParentGuid,
source) {
if (this._ignore(itemId, newParent, guid, source)) {
return;
}

this._log.trace("onItemMoved: " + itemId);
this._add(oldParent, oldParentGuid);
Expand Down
97 changes: 97 additions & 0 deletions services/sync/tests/unit/test_bookmark_engine.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,103 @@ 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 server = serverForFoo(engine);
new SyncTestingInfrastructure(server.server);

let collection = server.user("foo").collection("bookmarks");

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}`);

let bmk1_id = PlacesUtils.bookmarks.insertBookmark(
folder1_id, Utils.makeURI("http://getthunderbird.com/"),
PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!");
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;
{
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";
localRecord.parentid = folder1_guid;

let remoteRecord = collection.insert(bmk2_guid, encryptPayload(localRecord.cleartext));
remoteRecord.get = function get() {
_("Inserting bookmark into local store");
bmk3_id = PlacesUtils.bookmarks.insertBookmark(
folder1_id, Utils.makeURI("https://mozilla.org/"),
PlacesUtils.bookmarks.DEFAULT_INDEX, "Mozilla");

return ServerWBO.prototype.get.apply(this, arguments);
};
}

{
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 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,
"Bookmark created during first sync should exist locally");
ok(!collection.wbo(bmk3_guid),
"Bookmark created during first sync shouldn't be uploaded yet");

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);

{
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");
}
} finally {
store.wipe();
Svc.Prefs.resetBranch("");
Service.recordManager.clearCache();
yield new Promise(resolve => server.stop(resolve));
Svc.Obs.notify("weave:engine:stop-tracking");
}
});

add_task(function* bad_record_allIDs() {
let server = new SyncServer();
server.start();
Expand Down
4 changes: 2 additions & 2 deletions toolkit/components/places/nsNavHistoryResult.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3965,12 +3965,12 @@ nsNavHistoryFolderResultNode::OnItemMoved(int64_t aItemId,
}
if (aOldParent == mTargetFolderItemId) {
OnItemRemoved(aItemId, aOldParent, aOldIndex, aItemType, itemURI,
aGUID, aOldParentGUID, nsINavBookmarksService::SOURCE_DEFAULT);
aGUID, aOldParentGUID, aSource);
}
if (aNewParent == mTargetFolderItemId) {
OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, itemURI, itemTitle,
RoundedPRNow(), // This is a dummy dateAdded, not the real value.
aGUID, aNewParentGUID, nsINavBookmarksService::SOURCE_DEFAULT);
aGUID, aNewParentGUID, aSource);
}
}
return NS_OK;
Expand Down

0 comments on commit 14accfe

Please sign in to comment.