Closed Bug 704490 Opened 13 years ago Closed 13 years ago

Add support for using local DBs for Bookmarks and History

Categories

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

x86
Linux
defect

Tracking

(firefox11 fixed, firefox-esr10 wontfix, status1.9.2 unaffected, fennec11+)

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
firefox-esr10 --- wontfix
status1.9.2 --- unaffected
fennec 11+ ---

People

(Reporter: mfinkle, Assigned: lucasr)

References

Details

(Whiteboard: [sg:want])

Attachments

(3 files, 4 obsolete files)

Currently Fennec uses the Android system DBs for storing bookmarks and history. We'd like to add support for using a local (in Fennec profile) DBs as well. The DBs can have the same schema as the system DBs so switching between them is easy.
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P1
mfinkle: do you have a timeline for this? And do you intend to do it seamlessly through a separate content provider?

I don't have a good answer for how Sync (which isn't aware of Fennec profiles) could decide which DB to use, or how to make that behave correctly. (Which is not to say I don't think offering non-Google storage is a good idea.)
Seems like we should expose the DBs as content providers, so Sync can access and update the data even while Firefox is not running. We can also assume that we'll only have a single profile for now, but we should put some kind of "what is the current profile" mechanism into the system. I don't know exactly how that would work yet.
Blocks: 695463
Whiteboard: [sg:want]
A few more thoughts around profiles and intermediation:

* Sync would presumably store some reference to the profile in its Sync account record on the device. The string name should suffice.

* During setup, we'd need a way to ask for a list of profiles, in order to decide which ones to offer in the UI. Alternatively, if Fennec invokes our setup activity, it could include the destination profile in the URI. (This would be the most likely flow, I think.)

* During sync, we'd need an interface which takes a profile as input and returns a collection of content providers to which we can sync. We'd call this on every sync.

* This information hand-off would presumably also include just enough tracking information for us to figure out when the user has switched databases, so we know to do a full sync and clean up our tracking metadata.
Attachment #577989 - Flags: review?(blassey.bugs)
Attachment #577990 - Flags: review?(blassey.bugs)
I have Fennec fully working using local DB with those 2 patches. No obvious crashes AFAIK. The only piece that is still missing is how we're going to switch a profile between Android's and Fennec's DB. My initial idea is to use a simple Android settings key per profile. Any other ideas? We don't want to rely on Gecko prefs for that because the correct DB has to be immediately usable even before Gecko is fully loaded.

A possible follow-up patch is to create default bookmarks when DB is first created. Android uses a simple array resource for that. We could do the same.
Attachment #577990 - Flags: feedback?(rnewman)
Attachment #577990 - Flags: feedback?(mark.finkle)
Attachment #577990 - Flags: feedback?(kgupta)
Attachment #577989 - Flags: feedback?(rnewman)
Attachment #577989 - Flags: feedback?(mark.finkle)
Attachment #577989 - Flags: feedback?(kgupta)
Comment on attachment 577989 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

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

Code-wise, this looks good to me. Now, on to the meat!


Further to initial conversation on IRC: in general, Sync wants a lastModified and a GUID column for all data sources that it needs to sync. Having now looked at some context for that pre-coffee conversation (i.e., this patch!), I need to give you a little more of a nuanced opinion :D


For the built-in Browser DB we can't add columns, so we're implementing a two-stage sync as described here: <https://wiki.mozilla.org/Services/NativeSync/Bookmarks_and_History>.


For Fennec's own hybrid favicons (which we'll want to sync eventually, and thus should plan for now) this has a slight complication. We care about when the favicon data changed (favicon modified time) and when the bookmark changed to point to a different favicon (bookmark modified time). You have a separate table for the latter, and the data for the former is jammed into the built-in Browser table. We obviously don't want to re-upload a favicon every time a new *.foo.com page points to the same ol' sequence of bytes.

(This is a very different schema to Places, which has favicons as a first-class entity: it's inverted.)

I'm not sure what proposal to make on this front. The Android design is bass-ackward. What do you think about having another table for favicons, reifying them, on which we can hang URL (the real primary key), guid, change time?


For bookmarks and history we have two choices.

On one hand, you could just behave exactly like the built-in Browser store, and we'll apply exactly the same kinds of awful workarounds that we do for them. This is arguably simpler, but has tradeoffs. We'll just use your translation interface instead of Browser.

On the other hand, you could provide a rich schema: every record having a GUID and a lastModified time. We're then free to use those to implement a better scheme, eventually allowing us to rely on ICS and Fennec's improved schema to discard our duplicate DB.

The question then becomes how to layer that on top of the built-in stuff, given that this is an abstraction layer, and it might not do to have some rows come back simply without a GUID or lastModified!


This might need some more pondering.

::: mobile/android/base/Tab.java
@@ +335,5 @@
>      private class AddBookmarkTask extends GeckoAsyncTask<Void, Void, Void> {
>          @Override
>          protected Void doInBackground(Void... unused) {
>              ContentResolver resolver = Tabs.getInstance().getContentResolver();
> +            BrowserDB.addBookmark(resolver, getTitle(), getURL());

Doesn't this schema just make ya cry?
Attachment #577989 - Flags: feedback?(rnewman) → feedback+
One more thing I forgot to mention in my comments for Part 1: of course, for this to be useful for Sync it has to be accessible without being built-in to Fennec. We need to be able to ask the system for an appropriate instance that can run queries on our behalf.

That means you need to implement a ContentProvider/ContentResolver. sriram is the source of knowledge here :)
Comment on attachment 577990 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

Clearing review flag on this until we decide on a direction for Sync support.
Attachment #577990 - Flags: feedback?(rnewman)
Comment on attachment 577989 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

I assume you tested as much of the application with this patch applied as you could, looking for any breakage.
Attachment #577989 - Flags: feedback?(mark.finkle) → feedback+
Updated patch to also cover the new native about:home.
Attachment #577989 - Attachment is obsolete: true
Attachment #577989 - Flags: review?(blassey.bugs)
Attachment #577989 - Flags: feedback?(kgupta)
Attachment #578228 - Flags: review?(blassey.bugs)
Updated patch to also cover the new native about:home.
Attachment #577990 - Attachment is obsolete: true
Attachment #577990 - Flags: review?(blassey.bugs)
Attachment #577990 - Flags: feedback?(mark.finkle)
Attachment #577990 - Flags: feedback?(kgupta)
Attachment #578229 - Flags: review?(blassey.bugs)
After approval, I suggest us to land these patches with local DB always disabled. I'll then write the switch mechanism (through an Android setting key) with its respective prefs UI on a follow-up patch to allow users to enable/disable it.

It would be great to land those patches asap because they touch many parts of the app and it will be a PITA to keep rebasing them all the time.
Comment on attachment 578228 [details] [diff] [review]
(1/2) Abstract all bookmark/history access behing a common API

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

::: mobile/android/base/db/AndroidBrowserDB.java
@@ +95,5 @@
> +    }
> +
> +    public Cursor getRecentHistory(ContentResolver cr, int limit) {
> +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> +                            null,

use a projection

@@ +111,5 @@
> +    }
> +
> +    public Cursor getAllBookmarks(ContentResolver cr) {
> +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> +                            null,

use a projection

@@ +125,5 @@
> +    }
> +
> +    public boolean isBookmark(ContentResolver cr, String uri) {
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection

@@ +138,5 @@
> +    }
> +
> +    public void addBookmark(ContentResolver cr, String title, String uri) {
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection

@@ +218,5 @@
> +            BitmapDrawable thumbnail) {
> +        Bitmap bitmap = thumbnail.getBitmap();
> +
> +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> +                                 null,

use a projection
Attachment #578228 - Flags: review?(blassey.bugs) → review+
(In reply to Brad Lassey [:blassey] from comment #15)
> Comment on attachment 578228 [details] [diff] [review] [diff] [details] [review]
> (1/2) Abstract all bookmark/history access behing a common API
> 
> Review of attachment 578228 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/AndroidBrowserDB.java
> @@ +95,5 @@
> > +    }
> > +
> > +    public Cursor getRecentHistory(ContentResolver cr, int limit) {
> > +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> > +                            null,
> 
> use a projection
> 
> @@ +111,5 @@
> > +    }
> > +
> > +    public Cursor getAllBookmarks(ContentResolver cr) {
> > +        Cursor c = cr.query(Browser.BOOKMARKS_URI,
> > +                            null,
> 
> use a projection
> 
> @@ +125,5 @@
> > +    }
> > +
> > +    public boolean isBookmark(ContentResolver cr, String uri) {
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection
> 
> @@ +138,5 @@
> > +    }
> > +
> > +    public void addBookmark(ContentResolver cr, String title, String uri) {
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection
> 
> @@ +218,5 @@
> > +            BitmapDrawable thumbnail) {
> > +        Bitmap bitmap = thumbnail.getBitmap();
> > +
> > +        Cursor cursor = cr.query(Browser.BOOKMARKS_URI,
> > +                                 null,
> 
> use a projection

Added all projections.
Yesterday rnewman and I discussed what needs to be in the local DB schema from a sync perspective. Here are the changes I'll make to the schema before landing those patches:

1. Add a "GUID" column to "History" and "Bookmarks" tables
2. Add "GUID" and "Last modified" columns "Images" table
3. Add "favicon url" column to Images table.

About 3, currently we have a separate database to store favicon URLs. However, we need to have this information in the local DB for syncing. So, my plan to move the favicon URL DB to be used only when profile is using Android's DB. On local DB, the favicon URL will be available directly in the "Images" table. I can do this on a follow-up patch (bug 707119) but I want to have the favicon-url in the local DB schema from day 1 to avoid having to write DB migration just for that later.

Other necessary follow-up changes:
1. Provide a special query returning the schema version (bug 707124)
2. Create a ContentProvider to provide profile information (list of profiles, which DB a profile is using, etc) (bug 707123)

Will be sending an updated 2/2 patch with the schema changes above in the next hour or so.
Updated patch with changes suggested by blassey. Keeping the r+.
Attachment #578228 - Attachment is obsolete: true
Attachment #578541 - Flags: review+
FYI: Filed bug 707150 as a follow-up bug to add mechanism to enable/disable local bookmarks/history DB.
Updated patch with all the schema changes to make local DB "sync-ready". All follow-up bugs are now files. Just waiting for review on 2/2 now.
Attachment #578229 - Attachment is obsolete: true
Attachment #578229 - Flags: review?(blassey.bugs)
Attachment #578564 - Flags: review?(blassey.bugs)
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

rnewman, I've added all the bits we agreed on yesterday. Could you have a look at the schema and let me know if there's anything missing?
Attachment #578564 - Flags: feedback?(rnewman)
FYI: I pushed patch 1/2 to avoid constant rebase headaches: http://hg.mozilla.org/projects/birch/rev/839f36123bce
FYI: backed out patch 1/2 due to Try failure: http://hg.mozilla.org/projects/birch/rev/6bc702e26354
(In reply to Lucas Rocha (:lucasr) from comment #17)
> 
> About 3, currently we have a separate database to store favicon URLs.
> However, we need to have this information in the local DB for syncing. So,
> my plan to move the favicon URL DB to be used only when profile is using
> Android's DB. On local DB, the favicon URL will be available directly in the
> "Images" table. I can do this on a follow-up patch (bug 707119) but I want
> to have the favicon-url in the local DB schema from day 1 to avoid having to
> write DB migration just for that later.

We also need _some_ way to get a GUID for a favicon. It doesn't matter to me where this lives, so long as we're at some point able to assemble the following:

  * GUID
  * Favicon URL
  * Timestamp favicon itself was last modified

and for each bookmark and history item, figure out (indirectly is fine) which favicon GUID applies.

If you can't do this, it's not a massive deal; we can keep a local DB to track these things. But it'd help having it all in one place.

> Will be sending an updated 2/2 patch with the schema changes above in the
> next hour or so.

Fantastic!
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

Ah, I see this:

  map.put(Images.GUID, Images.GUID);

so I think this all makes sense. Now let's see if I can access the CP from another app :D
Attachment #578564 - Flags: feedback?(rnewman) → feedback+
(In reply to Richard Newman [:rnewman] from comment #25)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Ah, I see this:
> 
>   map.put(Images.GUID, Images.GUID);
> 
> so I think this all makes sense. Now let's see if I can access the CP from
> another app :D

I'll be landing the CP with no way to access from outside Fennec (i.e. android:exported="false"). I filed a separate bug to figure out the right/safe way to expose the CP to Sync (see bug 707636).
Blocks: 701835
Blocks: 698828
Blocks: 701913
Blocks: 707732
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

::: mobile/android/base/db/BrowserProvider.java
@@ +284,5 @@
> +
> +        if (dbHelper == null) {
> +            synchronized (this) {
> +                dbHelper = new DatabaseHelper(getContext(), getDatabasePath(profile));
> +                mDatabasePerProfile.put(profile, dbHelper);

So, is it possible to operate on more than one profile at a time? This shouldn't happen in normal use of the browser, but I wonder if it is a requirement for sync?

@@ +327,5 @@
> +
> +        if (profileDir == null) {
> +            Log.d(LOGTAG, "Couldn't find directory for profile: " + profile);
> +            return null;
> +        }

There is a getProfileDir() function in GeckoApp now. Use it here and improve it if need be (there are two XXX ToDo's in the impl
(In reply to Brad Lassey [:blassey] from comment #27)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +284,5 @@
> > +
> > +        if (dbHelper == null) {
> > +            synchronized (this) {
> > +                dbHelper = new DatabaseHelper(getContext(), getDatabasePath(profile));
> > +                mDatabasePerProfile.put(profile, dbHelper);
> 
> So, is it possible to operate on more than one profile at a time? This
> shouldn't happen in normal use of the browser, but I wonder if it is a
> requirement for sync?

Yep. The idea is be able to deliver commands to the profile databases at any given time. The ContentProvider will keep separate database connections for each database accordingly.

> @@ +327,5 @@
> > +
> > +        if (profileDir == null) {
> > +            Log.d(LOGTAG, "Couldn't find directory for profile: " + profile);
> > +            return null;
> > +        }
> 
> There is a getProfileDir() function in GeckoApp now. Use it here and improve
> it if need be (there are two XXX ToDo's in the impl

Ok, will have a look into that.
Lucas: could you un-bitrot these? They don't apply on current m-c.

I was all set to do the following and try throwing a sketch patch at you…

• For "deletion", update lastModified and clear other columns
• Add a "uri IS NOT NULL" clause to retrieval operations
• Add a cleanup operation a la

  DELETE FROM bookmarks WHERE uri IS NULL and lastModified < (30 days ago)

Then Sync will do the following:

• Retrieve all items, counting those without a URI as deleted
• Purge flagged items on sync completion.

I would probably also have attempted part of Bug 708149, namely adding a couple of extra columns for Sync to reduce the impedance mismatch.

Thoughts on this?
Depends on: 708151
Comment on attachment 578564 [details] [diff] [review]
(2/2) Introduce new local bookmarks/history database

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

::: mobile/android/base/db/BrowserContract.java
@@ +94,5 @@
> +        public static final String IS_FOLDER = "folder";
> +
> +        public static final String PARENT = "parent";
> +
> +        public static final String POSITION = "position";

what is a bookmark position?

::: mobile/android/base/db/BrowserProvider.java
@@ +103,5 @@
> +
> +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;

why not just use URI_MATCHER directly?

@@ +104,5 @@
> +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;
> +        final String authority = BrowserContract.AUTHORITY;

why not use BrowserContract.AUTHORITY directly?

@@ +105,5 @@
> +
> +    static {
> +        final UriMatcher matcher = URI_MATCHER;
> +        final String authority = BrowserContract.AUTHORITY;
> +        HashMap<String, String> map;

why not use the *_PROJECTION_MAPs directly?
Attachment #578564 - Flags: review?(blassey.bugs) → review+
Depends on: 708331
Depends on: 708485
(In reply to Brad Lassey [:blassey] from comment #30)
> Comment on attachment 578564 [details] [diff] [review] [diff] [details] [review]
> (2/2) Introduce new local bookmarks/history database
> 
> Review of attachment 578564 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/BrowserContract.java
> @@ +94,5 @@
> > +        public static final String IS_FOLDER = "folder";
> > +
> > +        public static final String PARENT = "parent";
> > +
> > +        public static final String POSITION = "position";
> 
> what is a bookmark position?

Sync needs it.
 
> ::: mobile/android/base/db/BrowserProvider.java
> @@ +103,5 @@
> > +
> > +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> 
> why not just use URI_MATCHER directly?

I thought it was cleaner. No big deal. Changed it to use URI_MATCHER directly.

> @@ +104,5 @@
> > +    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> > +        final String authority = BrowserContract.AUTHORITY;
> 
> why not use BrowserContract.AUTHORITY directly?

Same reason. Fixed.
 
> @@ +105,5 @@
> > +
> > +    static {
> > +        final UriMatcher matcher = URI_MATCHER;
> > +        final String authority = BrowserContract.AUTHORITY;
> > +        HashMap<String, String> map;
> 
> why not use the *_PROJECTION_MAPs directly?

I'll keep this one as it makes the code more readable.
Needed changes to use getProfileDir() from GeckoApp.
Attachment #580029 - Flags: review?(blassey.bugs)
Pushed the first patch: http://hg.mozilla.org/integration/mozilla-inbound/rev/af13b9729453

Just waiting for review on the getProfileDir() patch to land the second.
Comment on attachment 580029 [details] [diff] [review]
Add API to get directory for any given profile

Looks good to me
Attachment #580029 - Flags: review?(blassey.bugs) → review+
Pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ebc46471b041
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb39bae574f8

Note that local DB is still disabled (as I proposed on comment 13). I'll enable it once we decide on how to expose this feature (Android DB vs Local DB) to users (bug 707150).
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 708651
Oops, shouldn't have resolved it just yet, sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Lucas Rocha (:lucasr) from comment #31)
> (In reply to Brad Lassey [:blassey] from comment #30)
> > Comment on attachment 578564 [details] [diff] [review]
> > (2/2) Introduce new local bookmarks/history database
> > 
> > Review of attachment 578564 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/base/db/BrowserContract.java
> > @@ +94,5 @@
> > > +        public static final String IS_FOLDER = "folder";
> > > +
> > > +        public static final String PARENT = "parent";
> > > +
> > > +        public static final String POSITION = "position";
> > 
> > what is a bookmark position?
> 
> Sync needs it.

what is a bookmark position? why does sync need it?
(In reply to Brad Lassey [:blassey] from comment #38)
> (In reply to Lucas Rocha (:lucasr) from comment #31)
> > (In reply to Brad Lassey [:blassey] from comment #30)

> > > what is a bookmark position?
> > 
> > Sync needs it.
> 
> what is a bookmark position? why does sync need it?

It's the position of the bookmark in the menu or toolbar where it is being shown. If we don't support it, like the other bits, then when the bookmarks are synced back to desktop, the order of you bookmarks will be screwed.
(In reply to Mark Finkle (:mfinkle) from comment #39)

> It's the position of the bookmark in the menu or toolbar where it is being
> shown. If we don't support it, like the other bits, then when the bookmarks
> are synced back to desktop, the order of you bookmarks will be screwed.

Over the past 18 months, a bookmarks engine bug that caused bookmarks to be reordered or moved between folders was the single biggest source of user anger at Sync that I can remember.

(Bug 621584.)

Apparently users really care about this!

Note that without any kind of positional information, Fennec's own bookmark display would be unpredictable after a sync operation: this is how you can tell Sync not to permute the contents of a container. We'll respect position as best we can.
https://hg.mozilla.org/mozilla-central/rev/bb39bae574f8
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Depends on: 709655
Blocks: 713623
tracking-fennec: --- → 11+
Depends on: 722184
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: