Bug 1351915 - Fire onItemChanged when deduping sync bookmarks. r?kitcambridge draft
authorEdouard Oger <eoger@fastmail.com>
Sat, 08 Apr 2017 19:57:39 -0400
changeset 563830 b573d89ecfbb8250b1690ee4f99644366df541e2
parent 563802 a374c35469935a874fefe64d3e07003fc5bc8884
child 624593 28fe4da61bf999c9fac47cb8f050488003ae300d
push id54437
push userbmo:eoger@fastmail.com
push dateMon, 17 Apr 2017 22:16:26 +0000
reviewerskitcambridge
bugs1351915
milestone55.0a1
Bug 1351915 - Fire onItemChanged when deduping sync bookmarks. r?kitcambridge MozReview-Commit-ID: JHaCz42aRwx
services/sync/tests/unit/test_bookmark_duping.js
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/nsINavBookmarksService.idl
--- 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).
    */