708149 - Handling of unsupported bookmark records

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Richard Newman [:rnewman]]

Native Fennec has a simpler bookmark schema than Sync.

In particular, it only supports two types of records (folder and bookmark), and it's missing columns (keyword, tags, description, as well as the ones for livemarks et al).

Right now we can live without some of these things, but:

* It would be nice if the basics (keyword, tags, description) could be persisted, allowing round-tripping of ordinary bookmark records
* Eventually we'll perhaps need to persist other kinds of bookmarks in a separate store, in order to allow for round-tripping.

We also need to document this.

Comment 1

[Lucas Rocha (:lucasr)]

I could add the keyword, tags, description columns if you really need them. Although those are things that we won't be using in Fennec at all. Isn't it simpler to just discard those things when syncing to Fennec? What are the specific complications of not having those columns?

Comment 2

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> I could add the keyword, tags, description columns if you really need them.
> Although those are things that we won't be using in Fennec at all. Isn't it
> simpler to just discard those things when syncing to Fennec? What are the
> specific complications of not having those columns?

Remember, syncing is bidirectional. If we discard those things, then the next time we sync that record *up* (e.g., if you change that bookmark in Fennec, or if Fennec is the first device to notice a Sync node reassignment), then the user will have lost data. We need Fennec to allow us to store what users expect to round-trip.

Given how much user rage we received when we accidentally *re-ordered* people's bookmarks, I am keen to avoid real data loss of things that users rely on in their Awesomebar.

