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