Closed Bug 941318 Opened 6 years ago Closed 6 years ago

Create content provider to store "My List" data

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

(Whiteboard: shovel-ready)

Attachments

(2 files, 5 obsolete files)

Bug 862805 has some discussion about this.

We want to be able to query this content provider from Java in order to display custom list data on about:home. We also need to be able to insert data into this content provider from JS. We discussed creating some sort of JS content provider wrapper to do this, but that should happen in a different bug.
Depends on: 941357
Assignee: nobody → margaret.leibovic
Attached patch WIP (obsolete) — Splinter Review
This is a pretty big mess, but I just wanted to get a proof of concept working. While working on this, I filed bug 941357, because there's way too much copy/paste going on in here.

But it works!

However, I did uncover a few issues with my patch from bug 862805. And I found a problem that we can't access mPageId from inside the cursor loader, not sure what the solution is to work around that static context issue...

Asking for feedback, but more as a way to let you know this patch exists. It still needs a lot of work :)
Attachment #8335771 - Flags: feedback?(wjohnston)
Attachment #8335771 - Flags: feedback?(rnewman)
Comment on attachment 8335771 [details] [diff] [review]
WIP

Won't we need to use PerProfileContentProvider as the base class, since this DB will need to be accessed by Java and Gecko?
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8335771 [details] [diff] [review]
> WIP
> 
> Won't we need to use PerProfileContentProvider as the base class, since this
> DB will need to be accessed by Java and Gecko?

Right now we access browser.db from Gecko, and that seems to be okay:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6870

It's not obvious, but BrowserProvider/TabsProvider (and now this new provider) *do* keep one DB per profile. AIUI, PerProfileContentProvider is used if we want to access the DB over the SQLiteBridge.

I filed bug bug 941357 to try to clean up some of this code, but yeah, we should do something to make it more clear what's going on.
(In reply to :Margaret Leibovic from comment #3)
> (In reply to Mark Finkle (:mfinkle) from comment #2)
> > Comment on attachment 8335771 [details] [diff] [review]
> > WIP
> > 
> > Won't we need to use PerProfileContentProvider as the base class, since this
> > DB will need to be accessed by Java and Gecko?
> 
> Right now we access browser.db from Gecko, and that seems to be okay:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#6870

Actually, this worries me too :)

> It's not obvious, but BrowserProvider/TabsProvider (and now this new
> provider) *do* keep one DB per profile. AIUI, PerProfileContentProvider is
> used if we want to access the DB over the SQLiteBridge.

Right, per profile is the norm now. It's the SQLiteBridge issue that I am more concerned about. We ran into problems before where Gecko and Java were accessing the same DB using two different SQLite engines.
Blocks: 942288
Comment on attachment 8335771 [details] [diff] [review]
WIP

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

::: mobile/android/base/db/CustomPageProvider.java
@@ +34,5 @@
> +    private static final String LOGTAG = "GeckoCustomPageProvider";
> +
> +    private Context mContext;
> +
> +    static final String DATABASE_NAME = "custompage.db";

rnewman made a good point today that we should strongly consider using browser.db to back this data, since that will make awesomescreen integration easier.
(In reply to :Margaret Leibovic from comment #5)

> > +    static final String DATABASE_NAME = "custompage.db";
> 
> rnewman made a good point today that we should strongly consider using
> browser.db to back this data, since that will make awesomescreen integration
> easier.

I'd like to hear more. I'd rather keep this custom data as far from browser.db as possible. I worry about corruption and DB locks and the effect of MOAR data on the time it takes to query for awesomebar results. I'd be happier if each provider was in it's own DB file.
(In reply to Mark Finkle (:mfinkle) from comment #6)

> I'd like to hear more. I'd rather keep this custom data as far from
> browser.db as possible. I worry about corruption and DB locks and the effect
> of MOAR data on the time it takes to query for awesomebar results. I'd be
> happier if each provider was in it's own DB file.

Positives for sharing the DB:

* One less open DB connection. They're not free.

* Integration with the Awesomebar means extending a query, rather than issuing queries to two databases and merging the cursors in memory. The latter is doable, but there's a fair amount of complexity hiding there.

* If either of these DBs screw up, the user is screwed, to a point. This is the old multi-engine plane fallacy: adding more engines *increases* the likelihood of an individual failure, but reduces the chances of both dying at the same time. The UX here is impacted if *either* dies.

Downsides, of course, are perhaps an increased chance of DB page fragmentation, larger individual DB size, and increased consequences if we have to blow away one failed DB.
(In reply to Richard Newman [:rnewman] from comment #7)

> * Integration with the Awesomebar means extending a query, rather than
> issuing queries to two databases and merging the cursors in memory. The
> latter is doable, but there's a fair amount of complexity hiding there.

I think we need to be very careful with dreams of Awesomebar search integration. Just because it's possible, doesn't mean we should do it. Everything in moderation. I worry we could ruin the value of the Awesomebar search by throwing too many datasources into the query.
Comment on attachment 8335771 [details] [diff] [review]
WIP

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

Some general thoughts. I can probably pull out more as this goes along if you still want thoughts.

::: mobile/android/base/db/BrowserContract.java
@@ +275,5 @@
>          public static final String LAST_MODIFIED = "last_modified";
>      }
> +
> +    // Custom page lists
> +    public static final class ListData implements CommonColumns, URLColumns {

Naming nit: "ListData" feels really vague. Similarly LIST_ID

::: mobile/android/base/db/CustomPageProvider.java
@@ +34,5 @@
> +    private static final String LOGTAG = "GeckoCustomPageProvider";
> +
> +    private Context mContext;
> +
> +    static final String DATABASE_NAME = "custompage.db";

I think in general we'll need the awesomescreen to draw from several data sources, so I'm not sure this is worth doing.

@@ +73,5 @@
> +            Log.d(LOGTAG, message);
> +        }
> +    }
> +
> +    final class DatabaseHelper extends SQLiteOpenHelper {

The BrowserDB version of this is so ugly, maybe we pull this out into its own class already so that it doesn't clutter things up.

@@ +87,5 @@
> +            db.execSQL("CREATE TABLE " + TABLE_LIST_DATA + "(" +
> +                    ListData._ID + " INTEGER PRIMARY KEY AUTOINCREMENT," +
> +                    ListData.LIST_ID + " INTEGER," +
> +                    ListData.TITLE + " TEXT," +
> +                    ListData.URL + " TEXT)"

Like I said before, I wonder if we want to make this more general, and let the add-on handle loading these when they're tapped... i.e. URL is very specific. From the mockups we've seen, I assume we'd want at least a "description" column and an "image" column. I also wonder if we'd want to leave a general "annotation" one so that we have some expansion ability for things that we'd never want to sort/filter search on....

@@ +110,5 @@
> +            // Unimplemented
> +        }
> +    }
> +
> +    private DatabaseHelper getDatabaseHelperForProfile(String profile) {

:( yeah, this duplicated logic makes me sad.

::: mobile/android/base/home/CustomListPage.java
@@ +129,5 @@
>          public Cursor loadCursor() {
> +            final ContentResolver cr = getContext().getContentResolver();
> +
> +            // XXX: Make this profile generic
> +            final Uri uri = ListData.CONTENT_URI.buildUpon().appendQueryParameter(BrowserContract.PARAM_PROFILE, "default").build();

We should move this to DBUtils. Maybe a lot of this can go there...

@@ +134,5 @@
> +
> +            final String[] projection = new String[] {
> +                ListData._ID,
> +                ListData.URL,
> +                ListData.TITLE

I wonder if its worth caching some of these really generic projection stuff. i.e. this seems really similar to LIST_COLUMNS and I wish we could somehow sew them together.
Attachment #8335771 - Flags: feedback?(wjohnston) → feedback+
Attached patch WIP v2 (obsolete) — Splinter Review
This build on top of the work for bug 941357, and it limits the scope of this patch to just creating the content provider, not trying to use it in any UI.

We can continue to have a discussion about whether or not we want to put this data in browser.db or keep it in a separate db, but right now I'm leaning towards the latter, mainly to avoid needing to touch our browser.db schema as we iterate on this feature.

We should also discuss what schema we want for this feature, including what to name the table where we store this data. In this patch I just named it `lists`, but that doesn't feel semantically correct because it doesn't hold lists, but rather list items (that's why I included a LIST_ID column, that should refer back to the list that holds the given item).

Anyway, this seems a bit cleaner than the previous patch. I took wesj's advice to move the DatabaseHelper out into its own file. I'd like to do that for TabsProvider/BrowserProvider in the future, but that's tangential to this work.
Attachment #8335771 - Attachment is obsolete: true
Attachment #8335771 - Flags: feedback?(rnewman)
Note: When thinking about the schema we want to use, we should think about the different kinds of list displays we want to support. Check out bug 942887 for an example of an "article layout", which includes short description text in addition to titles, as well as larger images.

I imagine we'll make separate table for images, but we'll need some sort of key on the list items to indicate which image they want to use for their display.
Consider that not only do we need to support different kind of views, but also searching/filtering/etc. in the Awesomebar and elsewhere.

I can think of:

Image types -- 1:N, typed:
  Icon (e.g., kind)
  Thumbnail (small, square image)
  Banner (wide, larger image)
  Lead (full-size image)

Text:
  Title
  Author
  Summary (~one-liner, can be truncated)
  Extended ("above the fold")
  Full content ("below the fold", for full display)
  URL/permalink
  # of comments?

Metadata:
  Provider ID             }
  Item ID (textual)       } unique pair
  First added
  Last updated
  Last viewed
  # of views?
  # of clickthroughs? (e.g., for awesomebar weighting -- equivalent to visits)
  Starred/pinned/favorited/shared flags

At minimum we should be able to replicate something like Feedly's views, and store Atom content from the web.
Whiteboard: shovel-ready
(In reply to Richard Newman [:rnewman] from comment #12)

>   Full content ("below the fold", for full display)

This is starting to get into implementing a full-blown reading list... which is interesting. We haven't fully specified what happens when a user clicks on a list item. For v1, I'd support just opening a URL, but for a future version we could open this content in reader mode. Definitely not in the scope of this bug :)

> Metadata:
>   Provider ID             }
>   Item ID (textual)       } unique pair
>   First added
>   Last updated
>   Last viewed
>   # of views?
>   # of clickthroughs? (e.g., for awesomebar weighting -- equivalent to
> visits)
>   Starred/pinned/favorited/shared flags

What about how we'll sort the list in the view? Should we have a position column as well? For things like RSS feeds it would make sense to sort with newest items on top, but other providers may want to have a fixed structure (e.g. synced bookmarks).
(In reply to :Margaret Leibovic from comment #13)

> This is starting to get into implementing a full-blown reading list... which
> is interesting. We haven't fully specified what happens when a user clicks
> on a list item. For v1, I'd support just opening a URL, but for a future
> version we could open this content in reader mode. Definitely not in the
> scope of this bug :)

Bear in mind that if we get the schema close to right now, the migration will be less painful later. No harm in adding the columns and tables now.

> What about how we'll sort the list in the view? Should we have a position
> column as well? For things like RSS feeds it would make sense to sort with
> newest items on top, but other providers may want to have a fixed structure
> (e.g. synced bookmarks).

Makes sense. Sync calls this "sortindex".

Also need a version number.
Depends on: 949039
Attached patch listscontentprovider.diff (obsolete) — Splinter Review
I kinda scope-creeped this to include the beginnings of bug 949039 because we need a content provider to exist before we can do that bug, but it would be nice to have that test endpoint to help us develop the schema we want.

I'd love to land something that we can iterate on, but I don't want to deal with db migrations, so maybe we can just land this with this test endpoint, but disable the database bit?
Attachment #8344111 - Attachment is obsolete: true
Attachment #8346291 - Flags: feedback?(wjohnston)
Attachment #8346291 - Flags: feedback?(rnewman)
Attached patch WIP v3 (obsolete) — Splinter Review
Oops, uploaded an older patch.
Attachment #8346291 - Attachment is obsolete: true
Attachment #8346291 - Flags: feedback?(wjohnston)
Attachment #8346291 - Flags: feedback?(rnewman)
Attachment #8346292 - Flags: feedback?(wjohnston)
Attachment #8346292 - Flags: feedback?(rnewman)
Attached patch test WIP (obsolete) — Splinter Review
I made a test to test the test data. Yo dawg.

(This also includes a test for inserting real items, but I could disable that if we land this with the db part disabled.)
Attachment #8346295 - Flags: feedback?(wjohnston)
Attachment #8346295 - Flags: feedback?(rnewman)
Also, this test is based on top of the patch in bug 949208.
Comment on attachment 8346292 [details] [diff] [review]
WIP v3

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

I think this seems fine. Just lots of duplicated logic that probably differs just enough to be annoying.

::: mobile/android/base/db/BrowserContract.java
@@ +24,5 @@
>      public static final String TABS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.tabs";
>      public static final Uri TABS_AUTHORITY_URI = Uri.parse("content://" + TABS_AUTHORITY);
>  
> +    public static final String LISTS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.lists";
> +    public static final Uri LISTS_AUTHORITY_URI = Uri.parse("content://" + LISTS_AUTHORITY);

Can we just put this in the ListProvider itself. Having to look across several files to find stuff drives me nuts... I also kinda wonder if we can (someday) use the contracts to autogenerate most of the table creation stuff.

::: mobile/android/base/db/ListsProvider.java
@@ +23,5 @@
> +import android.os.Build;
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public class ListsProvider extends ContentProvider {

'Lists' feels vague. I know you've been looking at the naming here though. Especially since its a Java generic class. Maybe 'HomePagerLists' would be clearer.

@@ +36,5 @@
> +
> +    static final int ITEMS = 101;
> +    static final int ITEMS_ID = 102;
> +
> +    final String[] ITEMS_COLUMNS = new String[] {

Just COLUMNS

@@ +50,5 @@
> +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items/test", ITEMS_TEST);
> +
> +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items", ITEMS);
> +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items/#", ITEMS_ID);
> +    }

Oh it looks like most of this has moved!

@@ +78,5 @@
> +        String profile = null;
> +        if (uri != null) {
> +            profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE);
> +        }
> +        return mDatabases.getDatabaseHelperForProfile(profile, isTest(uri)).getReadableDatabase();

I wonder if we can rename this to just getForProfile(profile, PerProfileDatabases.READ_ONLY). Is there a good reason to expose the helper outside?

@@ +138,5 @@
> +        int deleted = 0;
> +
> +        if (Build.VERSION.SDK_INT >= 11) {
> +            trace("Beginning delete transaction: " + uri);
> +            db.beginTransaction();

I'm sure this is not your code at all, but why is this v11+?

@@ +215,5 @@
> +        return result;
> +    }
> +
> +    @Override
> +    public int bulkInsert(Uri uri, ContentValues[] values) {

I hate that all these methods look exactly the same. I'm not sure how to fix that though. I wonder if we could do something generic like:

public int insert(Uri uri, ContentValues[] values) {
  inTransaction(new TransactionHelper<Integer>() {
    @Override
    public Integer doWork(SqliteDatabase db) {
      return db.insertInTransaction();
    }    
  });
}

or something similar for the insert/delete/updateInTransaction methods.
Attachment #8346292 - Flags: feedback?(wjohnston) → feedback+
(In reply to Wesley Johnston (:wesj) from comment #19)
 
> I think this seems fine. Just lots of duplicated logic that probably differs
> just enough to be annoying.

Yeah... to a certain extent it's basic ContentProvider boilerplate, but we probably could make a new abstract class that handles that, then have sub-classes implement the transaction body methods. It's a hairy yak.

> ::: mobile/android/base/db/BrowserContract.java
> @@ +24,5 @@
> >      public static final String TABS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.tabs";
> >      public static final Uri TABS_AUTHORITY_URI = Uri.parse("content://" + TABS_AUTHORITY);
> >  
> > +    public static final String LISTS_AUTHORITY = AppConstants.ANDROID_PACKAGE_NAME + ".db.lists";
> > +    public static final Uri LISTS_AUTHORITY_URI = Uri.parse("content://" + LISTS_AUTHORITY);
> 
> Can we just put this in the ListProvider itself. Having to look across
> several files to find stuff drives me nuts... I also kinda wonder if we can
> (someday) use the contracts to autogenerate most of the table creation stuff.

I was following the exiting patterns we've been using. I agree following all of these classes is annoying, but I think we put these things in BrowserContract so that sync can know about them without knowing about the content provider internals.

> ::: mobile/android/base/db/ListsProvider.java
> @@ +23,5 @@
> > +import android.os.Build;
> > +import android.text.TextUtils;
> > +import android.util.Log;
> > +
> > +public class ListsProvider extends ContentProvider {
> 
> 'Lists' feels vague. I know you've been looking at the naming here though.
> Especially since its a Java generic class. Maybe 'HomePagerLists' would be
> clearer.

Yeah, or maybe just HomeLists? Will these lists ever end up in a different part of our UI? I think for now it's safe to assume they're just in about:home (I don't want to mix in the "page" term as well, though, since that's a different concept).

> @@ +36,5 @@
> > +
> > +    static final int ITEMS = 101;
> > +    static final int ITEMS_ID = 102;
> > +
> > +    final String[] ITEMS_COLUMNS = new String[] {
> 
> Just COLUMNS

Well, these are columns for the list_items table. Right now this lists.db just has one table (list_items), but it will grow to have other tables for things like images.

> @@ +50,5 @@
> > +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items/test", ITEMS_TEST);
> > +
> > +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items", ITEMS);
> > +        URI_MATCHER.addURI(BrowserContract.LISTS_AUTHORITY, "items/#", ITEMS_ID);
> > +    }
> 
> Oh it looks like most of this has moved!

?

> @@ +78,5 @@
> > +        String profile = null;
> > +        if (uri != null) {
> > +            profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE);
> > +        }
> > +        return mDatabases.getDatabaseHelperForProfile(profile, isTest(uri)).getReadableDatabase();
> 
> I wonder if we can rename this to just getForProfile(profile,
> PerProfileDatabases.READ_ONLY). Is there a good reason to expose the helper
> outside?

Nope. In bug 941357, I kept the basic method signatures in place, but moved duplicated things into PerProfileDatabases. Feel free to file a follow-up to continue to improve this logic :)

> @@ +138,5 @@
> > +        int deleted = 0;
> > +
> > +        if (Build.VERSION.SDK_INT >= 11) {
> > +            trace("Beginning delete transaction: " + uri);
> > +            db.beginTransaction();
> 
> I'm sure this is not your code at all, but why is this v11+?

You're right, I just copied this. My guess is that these transaction APIs didn't exist before v11. Although the docs tell me they existed in API level 1...

http://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#beginTransaction%28%29

We can file another bug to investigate this.

> @@ +215,5 @@
> > +        return result;
> > +    }
> > +
> > +    @Override
> > +    public int bulkInsert(Uri uri, ContentValues[] values) {
> 
> I hate that all these methods look exactly the same. I'm not sure how to fix
> that though. I wonder if we could do something generic like:
> 
> public int insert(Uri uri, ContentValues[] values) {
>   inTransaction(new TransactionHelper<Integer>() {
>     @Override
>     public Integer doWork(SqliteDatabase db) {
>       return db.insertInTransaction();
>     }    
>   });
> }
> 
> or something similar for the insert/delete/updateInTransaction methods.

Yeah... but I think they may be slightly different enough that this won't work. Looking at the bigger picture, this same issue exists in BrowserProvider and TabsProvider, which is why I suggested an abstract class up above.
> > I'm sure this is not your code at all, but why is this v11+?
> 
> You're right, I just copied this. My guess is that these transaction APIs
> didn't exist before v11. Although the docs tell me they existed in API level
> 1...
> 
> http://developer.android.com/reference/android/database/sqlite/
> SQLiteDatabase.html#beginTransaction%28%29
> 
> We can file another bug to investigate this.

One thing that might explain this (if not being the entire cause): 11+ is where we turn off Android's DB locking, which allows us to run DB operations in parallel. (Android decided that was a bad idea in API 16, and made half of what we do a no-op.)

Note that transaction handling did change in API 11: it added beginTransactionNonExclusive, for example. I'd be leery of changing this pattern.

Also see my investigation in Bug 947939.
Attached patch MVPatchSplinter Review
Here's something that I think we can land. I added a flag to disable DB usage, so for now we won't ever create (and later need to deal with upgrading) a DB, but we can flip this flag locally for development.

I renamed the "test" data endpoint to "fake", since "test" can incorrectly imply that this endpoint is used for robocop tests, when really it's just used to return fake data for development.

The queryFakeItems method in here is pretty dumb right now, but I think we can iterate on this in a follow-up (we can re-purpose bug 949039 for that).
Attachment #8346292 - Attachment is obsolete: true
Attachment #8346292 - Flags: feedback?(rnewman)
Attachment #8349564 - Flags: review?(wjohnston)
Attached patch test for MVPatchSplinter Review
I disabled the bits of the test that deal with the database, but we can re-enable them locally to help us develop the DB side of things.

TestFakeItems depends on the current set of fake items returned by the fake endpoint, but we can improve this as we improve the endpoint.

Here's a try push:
https://tbpl.mozilla.org/?tree=Try&rev=72045bbbaee2
Attachment #8346295 - Attachment is obsolete: true
Attachment #8346295 - Flags: feedback?(wjohnston)
Attachment #8346295 - Flags: feedback?(rnewman)
Attachment #8349570 - Flags: review?(wjohnston)
Comment on attachment 8349564 [details] [diff] [review]
MVPatch

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

LGTM, except for this null db issue.

::: mobile/android/base/db/HomeListsProvider.java
@@ +135,5 @@
> +    public int delete(Uri uri, String selection, String[] selectionArgs) {
> +        trace("Calling delete on URI: " + uri);
> +
> +        final SQLiteDatabase db = getWritableDatabase(uri);
> +        int deleted = 0;

Isn't this going to return null if DB_DISABLED? Does that kill things below when you call db.beginTransaction?
Attachment #8349564 - Flags: review?(wjohnston) → review+
Comment on attachment 8349570 [details] [diff] [review]
test for MVPatch

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

::: mobile/android/base/tests/testHomeListsProvider.java
@@ +21,5 @@
> +    }
> +
> +    private void loadContractInfo() throws Exception {
> +        mItemsFakeUri = getUriColumn("HomeListItems", "CONTENT_FAKE_URI");
> +        mItemsUri = getContentUri("HomeListItems");

We'll probably need to append a list id onto here, right?

@@ +26,5 @@
> +
> +        mItemsIdCol = getStringColumn("HomeListItems", "_ID");
> +        mItemsProviderIdCol = getStringColumn("HomeListItems", "PROVIDER_ID");
> +        mItemsTitleCol = getStringColumn("HomeListItems", "TITLE");
> +        mItemsUrlCol = getStringColumn("HomeListItems", "URL");

I thought we removed the need for all this reflection? Can we just use org.mozilla.gecko.db.BrowserContract stuff here?
Attachment #8349570 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #24)
> Comment on attachment 8349564 [details] [diff] [review]
> MVPatch
> 
> Review of attachment 8349564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, except for this null db issue.
> 
> ::: mobile/android/base/db/HomeListsProvider.java
> @@ +135,5 @@
> > +    public int delete(Uri uri, String selection, String[] selectionArgs) {
> > +        trace("Calling delete on URI: " + uri);
> > +
> > +        final SQLiteDatabase db = getWritableDatabase(uri);
> > +        int deleted = 0;
> 
> Isn't this going to return null if DB_DISABLED? Does that kill things below
> when you call db.beginTransaction?

Yeah... I kinda punted on that, since there won't be any consumers of these DB endpoints in the tree. I guess I could throw a more informative exception if you're trying to run a query against a DB that doesn't exist.
(In reply to Wesley Johnston (:wesj) from comment #25)
> Comment on attachment 8349570 [details] [diff] [review]
> test for MVPatch
> 
> Review of attachment 8349570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testHomeListsProvider.java
> @@ +21,5 @@
> > +    }
> > +
> > +    private void loadContractInfo() throws Exception {
> > +        mItemsFakeUri = getUriColumn("HomeListItems", "CONTENT_FAKE_URI");
> > +        mItemsUri = getContentUri("HomeListItems");
> 
> We'll probably need to append a list id onto here, right?

Oh, this brings up a point about how we want to design these URI endpoints. Right now, the /items URI just returns all items, and then I assume we'll use a selection to filter for the correct provider id. The /items/# URI will return a single item with a given id (the item id, not the provider id).

We'd need to make another URI if we want to do something like /items/<provider_id>, but I'm not sure how to do that URI matching, or if this is actually something we should do.

> @@ +26,5 @@
> > +
> > +        mItemsIdCol = getStringColumn("HomeListItems", "_ID");
> > +        mItemsProviderIdCol = getStringColumn("HomeListItems", "PROVIDER_ID");
> > +        mItemsTitleCol = getStringColumn("HomeListItems", "TITLE");
> > +        mItemsUrlCol = getStringColumn("HomeListItems", "URL");
> 
> I thought we removed the need for all this reflection? Can we just use
> org.mozilla.gecko.db.BrowserContract stuff here?

Copypasta! I can try this without the reflection.
Blocks: 949039
No longer depends on: 949039
Blocks: 952310
(In reply to :Margaret Leibovic (away Dec 20 - Jan 6) from comment #27)

> We'd need to make another URI if we want to do something like
> /items/<provider_id>, but I'm not sure how to do that URI matching, or if
> this is actually something we should do.

Two URI matchers, if you really need to address items independently of providers, or one if you don't:

/providers/*/items/#
/items/#

I'd imagine that the former is sufficient -- items should be scoped to a provider (or a list).
You need to log in before you can comment on or make changes to this bug.