Open
Bug 1177516
Opened 9 years ago
Updated 1 year ago
Ensure that default bookmarks aren't duplicated
Categories
(Firefox :: Sync, defect, P3)
Firefox
Sync
Tracking
()
NEW
People
(Reporter: rfkelly, Unassigned)
References
Details
(Whiteboard: [sync-quality])
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
Steps to reproduce:
* Have an existing desktop profile with carefully curated bookmarks, connected to sync
* Create a new desktop firefox profile and connect it to the same sync account
Result:
The default "Most Visited" and "Getting Started" bookmarks from the new profile get propagated back and appear in toolbar on the old profile. It's unexpected and annoying. I removed them from the original profile, it's pretty obvious that I don't want them back :-)
Can we somehow flag these default bookmarks and avoid syncing them back from second devices?
Updated•9 years ago
|
Priority: -- → P1
Comment 1•9 years ago
|
||
Another solution would be to bring back the "merge/replace all/replace self" option to FxA Sync, which has been requested by some users: Bug 966530.
This would require some UX treatment of what that would look like in FxA Sync.
Flags: needinfo?(rfeeley)
Updated•9 years ago
|
Rank: 15
Comment 2•9 years ago
|
||
Yes, I totally support this, but don't imagine there is any actual new screens to build or UX flows to work out. Deleting any bookmark intentionally, should remove it from the constellation.
Flags: needinfo?(rfeeley)
Comment 3•9 years ago
|
||
We can probably address this by giving the default bookmarks a magic GUID.
Updated•9 years ago
|
Whiteboard: [fxsync]
Updated•9 years ago
|
Flags: firefox-backlog+
Priority: P1 → P2
Comment 4•9 years ago
|
||
This is early in the user's 2nd device Sync experience, and it's bad.
Priority: P2 → P1
Comment 5•9 years ago
|
||
I don't think we can fix this in the general case as things stand. Places removes bookmarks on deletion (as opposed to Sync, which sets a "deleted" flag). So if a user deleted the default bookmarks on profile 1 before setting up Sync, we have no record of it being deleted. Fixing it just for users who delete those bookmarks after setting up Sync seems doable (we'd probably just need to assign a fixed guid for these bookmarks at build-time), but doesn't seem worthwhile to me.
This should probably depend on a bug for upgrading places to better support Sync, but I can't find a bug already open for that, and it's still so vague I don't see value in opening one before we have some ideas for what that means - it will probably be inspired by the iOS implementation.
Comment 6•9 years ago
|
||
(heh - had a thought a few seconds after hitting "send" :)
I guess a different approach might be to assign these default bookmarks a guid, and add these guids to a list of bookmarks Sync completely ignores. It sounds a little fragile unless we have a way to query this canonical set of guids at runtime, but that might be doable.
Comment 7•9 years ago
|
||
Smart queries like Most Visited can be easily recognized, and I was expecting Sync to already skip them.
we could also annotate default bookmarks, if needed.
Comment 8•9 years ago
|
||
PS: but note, only on new profiles, we cannot distinguish default bookmarks on old profiles.
Comment 9•9 years ago
|
||
> I guess a different approach might be to assign these default bookmarks a guid, and add these guids to a list of bookmarks Sync completely ignores. It sounds a little fragile unless we have a way to query this canonical set of guids at runtime, but that might be doable.
I like this approach. I'm also fine with it only being for new profiles.
Comment 10•9 years ago
|
||
The way we did this originally on iOS was to make those default bookmarks not be bookmarks: we add them at display time, but they're not in the database. (This also lets us re-localize them dynamically, replace them, etc.)
Creating a kind of 'ignore' list of GUIDs is error prone, because these are full bookmarks, so you can move them, rename them, copy them, and so on. You need to make sure you ignore those things _everywhere_: child lists, change tracking, etc.
For example: when a user creates a "Junk Bookmarks" folder in that new profile, and moves the defaults there, do you sync an empty folder around?
The final solution we picked for iOS was to simply not have default bookmarks at all. We have a nice-looking empty state instead.
The default bookmarks offer no value, users universally hated them and wanted to be able to delete them, etc. etc. We now ship the Alexa Top 5 as our default Suggested Sites, and if you want help and support, you'll look in Settings and tap the "Help" button.
More fun: we've changed these URLs on desktop over time, so long-term Sync users with multiple profiles might see four or five of these distinct bookmarks in their profiles.
Comment 11•9 years ago
|
||
Good points, Richard.
> The final solution we picked for iOS was to simply not have default bookmarks at all. We have a nice-looking empty state instead.
I was going to suggest this as well, because it's probably the simplest thing.
I'll try to advocate for that.
Comment 12•9 years ago
|
||
I'd also be fine (quite happy) with having no default bookmarks and moving relevant information to proper menus. That may simplify some of our startup code.
Also, 1 of the default bookmarks we have is on the toolbar and the toolbar is hidden by default... not very useful.
Btw, there's still some sort of bug in Sync if it syncs the smart bookmarks (most visited, recently bookmarked and so on), those are clearly marked with a "Places/SmartBookmark" annotation and should definitely be skipped (this doesn't means that sync should check the anno for each bookmark clearly, it should fetch all of them at once and build a map of bookmarks to be skipped).
Comment 13•9 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #12)
> Btw, there's still some sort of bug in Sync if it syncs the smart bookmarks
> (most visited, recently bookmarked and so on), those are clearly marked with
> a "Places/SmartBookmark" annotation and should definitely be skipped (this
> doesn't means that sync should check the anno for each bookmark clearly, it
> should fetch all of them at once and build a map of bookmarks to be skipped).
I think we are fine there - we do find the query itself, but do not attempt to execute it and sync the results. We handle the query so that we handle when the user moves the query itself. The bookmarks engine just starts with the list of roots we care about, and recursively finds children only for nodes with type RESULT_TYPE_FOLDER.
Comment 14•9 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #13)
> I think we are fine there - we do find the query itself, but do not attempt
> to execute it and sync the results.
I think you misunderstood me, executing any query and syncing results would clearly be a mistake and I don't expect us to do such a dumb thing.
What I was saying is that when that query is added, moved or removed, we should ignore such operation, don't sync the query to another profile, cause those queries are created by firefox in each profile automatically. So it's about the query itself, not its contents.
So Sync should build a list of guids with the smartbookmark annotation, and when syncing ignore any action related to those guids.
Comment 15•9 years ago
|
||
Isn't that the same as the default bookmarks (ie, they are created once, then Sync attempts to track moves etc on them)? IMO we should reflect when a user moves one of these default queries to a non-default folder. Or am I still missing your point? :)
Updated•9 years ago
|
Priority: P1 → P2
Summary: Don't sync default bookmarks from new profiles → Ensure that default bookmarks aren't duplicated
Comment 16•9 years ago
|
||
There are 2 ways this bug manifests itself:
* On profile 1, delete the default bookmarks, then setup Sync. On profile 2 setup Sync. The default bookmarks from profile 2 are then synced back to profile 1. This is what most of the conversations above are about, and is difficult to fix.
Chris says another manifestation is:
* On profile 1 leave the default bookmarks alone and setup Sync. On profile 2 leave the default bookmarks alone and setup Sync. Both profiles now have 2 copies of these default bookmarks. This *should* be easier to fix as Sync already has some duplicate detection code that should be able to handle this case (and I'm surprised it doesn't)
This latter case is what we are now trying to handle in this bug.
Comment 17•9 years ago
|
||
Assigning myself to investigate the second case.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Updated•9 years ago
|
Comment 18•8 years ago
|
||
Clearing myself for now. The bookmarks tracker refactor should make it easier to ignore changes to default bookmarks.
Assignee: kcambridge → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Whiteboard: [fxsync] → [sync-data-integrity]
Updated•8 years ago
|
Rank: 15
Whiteboard: [sync-data-integrity] → [data-integrity]
Updated•8 years ago
|
Assignee: nobody → kcambridge
Priority: P2 → P3
Whiteboard: [data-integrity] → [sync-quality]
Comment 19•8 years ago
|
||
And...unassigning myself again, because the engineering work for this is likely to happen in the other tracker bugs.
Assignee: kcambridge → nobody
Comment 21•8 years ago
|
||
Bug 1315351 has another "interesting" issue with these default bookmarks - 2 computers with different locales, meaning the default bookmarks will have different titles, even if the URL itself is identical. Simply assigning a GUID to the bookmark still will not do the right thing - sync would match them via GUID, but would consider the different titles being a change, so one of the 2 bookmarks would have the title adjusted - probably not what the user expects.
Updated•8 years ago
|
Comment 22•7 years ago
|
||
I wrote a bit more about localized default bookmarks in bug 1401595, comment 1.
Updated•7 years ago
|
Comment 23•7 years ago
|
||
Once bug 1343103 lands, all we should need to do is ensure the default bookmarks have a pre-defined guid.
Depends on: 1343103
Priority: P3 → P2
Updated•7 years ago
|
Priority: P2 → P1
Comment 24•7 years ago
|
||
This is probably most of the work that needs to be done, I did this a while ago, but am attaching it in case someone ends up taking it before me.
Updated•7 years ago
|
Priority: P1 → P2
Updated•6 years ago
|
Updated•5 years ago
|
Priority: P2 → P3
Updated•2 years ago
|
Severity: normal → S3
Comment 27•2 years ago
|
||
The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:skhamis, could you consider increasing the bug severity?
For more information, please visit auto_nag documentation.
Flags: needinfo?(skhamis)
Comment 28•2 years ago
|
||
The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.
Flags: needinfo?(skhamis)
You need to log in
before you can comment on or make changes to this bug.
Description
•