Closed Bug 1443245 Opened 3 years ago Closed 2 years ago

Dedupe UNKNOWN bookmarks in the mirror

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: lina, Assigned: tcsc)

Details

Attachments

(1 file)

Currently, we only consider NEW bookmarks for deduping.

Deduping UNKNOWN bookmarks should be safe, too, and avoids uploading dupes if we restore default bookmarks at startup (https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/migration/MigrationUtils.jsm#376-377, https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/nsBrowserGlue.js#1645-1650,1655), from an old backup with different GUIDs, or from an ancient (pre-bug 967204) backup without GUIDs. Items that already exist on the server will match by GUID.
Priority: -- → P2
Assignee: nobody → tchiovoloni
Comment on attachment 8956943 [details]
Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging.

https://reviewboard.mozilla.org/r/225886/#review231830

Looks good, thanks and great catch! I'd like to have one more look with the change to the trigger.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1058
(Diff revision 1)
>                                  count: String(totalRows) });
>  
>      return localTree;
>    }
>  
>    /**

I think we'll want to also update the `syncStatus` in https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/toolkit/components/places/SyncedBookmarksMirror.jsm#2129-2135, so, even if we keep the local value state, we'll still mark it as `NORMAL` if we deduped, instead of waiting until the engine calls `pushChanges`. (This is wrong for `NEW` today, too. Also, the comment above that statement is wrong, s/needsMerge/hasRemoteValue).

::: toolkit/components/places/tests/sync/test_bookmark_deduping.js:79
(Diff revision 1)
>          value: "MostVisited",
>        }],
>      }],
>    });
> +
> +  // Make sure we still dedupe this even though it doesn't have SYNC_STATUS.NEW

Keeping with the above, would you mind checking, via `PlacesSyncUtils.bookmarks.fetchBookmarkSyncFields`, that we changed the status of `NEW` and `UNKNOWN` items to `NORMAL` after deduping?
Attachment #8956943 - Flags: review?(kit)
Comment on attachment 8956943 [details]
Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging.

https://reviewboard.mozilla.org/r/225886/#review231846

Awesome, thanks!
Attachment #8956943 - Flags: review?(kit) → review+
Comment on attachment 8956943 [details]
Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging.

https://reviewboard.mozilla.org/r/225886/#review231854

::: commit-message-c7543:1
(Diff revision 3)
> +Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NEW after merging. r?kitcambridge

Nit: "reset to NORMAL"
Comment on attachment 8956943 [details]
Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging.

https://reviewboard.mozilla.org/r/225886/#review231854

> Nit: "reset to NORMAL"

Whoop
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/93b55e817b82
Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/93b55e817b82
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.