1177516 - Ensure that default bookmarks aren't duplicated

Status: NEW

(Firefox :: Sync, defect, P3)

Description

[Ryan Kelly [:rfkelly]]

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?

Comment 1

[Chris Karlof [:ckarlof]]

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.

Comment 2

[Ryan Feeley [:rfeeley]]

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.

Comment 3

[Chris Karlof [:ckarlof]]

We can probably address this by giving the default bookmarks a magic GUID.

Comment 4

[Chris Karlof [:ckarlof]]

This is early in the user's 2nd device Sync experience, and it's bad.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

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

[Mark Hammond [:markh] [:mhammond]]

(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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

PS: but note, only on new profiles, we cannot distinguish default bookmarks on old profiles.

Comment 9

[Chris Karlof [:ckarlof]]

> 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

[Richard Newman [:rnewman]]

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

[Chris Karlof [:ckarlof]]

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

[Marco Bonardo [:mak]]

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

[Mark Hammond [:markh] [:mhammond]]

(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

[Marco Bonardo [:mak]]

(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

[Mark Hammond [:markh] [:mhammond]]

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? :)

Comment 16

[Mark Hammond [:markh] [:mhammond]]

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

[Lina Butler [:lina]]

Assigning myself to investigate the second case.

Comment 18

[Lina Butler [:lina]]

Clearing myself for now. The bookmarks tracker refactor should make it easier to ignore changes to default bookmarks.

Comment 19

[Lina Butler [:lina]]

And...unassigning myself again, because the engineering work for this is likely to happen in the other tracker bugs.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 22

[Lina Butler [:lina]]

I wrote a bit more about localized default bookmarks in bug 1401595, comment 1.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Once bug 1343103 lands, all we should need to do is ensure the default bookmarks have a pre-defined guid.

Comment 24

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: 0001-Bug-1177516-WIP-Ensure-that-default-bookmarks-have-c.patch

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.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 28

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

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.