Dedupe UNKNOWN bookmarks in the mirror

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: lina, Assigned: tcsc)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

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
Reporter

Comment 2

Last year
mozreview-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/#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 hidden (mozreview-request)
Reporter

Comment 4

Last year
mozreview-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/#review231846

Awesome, thanks!
Attachment #8956943 - Flags: review?(kit) → review+
Comment hidden (mozreview-request)
Reporter

Comment 6

Last year
mozreview-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 hidden (mozreview-request)
Assignee

Comment 8

Last year
mozreview-review-reply
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

Comment 9

Last year
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

Comment 10

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/93b55e817b82
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.