1162717 - Make BookmarkJSONUtils import the "mobile" root

Status: RESOLVED WORKSFORME

(Toolkit :: Places, defect)

Description

[Nick Alexander :nalexander [he/him]]

Over in Bug 775104, we're trying to make it easy to export your bookmarks from Firefox for Android and import them into Firefox Desktop.  We've got a patch that is basically working, except importing the "mobile" root does not work on Desktop.  I'd like to discuss making that work.

First, can we get some context on why Desktop does not *export* the "mobile" root in its backups?  ("mobile" is carefully marked to not export.)

Second, is the code at [1] intentionally not handling "mobile", or is this just an oversight?  ("mobile is a relatively new root.)

Third, would this get support from the Toolkit/Places peers?

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#305

Comment 1

[Nick Alexander :nalexander [he/him]]

gavin, marco: could you redirect or comment on the above questions?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #0)
> Over in Bug 775104, we're trying to make it easy to export your bookmarks
> from Firefox for Android and import them into Firefox Desktop.  We've got a
> patch that is basically working, except importing the "mobile" root does not
> work on Desktop.

This code writes a JSON file in something close to the current desktop format.  It writes all roots, by design -- it's intended as a way to move all bookmarks from a Mobile device to a Desktop device, without using Sync.

Comment 3

[Marco Castelluccio [:marco]]

Sorry, you probably meant another Marco!

Comment 4

[Richard Newman [:rnewman]]

IIRC as of ~2011, maybe into 2012, the mobile "root" wasn't a root at all — it was created by Sync and marked with the mobile root anno. See e.g., Bug 627519 for context.

I imagine that assumption is still baked in here. Seems like an excellent bug for mak and/or me to mentor.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, the mobile root was never a real root, since we never had an API to add roots.
It was built with the idea that Sync would have recreated it when missing.
Backups usually try to store things that user doesn't want to loose, and tries to keep that at a minimum. Add-ons can add fake roots and inflate our backups by a lot, thus we don't want to store roots we don't know about.
We could probably make it a bit more official.

What I'm not sure about is how do you plan to build the json, since from json we only have the option to Restore, if you json has only the mobile root, a user restoring from it would lose all of his bookmarks on menu, toolbar and unsorted... The format we usually support to Import (not restore) bookmarks is usually bookmarks.html format. Supporting partial imports from JSON is a lot more work.

So I'd first like to understand the plan and the use-case this is covering.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Marco Bonardo [::mak] from comment #5)
> yes, the mobile root was never a real root, since we never had an API to add
> roots.
> It was built with the idea that Sync would have recreated it when missing.
> Backups usually try to store things that user doesn't want to loose, and
> tries to keep that at a minimum. Add-ons can add fake roots and inflate our
> backups by a lot, thus we don't want to store roots we don't know about.
> We could probably make it a bit more official.
> 
> What I'm not sure about is how do you plan to build the json, since from
> json we only have the option to Restore, if you json has only the mobile
> root, a user restoring from it would lose all of his bookmarks on menu,
> toolbar and unsorted... The format we usually support to Import (not
> restore) bookmarks is usually bookmarks.html format. Supporting partial
> imports from JSON is a lot more work.

Yeah, when we started this work I was not aware of this.  More below.

> So I'd first like to understand the plan and the use-case this is covering.

We get a slow trickle of folks who want to copy their bookmarks from Mobile to Desktop and for reasons unknown are not able or willing to use Sync.

I believe we have users who

1) have more than just the "mobile" root on Mobile; and
2) may have additional bookmarks on Desktop.

For 1), I belive this because we heard from users who had Sync set up, had a complete set of bookmarks on Mobile (i.e., including Desktop's roots, not just the "mobile" root), and then disconnected their Desktop or got a new Desktop and didn't want to continue using Sync.

For 2), I have no anecdotes or data.

Given 1), Mobile must export all roots.

Given 2), JSON import is perhaps not the right approach: I was not aware that importing from JSON was so different from HTML, and that from JSON was so limiting :(  Importing from HTML is not great because the import is not clever with GUIDs; it always adds the imported records, even when they are present.  Importing from JSON is at least easy to explain -- we'll replace everything -- but does not do reasonable things with external roots :(

Does this help clarify the use case?  I have a perspective on what we should do but I'd like your input, mak.  Thanks!

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Nick Alexander :nalexander from comment #6)
> Given 2), JSON import is perhaps not the right approach: I was not aware
> that importing from JSON was so different from HTML, and that from JSON was
> so limiting :(

It's not a limitation due to impossibility to make it different, it's just how things were designed. JSON is the backup/restore format cause it's more data complete, HTML is the import/export format, mostly because services (like delicious) and other browsers (IE and Safari iirc) support exporting in html format.
Due to that, they were designed so that json always overwrite everything, while html always appends everything.

> Importing from HTML is not great because the import is not
> clever with GUIDs; it always adds the imported records, even when they are
> present.  Importing from JSON is at least easy to explain -- we'll replace
> everything -- but does not do reasonable things with external roots :(

Yes, I don't think the user wants to throw away all desktop bookmarks to import mobile ones.
Here you are requesting a special "merge" of bookmarks that should also be able to recognize if a bookmark already exists locally, and that's something we don't have. The only thing doing something close to that is Sync.

I see 2 possible solutions:
1. implement something like "import into folder" that puts all bookmarks from an html file into a user named folder, then the user will have to reorder the bookmarks in the Library and remove dupes.
2. Have the html import cide able to merge bookmarks. A simple heuristic could be to avoid creating a bookmark to a url in a folder where such url already exists. It might not be trivial to hook into the existing code, but it's feasible.

I think you likely want 2, and it wouldn't be that bad also as a behavior when importing from external services or other browsers, but it's also the more expensive solution, moreover you need someone to implement that, since I doubt I have any time to do that :/

I'm not sure regarding the guid problem with html, the fact is that in any case also json doesn't import guids (bug 967204). adding guids support to html shouldn't be too hard too.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I'd also say you don't really need the "mobile" root for your use case, it would be good enough to put bookmarks under "unfiled bookmarks", maybe in a specially named folder. you might put bookmarks that are more relevant into toolbar/menu depending on how much relevant they are in the mobile UI.

For the simplest case, you could already generate an html file containing "mobile" bookmarks and let Firefox append them to toolbar/menu/unfiled, the duplicates detection could happen in a follow-up. Since the users you are targeting are not using sync, there might not be too many dupes, they could be manually handled. Indeed this is already the path for users importing from external services.

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The mobile root has been recently changed and I think this was addressed