Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging. r?kitcambridge draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Wed, 07 Mar 2018 12:17:29 -0800
changeset 764646 9645ca08b3a5be59b19755ecaed6df367d110b42
parent 763903 c7543a3e938d5ec408a7d0c93dc71b40607e7d5b
push id101806
push userbmo:tchiovoloni@mozilla.com
push dateWed, 07 Mar 2018 23:05:43 +0000
reviewerskitcambridge
bugs1443245
milestone60.0a1
Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging. r?kitcambridge Also, correct comment and ensure we have test coverage for deduping UNKNOWN bookmarks. MozReview-Commit-ID: 7meySJhhsdO
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_deduping.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -1051,19 +1051,19 @@ class SyncedBookmarksMirror {
     this.recordTelemetryEvent("mirror", "fetch", "localTree",
                               { time: String(elapsedTime),
                                 count: String(totalRows) });
 
     return localTree;
   }
 
   /**
-   * Fetches content info for all NEW local items that don't exist in the
-   * mirror. We'll try to dedupe them to changed items with similar contents and
-   * different GUIDs in the mirror.
+   * Fetches content info for all NEW and UNKNOWN local items that don't exist
+   * in the mirror. We'll try to dedupe them to changed items with similar
+   * contents and different GUIDs in the mirror.
    *
    * @return {Map.<String, BookmarkContent>}
    *         New items in Places that don't exist in the mirror, keyed by their
    *         GUIDs.
    */
   async fetchNewLocalContents() {
     let newLocalContents = new Map();
     let startTime = Cu.now();
@@ -2079,19 +2079,20 @@ async function initializeTempMirrorEntit
   // remote items, and flags remote items as merged. In the trigger body, `OLD`
   // refers to the row for the unmerged item in `itemsToMerge`.
   await db.execute(`
     CREATE TEMP TRIGGER mergeGuids
     INSTEAD OF DELETE ON itemsToMerge
     BEGIN
       /* We update GUIDs here, instead of in the "updateExistingLocalItems"
          trigger, because deduped items where we're keeping the local value
-         state won't have "needsMerge" set. */
+         state won't have "hasRemoteValue" set. */
       UPDATE moz_bookmarks SET
-        guid = OLD.newGuid
+        guid = OLD.newGuid,
+        syncStatus = ${PlacesUtils.bookmarks.SYNC_STATUS.NORMAL}
       WHERE OLD.oldGuid <> OLD.newGuid AND
             id = OLD.localId;
 
       /* Record item changed notifications for the updated GUIDs. */
       INSERT INTO guidsChanged(itemId, oldGuid, level)
       SELECT OLD.localId, OLD.oldGuid, OLD.newLevel
       WHERE OLD.oldGuid <> OLD.newGuid;
 
--- a/toolkit/components/places/tests/sync/test_bookmark_deduping.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_deduping.js
@@ -70,16 +70,23 @@ add_task(async function test_duping() {
       url: "place:sort=8&maxResults=10",
       title: "Most Visited",
       annos: [{
         name: PlacesSyncUtils.bookmarks.SMART_BOOKMARKS_ANNO,
         value: "MostVisited",
       }],
     }],
   });
+
+  // Make sure we still dedupe this even though it doesn't have SYNC_STATUS.NEW
+  PlacesTestUtils.setBookmarkSyncFields({
+    guid: "folderBBBBBB",
+    syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN
+  });
+
   // Not a candidate for `bookmarkH111` because we didn't dupe `folderAAAAAA`.
   await PlacesUtils.bookmarks.insert({
     parentGuid: "folderAAAAAA",
     guid: "bookmarkHHHH",
     url: "http://example.com/h",
     title: "H",
   });
 
@@ -233,16 +240,22 @@ add_task(async function test_duping() {
     }, {
       guid: PlacesUtils.bookmarks.mobileGuid,
       type: PlacesUtils.bookmarks.TYPE_FOLDER,
       index: 4,
       title: MobileBookmarksTitle,
     }],
   }, "Should dedupe matching NEW bookmarks");
 
+  ok((await PlacesTestUtils.fetchBookmarkSyncFields(
+    "menu________", "folderB11111", "bookmarkC222", "separatorF11",
+    "folderA11111", "bookmarkG111", "separatorE11", "queryD111111"))
+    .every(info => info.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL));
+
+
   await buf.finalize();
   await PlacesUtils.bookmarks.eraseEverything();
   await PlacesSyncUtils.bookmarks.reset();
 });
 
 add_task(async function test_applying_two_empty_folders_doesnt_smush() {
   let buf = await openMirror("applying_two_empty_folders_doesnt_smush");