Closed Bug 1443245 Opened 6 years ago Closed 6 years ago

Dedupe UNKNOWN bookmarks in the mirror

Categories

(Firefox :: Sync, enhancement, P2)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: lina, Assigned: tcsc)

Details

Attachments

(1 file)

Bug 1443245 - Ensure UNKNOWN and NEW bookmarks are reset to NORMAL after merging.
59 bytes, text/x-review-board-request
lina
: review+
Details 
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: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: