Handling of unsupported bookmark records

RESOLVED FIXED in Firefox 13

Status

()

Firefox for Android
General
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: lucasr)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, blocking-fennec1.0 +, fennec+)

Details

(Whiteboard: [sync])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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?
(Reporter)

Comment 2

6 years ago
(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).
(Reporter)

Comment 3

6 years ago
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: --- → ?

Updated

5 years ago
tracking-fennec: ? → +
(Reporter)

Updated

5 years ago
Priority: -- → P3
I think this needs to be addressed to prevent a diminished desktop UX as in Bug 731024.

requesting P1 for both bugs.
(Reporter)

Updated

5 years ago
Blocks: 731024
(Reporter)

Comment 5

5 years ago
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: ? → +
(Reporter)

Comment 6

5 years ago
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]
(Assignee)

Comment 7

5 years ago
Created attachment 605740 [details] [diff] [review]
(1/3) Factor out method to migrate bookmarks table
Attachment #605740 - Flags: review?(rnewman)
(Assignee)

Comment 8

5 years ago
Created attachment 605741 [details] [diff] [review]
(2/3) Allow bookmarks table migration to apply a custom migrator
Attachment #605741 - Flags: review?(rnewman)
(Assignee)

Comment 9

5 years ago
Created attachment 605743 [details] [diff] [review]
(3/3) Replace folder column with type in bookmarks table
Attachment #605743 - Flags: review?(rnewman)
(Reporter)

Updated

5 years ago
Attachment #605740 - Flags: review?(rnewman) → review+
(Reporter)

Comment 10

5 years ago
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+
(Reporter)

Comment 11

5 years ago
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+
(Assignee)

Comment 12

5 years ago
(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"?
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 737024
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
Created attachment 607167 [details] [diff] [review]
Replace folder column with type in bookmarks table
Attachment #607167 - Flags: review?(rnewman)
(Assignee)

Updated

5 years ago
Attachment #605743 - Attachment is obsolete: true
(Assignee)

Comment 16

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

Comment 17

5 years ago
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+
(Reporter)

Comment 18

5 years ago
> 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
(Assignee)

Comment 19

5 years ago
(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.
(Reporter)

Comment 20

5 years ago
> > 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.
(Assignee)

Comment 21

5 years ago
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
(Reporter)

Comment 22

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Version: unspecified → Trunk
(Assignee)

Comment 24

5 years ago
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?
(Assignee)

Comment 25

5 years ago
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?
(Assignee)

Comment 26

5 years ago
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?
(Assignee)

Updated

5 years ago
Depends on: 735660
(Reporter)

Comment 27

5 years ago
(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.
(Reporter)

Comment 29

5 years ago
> > 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?
(Reporter)

Updated

5 years ago
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+
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
status-firefox13: --- → fixed
status-firefox14: --- → fixed
You need to log in before you can comment on or make changes to this bug.