Closed Bug 708149 Opened 8 years ago Closed 8 years ago

Handling of unsupported bookmark records

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
firefox13 --- fixed
firefox14 --- fixed
blocking-fennec1.0 --- +
fennec + ---

People

(Reporter: rnewman, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [sync])

Attachments

(3 files, 1 obsolete file)

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.
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?
(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).
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.
tracking-fennec: --- → ?
tracking-fennec: ? → +
Priority: -- → P3
I think this needs to be addressed to prevent a diminished desktop UX as in Bug 731024.

requesting P1 for both bugs.
Blocks: 731024
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.
Assignee: nobody → lucasr.at.mozilla
Status: NEW → ASSIGNED
blocking-fennec1.0: --- → ?
Component: Android Sync → General
Priority: P3 → P2
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
blocking-fennec1.0: ? → +
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.
Whiteboard: [sync]
Attachment #605743 - Flags: review?(rnewman)
Attachment #605740 - Flags: review?(rnewman) → review+
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.
Attachment #605741 - Flags: review?(rnewman) → review+
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…
Attachment #605743 - Flags: review?(rnewman) → review+
(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"?
(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.
Blocks: 737024
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.
Attachment #605743 - Attachment is obsolete: true
This patch only makes the small change I mentioned above in the getBookmarksInFolder() method.
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.
Attachment #607167 - Flags: review?(rnewman) → review+
> 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
(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.
> > 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.
Merged upstream, too. Thanks Lucas!
https://hg.mozilla.org/mozilla-central/rev/41a2a8a1bee8
https://hg.mozilla.org/mozilla-central/rev/3c0bcdf20a9b
https://hg.mozilla.org/mozilla-central/rev/83f8ebd969ce
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Version: unspecified → Trunk
Comment on attachment 605740 [details] [diff] [review]
(1/3) Factor out method to migrate bookmarks table

Mobile-only. Beta blocker.
Attachment #605740 - Flags: approval-mozilla-aurora?
Comment on attachment 605741 [details] [diff] [review]
(2/3) Allow bookmarks table migration to apply a custom migrator

Mobile-only. Beta blocker.
Attachment #605741 - Flags: approval-mozilla-aurora?
Comment on attachment 607167 [details] [diff] [review]
Replace folder column with type in bookmarks table

Mobile-only. Beta blocker.
Attachment #607167 - Flags: approval-mozilla-aurora?
Depends on: 735660
(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?
(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.
> > 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?
Blocks: 737951
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.
Attachment #605740 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #605741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #607167 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.