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 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 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 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 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

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 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 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.

(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.

(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 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 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 :-)

Got rid of BrowserDatabaseHelper dependency on BrowserProvider.

(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 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 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.

(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 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 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?

(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

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

(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.

(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.

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...

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

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

Fixed issue that broke tprobovider test.

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

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.