961238 - Make an abstract ContentProvider class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Sola Ogunsakin [:sola]]

Make a common parent class that implements the basic ContentProvider methods. Subclasses implement the "*InTransaction" methods for inserts/updates/deletes. https://bugzilla.mozilla.org/show_bug.cgi?id=959290#c5

Comment 1

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Comment 2

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8362558 [details] [diff] [review]
bug-961238-fix.patch

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

I really like it. Maybe SQLiteContentProvider could hold a common implementation of getReadableDatabase() & getWritableDatabase() (instead of abstract methods) given that they're pretty much the same in BrowserProvider and, say, HomeListsProvider?

I'd simplify the names a bit:
SQLiteContentProvider -> SQLiteProvider
TransactionalContentProvider -> TransactionalProvider

The 'Content' in the class names is a bit redundant given the context they're being used here.

::: mobile/android/base/db/TransactionalContentProvider.java
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.db;
> +
> +import org.mozilla.gecko.mozglue.RobocopTarget;

Unused?

Comment 4

[:Margaret Leibovic]

Comment on attachment 8362558 [details] [diff] [review]
bug-961238-fix.patch

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

Looking good! I've wanted us to do this for a while :)

I agree with lucasr's point about simplifying the naming.

You should also push a patch to try server once you have a version ready for review.

::: mobile/android/base/db/BrowserProvider.java
@@ +1952,5 @@
>                  c.close();
>          }
>      }
>  
> +    protected SQLiteDatabase getReadableDatabase(Uri uri) {

You should add @Override tags where you're implementing abstract methods.

@@ +2249,1 @@
>      public Uri insertInTransaction(Uri uri, ContentValues values) {

Add @Override tags to these new abstract methods you created.

::: mobile/android/base/db/SQLiteContentProvider.java
@@ +10,5 @@
> +import android.database.sqlite.SQLiteDatabase;
> +import android.net.Uri;
> +import android.util.Log;
> +
> +public abstract class SQLiteContentProvider extends ContentProvider {

Are we ever going to have a SQLiteContentProvider that's not a TransactionalContentProvider? If we don't currently plan to do that anywhere, I'd just leave these methods as part of TransactionalContentProvider, rather than add another layer of abstraction.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8362558 [details] [diff] [review]
bug-961238-fix.patch

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

::: mobile/android/base/db/TransactionalContentProvider.java
@@ +15,5 @@
> +public abstract class TransactionalContentProvider extends SQLiteContentProvider {
> +
> +    abstract public int updateInTransaction(Uri uri, ContentValues values, String selection, String[] selectionArgs);
> +    abstract public Uri insertInTransaction(Uri uri, ContentValues values);
> +    abstract public int deleteInTransaction(Uri uri, String selection, String[] selectionArgs);

Also, these methods don't need to be public, right?

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8362558 [details] [diff] [review]
bug-961238-fix.patch

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

Nice, good idea to do some refactoring.

::: mobile/android/base/db/BrowserProvider.java
@@ +1963,5 @@
>  
>          return mDatabases.getDatabaseHelperForProfile(profile, isTest(uri)).getReadableDatabase();
>      }
>  
> +    protected SQLiteDatabase getWritableDatabase(Uri uri) {

@Override tag

@@ -2172,5 @@
> -            db.beginTransaction();
> -            try {
> -                deleted = deleteInTransaction(db, uri, selection, selectionArgs);
> -                db.setTransactionSuccessful();
> -                trace("Successful delete transaction: " + uri);

Probably keep these trace statements in after you've moved the code, they seem useful.

::: mobile/android/base/db/SQLiteContentProvider.java
@@ +14,5 @@
> +public abstract class SQLiteContentProvider extends ContentProvider {
> +
> +    abstract protected SQLiteDatabase getReadableDatabase(Uri uri);
> +    abstract protected SQLiteDatabase getWritableDatabase(Uri uri);
> +

Nit: Unnecessary newline

Comment 7

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Changes:
-renamed TransactionalContentProvider to TransactionalProvider
-consolidated SQLiteContentProvider into TransactionalProvider
-moved getReadableDatabase() and getWritableDatabase() methods into TransactionalProvider
-moved BrowserDatabaseHelper out of BrowserProvider

Try run looks good so far https://tbpl.mozilla.org/?tree=Try&rev=825ca4c7ba32

Comment 8

[:Margaret Leibovic]

Comment on attachment 8368870 [details] [diff] [review]
bug-961238-fix.patch

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

rnewman might also want to look at this.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8368870 [details] [diff] [review]
bug-961238-fix.patch

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

I used opendiff to verify that the new BrowserDatabaseProvider matches the old BrowserDatabaseProvider, modulo a few necessary (and intentional) changes.

I really like this patch, I just want to see a new version that addresses my comments before giving an r+. It would also be good if lucasr or rnewman get the chance to look this over.

Also, you should file a follow-up to update TabsProvider to use TransactionalProvider.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +52,5 @@
> +    private static final String LOGTAG = "GeckoBrowserDBHelper";
> +    public static final int DATABASE_VERSION = 17;
> +
> +    protected Context mContext;
> +    protected BrowserProvider mProvider;

Nit: add an empty line below this line.

@@ +70,5 @@
> +    static final String TABLE_BOOKMARKS_TMP = BrowserProvider.TABLE_BOOKMARKS_TMP;
> +    static final String TABLE_HISTORY_TMP = BrowserProvider.TABLE_HISTORY_TMP;
> +    static final String TABLE_IMAGES_TMP = BrowserProvider.TABLE_IMAGES_TMP;
> +
> +    public BrowserDatabaseHelper(Context context, String databasePath, BrowserProvider provider) {

I don't love this two-way dependency between BrowserDatabseHelper and BrowserProvider, but I also think we should avoid changing logic around in this refactoring patch. If anything, this just exposes an existing dependency. From the current BrowserProvider code, it's pretty clear we didn't do a good job specifying what exactly BrowserDatabaseHelper should and shouldn't do.

Maybe we can clean some of this up in the future with bug 947018.

::: mobile/android/base/db/BrowserProvider.java
@@ +49,2 @@
>  
>      static final String DATABASE_NAME = "browser.db";

I think DATABASE_VERSION belongs in BroweserDatabaseHelper with DATABASE_VERSION.

@@ -1929,5 @@
> -                }
> -            }
> -        }
> -    }
> -

I must admit, it is really nice to see this giant hunk removed from BrowserProvider.

@@ +326,4 @@
>      private static final String[] mobileIdColumns = new String[] { Bookmarks._ID };
>      private static final String[] mobileIdSelectionArgs = new String[] { Bookmarks.MOBILE_FOLDER_GUID };
>  
> +    public Integer getMobileFolderId(SQLiteDatabase db) {

This method is actually only used in BrowserDatabseHelper, so you could move it to be a method in there.

@@ +1540,5 @@
>  
>          return results;
>      }
>  
> +	@Override

Tabs! Make sure you have your editor to set to use spaces, not tabs.

::: mobile/android/base/db/TransactionalProvider.java
@@ +19,5 @@
> +import android.util.Log;
> +
> +
> +public abstract class TransactionalProvider<T extends SQLiteOpenHelper> extends ContentProvider {
> +	private static final String LOGTAG = "GeckoTransProvider";

More tabs! Also, is "GeckoTransactionalProvider" too long for a log tag? If not, that would be clearer.

@@ +48,5 @@
> +
> +    protected SQLiteDatabase getReadableDatabase(Uri uri) {
> +        String profile = null;
> +        if (uri != null)
> +            profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE);

Nit: Brace single-line if statements.

Comment 10

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #9)

> I don't love this two-way dependency between BrowserDatabseHelper and
> BrowserProvider, but I also think we should avoid changing logic around in
> this refactoring patch. If anything, this just exposes an existing
> dependency.

Might be time to introduce an interface for the methods the helper calls on the provider? BrowserDatabaseDelegate?

> More tabs! Also, is "GeckoTransactionalProvider" too long for a log tag? If
> not, that would be clearer.

26 chars. Yes, too long. 23 to play with.

Comment 11

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #10)
> (In reply to :Margaret Leibovic from comment #9)
> 
> > I don't love this two-way dependency between BrowserDatabseHelper and
> > BrowserProvider, but I also think we should avoid changing logic around in
> > this refactoring patch. If anything, this just exposes an existing
> > dependency.
> 
> Might be time to introduce an interface for the methods the helper calls on
> the provider? BrowserDatabaseDelegate?
> 

looking through BrowserDatabaseHelper mProvider is only used once, to insert a favicon ( mProvider.insertFavicon(SQLiteDatabase, ContentValues) ). It might be worth duplicating that utility function for now to prevent this ugly dependency. Or we could make that method static and reference it as such for now. Dependence on a BrowserProvider instance would make it hard to implement bug 959290.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8368870 [details] [diff] [review]
bug-961238-fix.patch

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

I have a sneaking suspicion that some of BrowserProvider's imports can be cleaned up, unless you used an IDE to do this refactor.

Overall comments to think about (you'll get used to my Socratic reviewing style soon enough!):

* I'd like to see BDH partitioned very clearly into the SQLiteOpenHelper interface part and the internal methods part.

* Every point where BrowserProvider is referenced from BrowserDatabaseHelper requires a little consideration (or even justification), and vice versa.

* I understand the broad thrust of why this approach is being taken (to reuse TransactionalProvider), but I can't help but observe that all we're doing is making the abstract ContentProvider do very little, putting all of the logic into the SQLiteOpenHelper.

That's a step in the right direction -- composition over inheritance, and we're scratching some reuse out of a very complex class -- but it would be worth thinking more deeply about how we might get more value out of this by straightening up some more of the mess. Questions to consider: how could we make a BookmarksProvider and a HistoryProvider? Can we separate database upgrades from querying? Is there a better way to describe and manipulate schemas? What of the SQLiteOpenHelper itself is reusable?

* Is it safe to pass `new SQLiteOpenHelper()` to TransactionalProvider? (Phrased differently: what assumptions does PerProfileDatabases<T> make about the implementations of T?) If not, why not? Capture that reasoning in an interface or a subclass.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +46,5 @@
> +import java.util.regex.Matcher;
> +import java.util.regex.Pattern;
> +
> +
> +final class BrowserDatabaseHelper extends SQLiteOpenHelper {

Why final? Why package-scoped?

@@ +51,5 @@
> +
> +    private static final String LOGTAG = "GeckoBrowserDBHelper";
> +    public static final int DATABASE_VERSION = 17;
> +
> +    protected Context mContext;

This can be final.

@@ +56,5 @@
> +    protected BrowserProvider mProvider;
> +    static final String TABLE_BOOKMARKS = BrowserProvider.TABLE_BOOKMARKS;
> +    static final String TABLE_HISTORY = BrowserProvider.TABLE_HISTORY;
> +    static final String TABLE_FAVICONS = BrowserProvider.TABLE_FAVICONS;
> +    static final String TABLE_THUMBNAILS = BrowserProvider.TABLE_THUMBNAILS;

Seems like these should be direct references to a contract interface, or BrowserProvider should refer to these!

@@ +62,5 @@
> +    static final String VIEW_COMBINED = BrowserProvider.VIEW_COMBINED;
> +
> +    static final String VIEW_BOOKMARKS_WITH_FAVICONS = BrowserProvider.VIEW_BOOKMARKS_WITH_FAVICONS;
> +    static final String VIEW_HISTORY_WITH_FAVICONS = BrowserProvider.VIEW_HISTORY_WITH_FAVICONS;
> +    static final String VIEW_COMBINED_WITH_FAVICONS = BrowserProvider.VIEW_COMBINED_WITH_FAVICONS;

Is this combined view still used elsewhere? I thought we killed it.

@@ +854,5 @@
> +        Class<?> stringsClass = R.string.class;
> +        Field[] fields = stringsClass.getFields();
> +        Pattern p = Pattern.compile("^bookmarkdefaults_title_");
> +
> +        Integer mobileFolderId = mProvider.getMobileFolderId(db);

Perhaps pass the mobile folder ID into the Helper constructor?

@@ +927,5 @@
> +            Log.w(LOGTAG, "Favicon compression failed.");
> +        }
> +        iconValues.put(Favicons.DATA, data);
> +
> +        mProvider.insertFavicon(db, iconValues);

I'd probably inline this method, yeah. You're already taking a dependency on the structure of the ContentValues.

@@ +1656,5 @@
> +        }
> +    }
> +
> +    static final String qualifyColumn(String table, String column) {
> +        return BrowserProvider.qualifyColumn(table, column);

Fix this partial refactor.

::: mobile/android/base/db/TransactionalProvider.java
@@ +19,5 @@
> +import android.util.Log;
> +
> +
> +public abstract class TransactionalProvider<T extends SQLiteOpenHelper> extends ContentProvider {
> +	private static final String LOGTAG = "GeckoTransProvider";

Probably the log tag should come from the helper.

@@ +33,5 @@
> +    @Override
> +    public boolean onCreate() {
> +        synchronized (this) {
> +        	mContext = getContext();
> +            mDatabases = new PerProfileDatabases<T>(

Holy indentation, Batman.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 8368870 [details] [diff] [review]
bug-961238-fix.patch

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

It seems margaret and rnewman have already given enough feedback on this patch :-)

Comment 14

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Got rid of BrowserDatabaseHelper dependency on BrowserProvider.

Comment 15

[Sola Ogunsakin [:sola]]

(In reply to Richard Newman [:rnewman] from comment #12)
> Comment on attachment 8368870 [details] [diff] [review]
> bug-961238-fix.patch
> 
> Review of attachment 8368870 [details] [diff] [review]:
> -----------------------------------------------------------------
> * I'd like to see BDH partitioned very clearly into the SQLiteOpenHelper
> interface part and the internal methods part.

placed the overridden methods at the top and private methods below.
 
> * Every point where BrowserProvider is referenced from BrowserDatabaseHelper
> requires a little consideration (or even justification), and vice versa.

i've removed all uses of BrowserProvider from BrowserDatabaseHelper

> * I understand the broad thrust of why this approach is being taken (to
> reuse TransactionalProvider), but I can't help but observe that all we're
> doing is making the abstract ContentProvider do very little, putting all of
> the logic into the SQLiteOpenHelper.

the DB helper is only currently performing migrations and upgrades.

> That's a step in the right direction -- composition over inheritance, and
> we're scratching some reuse out of a very complex class -- but it would be
> worth thinking more deeply about how we might get more value out of this by
> straightening up some more of the mess. Questions to consider: how could we
> make a BookmarksProvider and a HistoryProvider? Can we separate database
> upgrades from querying? Is there a better way to describe and manipulate
> schemas? What of the SQLiteOpenHelper itself is reusable?

currently all of the shared methods needed to create a BookmarksProvider, HistoryProvider and ReadingListProvider are in TransactionalProvider or in the utility class DBUtils.

> * Is it safe to pass `new SQLiteOpenHelper()` to TransactionalProvider?
> (Phrased differently: what assumptions does PerProfileDatabases<T> make
> about the implementations of T?) If not, why not? Capture that reasoning in
> an interface or a subclass.

PerProfileDatabases only assumes that T is subclass of a SQLiteOpenHelper.

Comment 16

[Sola Ogunsakin [:sola]]

Comment on attachment 8368870 [details] [diff] [review]
bug-961238-fix.patch

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

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +46,5 @@
> +import java.util.regex.Matcher;
> +import java.util.regex.Pattern;
> +
> +
> +final class BrowserDatabaseHelper extends SQLiteOpenHelper {

from my understanding there should be one DBH per database.

@@ +62,5 @@
> +    static final String VIEW_COMBINED = BrowserProvider.VIEW_COMBINED;
> +
> +    static final String VIEW_BOOKMARKS_WITH_FAVICONS = BrowserProvider.VIEW_BOOKMARKS_WITH_FAVICONS;
> +    static final String VIEW_HISTORY_WITH_FAVICONS = BrowserProvider.VIEW_HISTORY_WITH_FAVICONS;
> +    static final String VIEW_COMBINED_WITH_FAVICONS = BrowserProvider.VIEW_COMBINED_WITH_FAVICONS;

being used by createCombinedViewOn13()

@@ +70,5 @@
> +    static final String TABLE_BOOKMARKS_TMP = BrowserProvider.TABLE_BOOKMARKS_TMP;
> +    static final String TABLE_HISTORY_TMP = BrowserProvider.TABLE_HISTORY_TMP;
> +    static final String TABLE_IMAGES_TMP = BrowserProvider.TABLE_IMAGES_TMP;
> +
> +    public BrowserDatabaseHelper(Context context, String databasePath, BrowserProvider provider) {

looking through this file mProvider is only used once, to insert a favicon ( mProvider.insertFavicon(SQLiteDatabase, ContentValues) ). It might be worth duplicating that utility function for now to prevent this ugly dependency. Or we could make that method static and reference it as such for now. Dependence on a BrowserProvider instance would make it hard to implement bug 959290.

@@ +854,5 @@
> +        Class<?> stringsClass = R.string.class;
> +        Field[] fields = stringsClass.getFields();
> +        Pattern p = Pattern.compile("^bookmarkdefaults_title_");
> +
> +        Integer mobileFolderId = mProvider.getMobileFolderId(db);

getMobileFolderId() was only being used by the helper. It has been moved in to BrowserDatabaseHelper

Comment 17

[:Margaret Leibovic]

Comment on attachment 8370330 [details] [diff] [review]
bug-961238-fix.patch

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

I still want to see another version before we land this. My main complaint is the duplicated insertFavicon logic.

rnewman had good points about where we can evolve this farther down the road, but the point of this first round of refactoring is just to move things into different files, and to minimize risk we should keep the changes as small as possible.

::: mobile/android/base/db/BrowserContract.java
@@ +106,5 @@
>          public static final String URL = "url";
>          public static final String DATA = "data";
>          public static final String PAGE_URL = "page_url";
> +
> +        static final String TABLE_NAME = "favicons";

Nit: I would put these above the column name declarations, since logically you have a table before you have columns. But I like this idea of putting the table name in here!

@@ +313,5 @@
>          public static final String IMAGE_URL = "image_url";
>          public static final String CREATED = "created";
>      }
> +
> +    public static final class Views {

I don't know if I like this, since this data type is inconsistent with the rest in this class. It seems like it might be better to put these view names with the data types they correspond to. For example, COMBINED could really just be Combined.VIEW_NAME. However, it gets a bit trickier with the *_with_favicons views. Maybe those should be Combined.VIEW_WITH_FAVICONS_NAME, Hisotory.VIEW_WITH_FAVICONS_NAME, etc.

What do you think?

@@ +321,5 @@
> +        public static final String HISTORY_WITH_FAVICONS = "history_with_favicons";
> +        public static final String COMBINED_WITH_FAVICONS = "combined_with_favicons";
> +    }
> +
> +    static final class Obsolete {

It's annoying that we have this here just for old migrations that aren't even used anymore.

But best to do one thing at a time. This can go away with bug 947018. Perhaps add some documentation explaining what this is for, and linking to that bug number?

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +54,5 @@
> +    public static final String DATABASE_NAME = "browser.db";
> +
> +    final protected Context mContext;
> +
> +    static final String TABLE_BOOKMARKS = BrowserContract.Bookmarks.TABLE_NAME;

You're importing BrowserContract.Bookmarks, so you should be able to just access this with Bookmarks.TABLE_NAME. Same goes for the other BrowserContract classes.

@@ +85,5 @@
> +        mContext = context;
> +    }
> +
> +    @Override
> +    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {

To help me review, please leave onUpgrade and onOpen where they were before. It makes comparing the diffs easier :) You can write a follow-up patch to move them if you want.

@@ +1685,5 @@
> +                c.close();
> +        }
> +    }
> +
> +    long insertFavicon(SQLiteDatabase db, ContentValues values) {

Is this method just totally copied over from BrowserProvider? That seems like a lot of code to just copy :/ I would prefer to see a cross dependency on BrowserProvider than to have this kind of code duplication.

::: mobile/android/base/db/BrowserProvider.java
@@ +63,5 @@
>      static final long DEFAULT_EXPIRY_PRESERVE_WINDOW = 1000L * 60L * 60L * 24L * 28L;     // Four weeks.
>      // Minimum number of thumbnails to keep around.
>      static final int DEFAULT_EXPIRY_THUMBNAIL_COUNT = 15;
>  
> +    static final String TABLE_BOOKMARKS = BrowserContract.Bookmarks.TABLE_NAME;

Same comment applies here.

::: mobile/android/base/db/TransactionalProvider.java
@@ +17,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +
> +public abstract class TransactionalProvider<T extends SQLiteOpenHelper> extends ContentProvider {

Add some documentation explaining what this is for.

Comment 18

[:Margaret Leibovic]

(In reply to Sola Ogunsakin [:sola] from comment #16)

> @@ +70,5 @@
> > +    static final String TABLE_BOOKMARKS_TMP = BrowserProvider.TABLE_BOOKMARKS_TMP;
> > +    static final String TABLE_HISTORY_TMP = BrowserProvider.TABLE_HISTORY_TMP;
> > +    static final String TABLE_IMAGES_TMP = BrowserProvider.TABLE_IMAGES_TMP;
> > +
> > +    public BrowserDatabaseHelper(Context context, String databasePath, BrowserProvider provider) {
> 
> looking through this file mProvider is only used once, to insert a favicon (
> mProvider.insertFavicon(SQLiteDatabase, ContentValues) ). It might be worth
> duplicating that utility function for now to prevent this ugly dependency.
> Or we could make that method static and reference it as such for now.
> Dependence on a BrowserProvider instance would make it hard to implement bug
> 959290.

Sorry, I didn't see this until now. If this is the case, I suppose it's fine to have the code duplication. However, it would be nice if there was a shared place for that. Or could BrowserProvider call into a method that lives in BrowserDatabseHelper?

Unfortunately, the responsibilities of these two classes are definitely not clear, and that's not something we're going to solve in this bug :(

Comment 19

[Sola Ogunsakin [:sola]]

Comment on attachment 8370330 [details] [diff] [review]
bug-961238-fix.patch

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

::: mobile/android/base/db/BrowserContract.java
@@ +313,5 @@
>          public static final String IMAGE_URL = "image_url";
>          public static final String CREATED = "created";
>      }
> +
> +    public static final class Views {

yeah that sounds good. the columns returned by each view would be those ones defined in its class.

Comment 20

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Comment 21

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Comment 22

[:Margaret Leibovic]

Comment on attachment 8371863 [details] [diff] [review]
bug-961238-fix.patch

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

Looking good! Most of my nits are about making the comments more grammatically correct, as well as access levels.

Let's get a try push, and then we can brace ourselves to land this!

::: mobile/android/base/db/BrowserContract.java
@@ +113,5 @@
>      @RobocopTarget
>      public static final class Thumbnails implements CommonColumns {
>          private Thumbnails() {}
>  
> +        public static final String TABLE_NAME = "thumbnails";

Why are some of these public but others aren't? Given that you're only changing files in the db package, I imagine they could all be package access only.

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +53,5 @@
> +    public static final String DATABASE_NAME = "browser.db";
> +
> +    final protected Context mContext;
> +
> +    static final String TABLE_BOOKMARKS_JOIN_FAVICONS = Bookmarks.TABLE_NAME + " LEFT OUTER JOIN " +

Oof, there's a lot more changes in here now than there were before. I liked that in your previous patches you just defined new member variables in here, so that the contents of all the methods could just stay the same.

I looked through a diff of the old and new versions, but there is a lot of code here, so I'm just trusting that the compiler will make sure you didn't miss changing anything that needed to change.

@@ +1634,5 @@
> +            }
> +        }
> +    }
> +
> +    static final String qualifyColumn(String table, String column) {

This can be private. I would also say that this method is silly and you should just call DBUtils.qualifyColumn directly, but that would require so many changes that it's not worth it.

@@ +1654,5 @@
> +            Log.d(LOGTAG, message);
> +        }
> +    }
> +
> +    protected Integer getMobileFolderId(SQLiteDatabase db) {

Why protected, not private?

@@ +1674,5 @@
> +                c.close();
> +        }
> +    }
> +
> +    long insertFavicon(SQLiteDatabase db, ContentValues values) {

Add a comment that this is duplicated in BrowserProvider. This can also be private, since it's only used in here.

::: mobile/android/base/db/TransactionalProvider.java
@@ +28,5 @@
> +    protected PerProfileDatabases<T> mDatabases;
> +
> +    /*
> +     * Returns the name of the database file. Used get a path
> +     * of the DB file.

s/a path of/a path to/

@@ +35,5 @@
> +     */
> +    abstract protected String getDatabaseName();
> +
> +    /*
> +     * Creates and returns the an instance of a DB helper. Given a

s/the//

@@ +45,5 @@
> +     */
> +    abstract protected T createDatabaseHelper(Context context, String databasePath);
> +
> +    /*
> +     * Insert an items into the database within a DB transaction.

s/an//

@@ +67,5 @@
> +    /*
> +     * Updates the database within a DB transaction.
> +     *
> +     * @param uri            query URI
> +     * @param values         updates to be made

Nit: This isn't very descriptive. To steal a line from Android's ContentProvider docs, maybe something better would be "A set of column_name/value pairs to add to the database."

@@ +77,5 @@
> +
> +    /*
> +     * Fetches a readable database based on the profile indicated in the
> +     * passed URI. If the URI does not contain a profile param, the default profile
> +     * is used

Nit: Period at the end of the sentence (I hate myself for making a nit about comment punctuation, but hey, you're writing a full sentence here!)

@@ +226,5 @@
> +        int successes = 0;
> +
> +        final SQLiteDatabase db = getWritableDatabase(uri);
> +
> +        db.beginTransaction();

I know you're copying this over from existing code in BrowserProvider, but do you know why this beginTransaction call isn't wrapped in a version check?

Comment 23

[Sola Ogunsakin [:sola]]

(In reply to :Margaret Leibovic from comment #22)
> Comment on attachment 8371863 [details] [diff] [review]
> bug-961238-fix.patch
> 
> @@ +226,5 @@
> > +        int successes = 0;
> > +
> > +        final SQLiteDatabase db = getWritableDatabase(uri);
> > +
> > +        db.beginTransaction();
> 
> I know you're copying this over from existing code in BrowserProvider, but
> do you know why this beginTransaction call isn't wrapped in a version check?

not sure. i'll add it in

Comment 24

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch.patch

Comment 25

[Sola Ogunsakin [:sola]]

Try run 
https://tbpl.mozilla.org/?tree=Try&rev=e67d3e17ef54

Comment 26

[:Margaret Leibovic]

(In reply to Sola Ogunsakin [:sola] from comment #23)
> (In reply to :Margaret Leibovic from comment #22)
> > Comment on attachment 8371863 [details] [diff] [review]
> > bug-961238-fix.patch
> > 
> > @@ +226,5 @@
> > > +        int successes = 0;
> > > +
> > > +        final SQLiteDatabase db = getWritableDatabase(uri);
> > > +
> > > +        db.beginTransaction();
> > 
> > I know you're copying this over from existing code in BrowserProvider, but
> > do you know why this beginTransaction call isn't wrapped in a version check?
> 
> not sure. i'll add it in

Can you file a bug to investigate whether we can actually just get rid of this logic? I'm suspicious of why we need it, if this bulk insert method was working fine without it.

Comment 27

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #26)

> Can you file a bug to investigate whether we can actually just get rid of
> this logic? I'm suspicious of why we need it, if this bulk insert method was
> working fine without it.

See Bug 744959, and Bug 947939 Comment 3 onwards.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bb4254c72efb

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/fec5d183e1b9 for robocop bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=34447609&tree=Fx-Team

Odd that your try push was green but it consistently failed on fx-team, but your patch was the only one Ryan pushed in that batch that touched Android and was about favicons, and the test failures were about favicons on Android...

Comment 30

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

My mistake. Made some changes after the try push that caused that error. I've updated the main patch with the fix and I just pushed to try again. https://tbpl.mozilla.org/?tree=Try&rev=42aeac796b6c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/bb5701078d13

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb5701078d13

Comment 33

[Ryan VanderMeulen [:RyanVM]]

And of course, right after I merge this to m-c I realize that troboprovider hasn't passed on fx-team since this landed. Argh. Backed out. Please make sure this has a full Android Try run before requesting checkin again.
https://hg.mozilla.org/mozilla-central/rev/879038dcacb7

https://tbpl.mozilla.org/php/getParsedLog.php?id=34506179&tree=Fx-Team

Comment 34

[Sola Ogunsakin [:sola]]

Attachment: bug-961238-fix.patch

Fixed issue that broke tprobovider test.

Comment 35

[Sola Ogunsakin [:sola]]

Did a try push, looks good to me https://tbpl.mozilla.org/?tree=Try&rev=773c70191b1f. Lets try this again :)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/02e2d24da0bd

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02e2d24da0bd

Comment 38

[Richard Newman [:rnewman]]

BrowserContract, by definition, shouldn't import huge chunks of Fennec.

This change introduced a dependency on DBUtils, which depends on GeckoAppShell. Which makes out-of-tree consumers of BrowserContract very unhappy.

This will need another commit to fix.

Comment 39

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/5d7a801f4743

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/5d7a801f4743