зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine. r=markh
MozReview-Commit-ID: DAsTlQbFanK --HG-- extra : rebase_source : f04946571993194c115fb62a08bd50a4a6b8ff83
This commit is contained in:
Kit Cambridge
Родитель
7c6c7db247
Коммит
43ffc0fef1
|
|
@ -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 = {
|
||||
|
|
@ -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";
|
||||
|
|
@ -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;
|
||||
}
|
||||
},
|
||||
|
|
@ -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);
|
||||
|
|
@ -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]);
|
||||
|
|
@ -938,7 +954,6 @@ BookmarksTracker.prototype = {
|
|||
break;
|
||||
case "bookmarks-restore-failed":
|
||||
this._log.debug("Tracking all items on failed import.");
|
||||
this.ignoreAll = false;
|
||||
break;
|
||||
}
|
||||
},
|
||||
|
|
@ -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) {
|
||||
|
|
@ -1018,9 +1037,10 @@ 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);
|
||||
|
|
@ -1028,8 +1048,8 @@ BookmarksTracker.prototype = {
|
|||
},
|
||||
|
||||
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;
|
||||
}
|
||||
|
||||
|
|
@ -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");
|
||||
|
|
@ -1073,8 +1090,6 @@ 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
|
||||
|
|
@ -1082,11 +1097,7 @@ BookmarksTracker.prototype = {
|
|||
// *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;
|
||||
|
|
@ -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)" : "")) +
|
||||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче