Ryan Kelly [:rfkelly] Reporter
	Description • 9 years ago

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?

	Chris Karlof [:ckarlof]
	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.

	Ryan Feeley [:rfeeley]
	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.

	Chris Karlof [:ckarlof]
	Comment 3 • 9 years ago

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

	Chris Karlof [:ckarlof]
	Comment 4 • 8 years ago

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

	Mark Hammond [:markh] [:mhammond]
	Comment 5 • 8 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.

	Mark Hammond [:markh] [:mhammond]
	Comment 6 • 8 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.

	Marco Bonardo [:mak]
	Comment 7 • 8 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.

	Marco Bonardo [:mak]
	Comment 8 • 8 years ago

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

	Chris Karlof [:ckarlof]
	Comment 9 • 8 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.

	Richard Newman [:rnewman]
	Comment 10 • 8 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.

	Chris Karlof [:ckarlof]
	Comment 11 • 8 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.

	Marco Bonardo [:mak]
	Comment 12 • 8 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).

	Mark Hammond [:markh] [:mhammond]
	Comment 13 • 8 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.

	Marco Bonardo [:mak]
	Comment 14 • 8 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.

	Mark Hammond [:markh] [:mhammond]
	Comment 15 • 8 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? :)

	Mark Hammond [:markh] [:mhammond]
	Comment 16 • 8 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.

	Lina Butler [:lina]
	Comment 17 • 8 years ago

Assigning myself to investigate the second case.

	Lina Butler [:lina]
	Comment 18 • 8 years ago

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

	Lina Butler [:lina]
	Comment 19 • 8 years ago

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

	Mark Hammond [:markh] [:mhammond]
	Comment 21 • 7 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.

	Lina Butler [:lina]
	Comment 22 • 7 years ago

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

	Mark Hammond [:markh] [:mhammond]
	Comment 23 • 6 years ago

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

	Thom Chiovoloni [:tcsc] (ex-moco)
	Comment 24 • 6 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.