Bug 1351915 - Fire onItemChanged when deduping sync bookmarks. r?kitcambridge
MozReview-Commit-ID: JHaCz42aRwx
--- a/services/sync/tests/unit/test_bookmark_duping.js
+++ b/services/sync/tests/unit/test_bookmark_duping.js
@@ -135,17 +135,17 @@ async function validate(collection, expe
add_task(async function test_dupe_bookmark() {
_("Ensure that a bookmark we consider a dupe is handled correctly.");
let { server, collection } = await this.setup();
try {
// The parent folder and one bookmark in it.
let {id: folder1_id, guid: folder1_guid } = await createFolder(bms.toolbarFolder, "Folder 1");
- let {guid: bmk1_guid} = await createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!");
+ let {id: localId, guid: bmk1_guid} = await createBookmark(folder1_id, "http://getfirefox.com/", "Get Firefox!");
engine.sync();
// We've added the bookmark, its parent (folder1) plus "menu", "toolbar", "unfiled", and "mobile".
equal(collection.count(), 6);
equal((await getFolderChildrenIDs(folder1_id)).length, 1);
// Now create a new incoming record that looks alot like a dupe.
@@ -153,36 +153,56 @@ add_task(async function test_dupe_bookma
let to_apply = {
id: newGUID,
bmkUri: "http://getfirefox.com/",
type: "bookmark",
title: "Get Firefox!",
parentName: "Folder 1",
parentid: folder1_guid,
};
+ collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 500);
- collection.insert(newGUID, encryptPayload(to_apply), Date.now() / 1000 + 500);
+ let onItemChangedObserved = false;
+ const obs = {
+ onItemChanged(id, prop, isAnno, newVal, lastMod, itemType, parentId, guid, parentGuid, oldVal, source) {
+ equal(id, localId);
+ equal(prop, "guid");
+ equal(newVal, newGUID);
+ equal(itemType, bms.TYPE_BOOKMARK);
+ equal(parentId, folder1_id);
+ equal(guid, newGUID);
+ equal(parentGuid, folder1_guid);
+ equal(oldVal, bmk1_guid);
+ equal(source, PlacesUtils.bookmarks.SOURCE_SYNC);
+ onItemChangedObserved = true;
+ }
+ };
+ PlacesUtils.bookmarks.addObserver(obs, false);
+
_("Syncing so new dupe record is processed");
engine.lastSync = engine.lastSync - 5;
engine.sync();
// We should have logically deleted the dupe record.
equal(collection.count(), 7);
ok(getServerRecord(collection, bmk1_guid).deleted);
// and physically removed from the local store.
await promiseNoLocalItem(bmk1_guid);
// Parent should still only have 1 item.
equal((await getFolderChildrenIDs(folder1_id)).length, 1);
// The parent record on the server should now reference the new GUID and not the old.
let serverRecord = getServerRecord(collection, folder1_guid);
ok(!serverRecord.children.includes(bmk1_guid));
ok(serverRecord.children.includes(newGUID));
+ ok(onItemChangedObserved);
+
// and a final sanity check - use the validator
await validate(collection);
+ PlacesUtils.bookmarks.removeObserver(obs);
} finally {
await cleanup(server);
}
});
add_task(async function test_dupe_reparented_bookmark() {
_("Ensure that a bookmark we consider a dupe from a different parent is handled correctly");
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -1767,26 +1767,28 @@ var touchSyncBookmark = Task.async(funct
// before uploading, instead of returning records to merge into the engine's
// initial changeset.
return pullSyncChanges(db);
});
var dedupeSyncBookmark = Task.async(function* (db, localGuid, remoteGuid,
remoteParentGuid) {
let rows = yield db.executeCached(`
- SELECT b.id, p.guid AS parentGuid, b.syncStatus
+ SELECT b.id, b.type, p.id AS parentId, p.guid AS parentGuid, b.syncStatus
FROM moz_bookmarks b
JOIN moz_bookmarks p ON p.id = b.parent
WHERE b.guid = :localGuid`,
{ localGuid });
if (!rows.length) {
throw new Error(`Local item ${localGuid} does not exist`);
}
let localId = rows[0].getResultByName("id");
+ let localParentId = rows[0].getResultByName("parentId");
+ let bookmarkType = rows[0].getResultByName("type");
if (PlacesUtils.isRootItem(localId)) {
throw new Error(`Cannot de-dupe local root ${localGuid}`);
}
let localParentGuid = rows[0].getResultByName("parentGuid");
let sameParent = localParentGuid == remoteParentGuid;
let modified = PlacesUtils.toPRTime(Date.now());
@@ -1834,16 +1836,26 @@ var dedupeSyncBookmark = Task.async(func
if (localSyncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL) {
yield db.executeCached(`
INSERT INTO moz_bookmarks_deleted (guid, dateRemoved)
VALUES (:localGuid, :modified)`,
{ localGuid, modified });
}
});
+ let observers = PlacesUtils.bookmarks.getObservers();
+ notify(observers, "onItemChanged", [ localId, "guid", false,
+ remoteGuid,
+ modified,
+ bookmarkType,
+ localParentId,
+ remoteGuid, remoteParentGuid,
+ localGuid, SOURCE_SYNC
+ ]);
+
// TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
// before uploading, instead of returning records to merge into the engine's
// initial changeset.
let changeRecords = yield pullSyncChanges(db);
if (BookmarkSyncLog.level <= Log.Level.Debug && !sameParent) {
let remoteParentSyncId = BookmarkSyncUtils.guidToSyncId(remoteParentGuid);
if (!changeRecords.hasOwnProperty(remoteParentSyncId)) {
@@ -1973,8 +1985,26 @@ function markChangesAsSyncing(db, change
var removeTombstones = Task.async(function* (db, guids) {
if (!guids.length) {
return Promise.resolve();
}
return db.execute(`
DELETE FROM moz_bookmarks_deleted
WHERE guid IN (${guids.map(guid => JSON.stringify(guid)).join(",")})`);
});
+
+/**
+ * Sends a bookmarks notification through the given observers.
+ *
+ * @param observers
+ * array of nsINavBookmarkObserver objects.
+ * @param notification
+ * the notification name.
+ * @param args
+ * array of arguments to pass to the notification.
+ */
+function notify(observers, notification, args = []) {
+ for (let observer of observers) {
+ try {
+ observer[notification](...args);
+ } catch (ex) {}
+ }
+}
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -146,26 +146,28 @@ interface nsINavBookmarkObserver : nsISu
* property (see the list below).
* @param aSource
* A change source constant from nsINavBookmarksService::SOURCE_*,
* passed to the method that notifies the observer.
*
* @note List of values that may be associated with properties:
* aProperty | aNewValue
* =====================================================================
+ * guid | The new bookmark guid.
* cleartime | Empty string (all visits to this item were removed).
* title | The new title.
* favicon | The "moz-anno" URL of the new favicon.
* uri | new URL.
* tags | Empty string (tags for this item changed)
* dateAdded | PRTime (as string) when the item was first added.
* lastModified | PRTime (as string) when the item was last modified.
*
* aProperty | aOldValue
* =====================================================================
+ * guid | The old bookmark guid.
* cleartime | Empty string (currently unused).
* title | Empty string (currently unused).
* favicon | Empty string (currently unused).
* uri | old URL.
* tags | Empty string (currently unused).
* dateAdded | Empty string (currently unused).
* lastModified | Empty string (currently unused).
*/