(And Fennec's Awesome screen could someday search on those fields even if not displayed in the UI, of course. Free data!)

The alternative is for us to create and manage another database to store the fields that Fennec doesn't. That doesn't seem like the best solution: two database handles, no shared transactionality, Sync-side management of profiles, stale data in Clear User Data scenarios, etc. etc.

If you're concerned with per-row space usage, I would be just as happy with a second table for this kind of property: (androidID, keyword, description, tagsJSON).

Comment 3

[Richard Newman [:rnewman]]

In the short term we're just going to drop livemarks/microsummaries/etc. on the floor, and hope that users don't ever have Fennec be their primary data source! But we need to address that someday. Keyword etc. are slightly more urgent, and easier to fix now than later.

Comment 4

[Tracy Walker [:tracy]]

I think this needs to be addressed to prevent a diminished desktop UX as in Bug 731024.

requesting P1 for both bugs.

Comment 5

[Richard Newman [:rnewman]]

Ramping this up due to Bug 731024.

Should be straightforward: an integer column grows from two-valued to N-valued, the CP learns about those, and some of the UI code guards against the additional types.

Setting P2 to match the other bug; can retriage when the flag? gets processed.

Comment 6

[Richard Newman [:rnewman]]

Additional record types:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsINavBookmarksService#Constants

TYPE_BOOKMARK           1 	This is an index for type "Bookmark".
TYPE_FOLDER             2 	This is an index for type "Folder".
TYPE_SEPARATOR          3 	This is an index for type "Separator".
TYPE_DYNAMIC_CONTAINER  4 	This is an index for type "Dynamic Container".

with the following logic layered on top:

    switch (type) {
      case bms.TYPE_FOLDER:
        if (PlacesUtils.annotations
                       .itemHasAnnotation(itemId, PlacesUtils.LMANNO_FEEDURI)) {
          return "livemark";
        }
        return "folder";

      case bms.TYPE_BOOKMARK:
        let bmkUri = bms.getBookmarkURI(itemId).spec;
        if (bmkUri.indexOf("place:") == 0) {
          return "query";
        }
        return "bookmark";

      case bms.TYPE_SEPARATOR:
        return "separator";

      default:
        return null;

and


  getTypeObject: function PlacesItem_getTypeObject(type) {
    switch (type) {
      case "bookmark":
      case "microsummary":
        return Bookmark;
      case "query":
        return BookmarkQuery;
      case "folder":
        return BookmarkFolder;
      case "livemark":
        return Livemark;
      case "separator":
        return BookmarkSeparator;
      case "item":
        return PlacesItem;
    }
    throw "Unknown places item object type: " + type;
  },

That is: Sync treats microsummaries as bookmarks, and otherwise supports queries, folders, livemarks, separators, and a generic fallthrough case (that I'm happy to ignore). Sync has no notion of a DYNAMIC_CONTAINER (which is essentially a program-driven folder, I think). 

I suggest you extend your enumeration to be

  bookmark:  0
  folder:    1
  separator: 2
  livemark:  3
  query:     4

which doesn't map to Places, but does have enough granularity that you can display each type differently.

I'd be happy to get mak's input on this.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Factor out method to migrate bookmarks table

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Allow bookmarks table migration to apply a custom migrator

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Replace folder column with type in bookmarks table

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 605741 [details] [diff] [review]
(2/3) Allow bookmarks table migration to apply a custom migrator

Review of attachment 605741 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/BrowserProvider.java.in
@@ +180,5 @@
>  
>      private HashMap<String, DatabaseHelper> mDatabasePerProfile;
>  
> +    private interface BookmarkMigrator {
> +        public void migrate(ContentValues bookmark);

I'd be inclined to say "Translate", not "Migrate", because it doesn't actually effect the move; it just alters the values.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 605743 [details] [diff] [review]
(3/3) Replace folder column with type in bookmarks table

Review of attachment 605743 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but please split the Sync changes into a separate patch (and point me to it or upload it here!) so I have an easier time backporting the fixes to the canonical repo.

Thanks!

::: mobile/android/base/tests/testBrowserProvider.java.in
@@ +158,1 @@
>                  String keyword) throws Exception {

Odd indentation…

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 605741 [details] [diff] [review]
> (2/3) Allow bookmarks table migration to apply a custom migrator
> 
> Review of attachment 605741 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserProvider.java.in
> @@ +180,5 @@
> >  
> >      private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> >  
> > +    private interface BookmarkMigrator {
> > +        public void migrate(ContentValues bookmark);
> 
> I'd be inclined to say "Translate", not "Migrate", because it doesn't
> actually effect the move; it just alters the values.

I agree "migrate" is not perfect but translate might be a bit misleading (because it clearly has different meanings in other contexts). Maybe "update" or "edit"?

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #11)
> Comment on attachment 605743 [details] [diff] [review]
> (3/3) Replace folder column with type in bookmarks table
> 
> Review of attachment 605743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, but please split the Sync changes into a separate
> patch (and point me to it or upload it here!) so I have an easier time
> backporting the fixes to the canonical repo.
> 
> Thanks!

I can upload a separate patch here but I'll still push the changes in one go because I don't want to push a patch that breaks the build.

> ::: mobile/android/base/tests/testBrowserProvider.java.in
> @@ +158,1 @@
> >                  String keyword) throws Exception {
> 
> Odd indentation…

Fixed.

Comment 14

[Lucas Rocha (:lucasr)]

I've made a small change to the third patch to ensure we keep the same behaviour (only showing folders and bookmarks) until we implement proper support for showing the other types of bookmarks. This is bug 737024.

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Replace folder column with type in bookmarks table

Comment 16

[Lucas Rocha (:lucasr)]

This patch only makes the small change I mentioned above in the getBookmarksInFolder() method.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 607167 [details] [diff] [review]
Replace folder column with type in bookmarks table

Review of attachment 607167 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/LocalBrowserDB.java
@@ +356,5 @@
> +                         Bookmarks.PARENT + " = ? AND " +
> +                         "(" + Bookmarks.TYPE + " = ? OR " + Bookmarks.TYPE + " = ?)",
> +                         new String[] { String.valueOf(folderId),
> +                                        String.valueOf(Bookmarks.TYPE_BOOKMARK),
> +                                        String.valueOf(Bookmarks.TYPE_FOLDER) },

I know it's slightly more obscure, but I would favor

  "Bookmarks.TYPE <= " + Bookmarks.TYPE_BOOKMARK

here. It's not injectable, and should be slightly more efficient (both in terms of allocating a shorter array and using a range query on an indexed column in the database). Commented, of course.

(The type enumerations are ordered for this reason.)

No big deal if you massively disagree.

Comment 18

[Richard Newman [:rnewman]]

> I agree "migrate" is not perfect but translate might be a bit misleading
> (because it clearly has different meanings in other contexts). Maybe
> "update" or "edit"?

"update" makes sense to me!

Here's something else to ruminate:

If we're ever in a situation whereby a migration involves changing more than one row (e.g., to implement a normalization), then this isn't going to work... and having a genuine "migrate" (that is, the BookmarkMigrator is given a row as input and a destination to which to store) would be appropriate.

Nothing that needs fixing now, but bear it in mind for the next schema rev :D

Comment 19

[Lucas Rocha (:lucasr)]

(In reply to Richard Newman [:rnewman] from comment #18)
> > I agree "migrate" is not perfect but translate might be a bit misleading
> > (because it clearly has different meanings in other contexts). Maybe
> > "update" or "edit"?
> 
> "update" makes sense to me!

I went with updateForNewTable() to be more obvious.

> Here's something else to ruminate:
> 
> If we're ever in a situation whereby a migration involves changing more than
> one row (e.g., to implement a normalization), then this isn't going to
> work... and having a genuine "migrate" (that is, the BookmarkMigrator is
> given a row as input and a destination to which to store) would be
> appropriate.
> 
> Nothing that needs fixing now, but bear it in mind for the next schema rev :D

Good point.

Comment 20

[Richard Newman [:rnewman]]

> > This looks good to me, but please split the Sync changes into a separate
> > patch (and point me to it or upload it here!) so I have an easier time
> > backporting the fixes to the canonical repo.

I merged these in by hand:

https://github.com/mozilla-services/android-sync/pull/116

When you land this on inbound, I'll merge that pull request.

Comment 21

[Lucas Rocha (:lucasr)]

Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/41a2a8a1bee8
http://hg.mozilla.org/integration/mozilla-inbound/rev/3c0bcdf20a9b
http://hg.mozilla.org/integration/mozilla-inbound/rev/83f8ebd969ce

Comment 22

[Richard Newman [:rnewman]]

Merged upstream, too. Thanks Lucas!

Comment 23

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/41a2a8a1bee8
https://hg.mozilla.org/mozilla-central/rev/3c0bcdf20a9b
https://hg.mozilla.org/mozilla-central/rev/83f8ebd969ce

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 605740 [details] [diff] [review]
(1/3) Factor out method to migrate bookmarks table

Mobile-only. Beta blocker.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 605741 [details] [diff] [review]
(2/3) Allow bookmarks table migration to apply a custom migrator

Mobile-only. Beta blocker.

Comment 26

[Lucas Rocha (:lucasr)]

Comment on attachment 607167 [details] [diff] [review]
Replace folder column with type in bookmarks table

Mobile-only. Beta blocker.

Comment 27

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #26)
> Mobile-only. Beta blocker.

Has the plan changed to release Beta from Aurora rather than m-c?

If so, there are plenty of other bugs that need to be uplifted before this one would have an effect, right? (Because this only helps Sync, and I haven't landed anything on Aurora since the last uplift.)

I thought we'd all breathed a sigh of relief as someone pointed out that dual-landing was wasting everyone's time?

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #27)
> (In reply to Lucas Rocha (:lucasr) from comment #26)
> > Mobile-only. Beta blocker.
> 
> Has the plan changed to release Beta from Aurora rather than m-c?

Yeah. We do not want to release a beta directly from m-c. We like having a little bit a stablility, offered by Aurora.
> 
> If so, there are plenty of other bugs that need to be uplifted before this
> one would have an effect, right? (Because this only helps Sync, and I
> haven't landed anything on Aurora since the last uplift.)

Let's get those bugs approved for Aurora using the approval-aurora flags on the patches. We can land those approved patches.

> I thought we'd all breathed a sigh of relief as someone pointed out that
> dual-landing was wasting everyone's time?

We are not in a "land everything in two places" mode like we were, but we do want to use a stable branch to release.

Comment 29

[Richard Newman [:rnewman]]

> > Has the plan changed to release Beta from Aurora rather than m-c?
> 
> Yeah. We do not want to release a beta directly from m-c. We like having a
> little bit a stablility, offered by Aurora.

I see.


> > If so, there are plenty of other bugs that need to be uplifted before this
> > one would have an effect, right? (Because this only helps Sync, and I
> > haven't landed anything on Aurora since the last uplift.)
> 
> Let's get those bugs approved for Aurora using the approval-aurora flags on
> the patches. We can land those approved patches.

I don't know the strict dependency tree. (Neither do we have patches on bugs to mark for approval.)

This will be a case of `cp -R`, and then continuing to `cp -R` chunks of Fennec until it builds, then `hg commit -m "I'm sorry."`.

Essentially every piece of Fennec that touches the BrowserDB, BrowserProvider, BrowserContract, FormHistoryProvider, PasswordsProvider, or Sync account setup needs to be uplifted, along with the whole Sync directory and makefiles.


> We are not in a "land everything in two places" mode like we were, but we do
> want to use a stable branch to release.

Can I politely ask that this gets communicated next time?

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 605740 [details] [diff] [review]
(1/3) Factor out method to migrate bookmarks table

[Triage Comment]
Mobile only, approving for some stability time on Aurora.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5223a4a37e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/56b4f77bbc10
https://hg.mozilla.org/releases/mozilla-aurora/rev/7d910d51187e