Closed
Bug 1366888
Opened 8 years ago
Closed 7 years ago
Only dupe NEW bookmarks to NORMAL bookmarks
Categories
(Firefox :: Sync, enhancement, P2)
Firefox
Sync
Tracking
()
RESOLVED
DUPLICATE
of bug 1305563
People
(Reporter: lina, Assigned: lina)
References
Details
(Whiteboard: [Sync Q3 OKR])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Details |
We don't currently account for the sync status when deduping bookmarks, meaning we might dupe a NORMAL (already on the server) bookmark to another NORMAL bookmark. We correctly upload a tombstone for the old GUID, but I wonder if the correct behavior is to only dupe NEW to NORMAL.
This is how I imagine form autofill will work, except autofill spells NORMAL as "SYNCING".
A complication here is that `pullChanges` marks NEW bookmarks as NORMAL when fetching changes before the sync. I think we can split this function into `pullChanges` and `markNewBookmarksAsSyncing`, as in bug 1323333.
Semantically, does this seem right? I *think* this means we could avoid including `hasDupe` in the record, too, though we'd still need it in the local GUID map.
Updated•8 years ago
|
Priority: -- → P2
Comment 1•8 years ago
|
||
(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #0)
> Semantically, does this seem right?
I think so, yeah.
> I *think* this means we could avoid
> including `hasDupe` in the record, too
hrm - I guess so, yeah.
> though we'd still need it in the local GUID map.
Right, but that has slightly different semantics IIUC (and the fact they are both .hasDupe probably accounts for some confusion about these 2 different concepts with the same name)
Updated•7 years ago
|
Assignee: nobody → kit
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•7 years ago
|
||
Comment on attachment 8883749 [details]
Bug 1366888 - Replace the GUID map with `PlacesSyncUtils.bookmarks.findDupeGuid`.
This patch still dedupes NORMAL to NORMAL; I didn't change that part yet (`syncStatus = :syncStatus OR 1` are placeholders). But here's how deduping might look without the GUID map, and preserving existing semantics.
Attachment #8883749 -
Flags: feedback?(markh)
Comment 4•7 years ago
|
||
While killing the guidmap sounds awesome, TBH I'd be reluctant to land such a change in 56 given we hope to land Ed's risky patch too. I guess I could be convinced otherwise, but that seems like asking for hurt. Would it be possible to have a short-term fix that checks the sync status after looking up the existing map and doing this kind of change later?
Comment 5•7 years ago
|
||
(and I'd be fine to wait if that wouldn't work for reasons I can't yet see ;)
Assignee | ||
Comment 6•7 years ago
|
||
I thought about adding this to the existing map, and decided against it:
1) For local dupes (bookmarks with the same attributes and parent title, but in different parts of the tree), we currently store the first GUID we see as a string object, and set `.hasDupe = true`. I don't think that would work here. Once we dedupe a bookmark, we want to remove its GUID from the map, but keep local dupes around as candidates. We'd need to change the GUID map to store a list of matching GUIDs, and remove duped GUIDs from that list. I admit this refactoring would be smaller than rewriting how duping works, though, and likely less risky.
2) The logic is still tricky to test independently of the engine. An advantage of factoring this out into PSU is that we can add tests for `findDupeGuid`, and nail down the semantics before we start on bug 1305563 and bug 1323333. (Two-phase would be lovely to get in to 57, but probably unrealistic with everything else on our plate this quarter). If we wait, we probably won't be any worse off than we are now.
3) I already had the code for this attached to bug 1323333, and this is a conservative approach to landing those changes in pieces. That's not a good reason by itself, though. :-)
If I can add more test coverage for duping, would you be open to having your mind changed? Or would you still prefer to use the existing GUID map in 56, or wait until 57 to land this?
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: P1 → P2
Assignee | ||
Comment 8•7 years ago
|
||
Rolling this up into the two-phase work.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Whiteboard: [Sync Q3 OKR]
You need to log in
before you can comment on or make changes to this bug.
Description
•