Looking at BrowserProvider/TabsProvider, some things that could move into some shared place: getDatabaseHelperForProfile getDatabasePath getReadableDatabase getWritableDatabase Adding a new content provider in 941318 may be the impetus to do this.

Also, the update/insert/etc. methods are almost all the same... maybe we should create our own abstract content provider class, then implement the methods updateInTransaction/insertInTransaction/etc. in the individual content provider classes.

I'd suggest adding these to PerProfileContentProvider, or even a parent class of that if it's not per-profile related.

Phase 1 is to move the DatabaseHelpers in TabsProvider/BrowserProvider to their own files, just to make this code more manageable to look at. Here's a try push with that: https://tbpl.mozilla.org/?tree=Try&rev=fe9077a8728c I'll upload patches once I make sure this actually works...

Well, that last try patch totally failed. I decided that moving the DatabaseHelper classes into their own files is a bit of scope creep here. The real point of this bug is that I want to be able to create a new content provider without us copy/pasting the profile managing logic (again). Here's a patch that factors that logic out into PerProfileDatabases, starting with TabsProvider. I'll upload another patch to do the same thing for BrowserProvider. We can move more clean-up work into other bugs, ones which don't block progress on the new lists content provider. Here's a try push with just this patch: https://tbpl.mozilla.org/?tree=Try&rev=5c26ffeeb778 Also, I got rid of the old migration code to move the database into the profile on 2.2 devices. It shipped in Firefox 15, we don't really care about 2.2 anymore, and in the worst case we just end up making a new database for users affected by this.

Comment on attachment 8343911 [details] [diff] [review] (Part 1) Create new PerProfileDatabases class Review of attachment 8343911 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/PerProfileDatabases.java @@ +21,5 @@ > + private final HashMap<String, T> mStorages = new HashMap<String, T>(); > + > + private Context mContext; > + private String mDatabaseName; > + private DatabaseHelperFactory<T> mHelperFactory; All three final. @@ +39,5 @@ > + if (profileDir == null) { > + return null; > + } > + > + return new File(profileDir, mDatabaseName).getAbsolutePath(); Maybe just return the File? @@ +53,5 @@ > + if (mStorages.containsKey(profile)) { > + return mStorages.get(profile); > + } > + > + final String databasePath = getDatabasePath(profile); null check -- gDP can return null, so you should throw here if it does. @@ +58,5 @@ > + > + final T helper = mHelperFactory.makeDatabaseHelper(mContext, databasePath); > + mStorages.put(profile, helper); > + > + DBUtils.ensureDatabaseIsNotLocked(helper, databasePath); Do this *before* we put it in the map? Otherwise the next call to gDHFP will return it anyway. ::: mobile/android/base/db/TabsProvider.java @@ +33,5 @@ > public class TabsProvider extends ContentProvider { > private static final String LOGTAG = "GeckoTabsProvider"; > private Context mContext; > > + private PerProfileDatabases mDatabases; PerProfileDatabases<TabsDatabaseHelper> mDatabases;

(In reply to Richard Newman [:rnewman] from comment #5) > @@ +53,5 @@ > > + if (mStorages.containsKey(profile)) { > > + return mStorages.get(profile); > > + } > > + > > + final String databasePath = getDatabasePath(profile); > > null check -- gDP can return null, so you should throw here if it does. I was just copying over the existing logic, but now I wonder if there's any case where we would actually want a null return value from getDatabasePath, so perhaps that method is the one that should throw. In fact, things are probably pretty borked if we can't find a profile directory.

So long as something throws somewhere :) (Bear in mind that the profile dir won't exist if we're syncing to a profile that hasn't yet been opened since the last Clear Data. If there are other callers of gDP, bear that in mind.)

Updated to address comments. I decided to just throw inside getDatabaseHelperForProfile, since that maintains current behavior for consumers of TabsProvider.gDP().

Try push for this patch as well: https://tbpl.mozilla.org/?tree=Try&rev=90c8f75687d8

Comment on attachment 8344060 [details] [diff] [review] (Part 2) Update BrowserProvider to use PerProfileDatabases Review of attachment 8344060 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/BrowserProvider.java @@ +364,5 @@ > Log.d(LOGTAG, message); > } > } > > + final class BrowserDatabaseHelperFactory implements DatabaseHelperFactory<BrowserDatabaseHelper> { You can make this private, rather than package-scoped, no? But you're probably better just inlining it into the one call we make: ..., new DatabaseHelperFactory<BrowserDatabaseHelper> { @Override ... }); @@ +1964,2 @@ > @RobocopTarget > + private String getDatabasePath(String profile) { Once we finish eliminating uses of reflection, you can't have this be private and still call it from Robocop. Leave it as public, unless you want to hide it so much you are happy mandating the use of reflection to call this. @@ +1973,5 @@ > > if (uri != null) > profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE); > > + return mDatabases.getDatabaseHelperForProfile(profile).getReadableDatabase(); So this no longer requires special casing for tests?

Comment on attachment 8344058 [details] [diff] [review] (Part 1 v2) Create new PerProfileDatabases class Review of attachment 8344058 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/db/PerProfileDatabases.java @@ +55,5 @@ > + } > + > + final String databasePath = getDatabasePathForProfile(profile); > + if (databasePath == null) { > + throw new NullPointerException("Database path is null for profile: " + profile); IllegalStateException?

(In reply to Richard Newman [:rnewman] from comment #10) > Comment on attachment 8344060 [details] [diff] [review] > (Part 2) Update BrowserProvider to use PerProfileDatabases > > Review of attachment 8344060 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mobile/android/base/db/BrowserProvider.java > @@ +364,5 @@ > > Log.d(LOGTAG, message); > > } > > } > > > > + final class BrowserDatabaseHelperFactory implements DatabaseHelperFactory<BrowserDatabaseHelper> { > > You can make this private, rather than package-scoped, no? But you're > probably better just inlining it into the one call we make: > > ..., new DatabaseHelperFactory<BrowserDatabaseHelper> { > @Override ... > }); For some reason I thought that anonymous interface implementations were bad. Do you know why I might have been confused? > @@ +1964,2 @@ > > @RobocopTarget > > + private String getDatabasePath(String profile) { > > Once we finish eliminating uses of reflection, you can't have this be > private and still call it from Robocop. Leave it as public, unless you want > to hide it so much you are happy mandating the use of reflection to call > this. Oops, that was an unintentional change. > @@ +1973,5 @@ > > > > if (uri != null) > > profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE); > > > > + return mDatabases.getDatabaseHelperForProfile(profile).getReadableDatabase(); > > So this no longer requires special casing for tests? Sigh, also a failure while copy/pasting. Good catch.

This time properly implemented to account for that `isTest` flag... which I don't like :( I didn't really want to propegate that flag over to PerProfileDatabases, but if I didn't do that, I'd have to change around getDatabaseHelperForProfile to somehow handle getting that special test path, and this seemed like the past of least resistance for right now. Here's another try push (I cancelled the last one): https://tbpl.mozilla.org/?tree=Try&rev=6c356d3663ea

(In reply to :Margaret Leibovic from comment #12) > For some reason I thought that anonymous interface implementations were bad. > Do you know why I might have been confused? They definitely can be bad: * They tend to proliferate, and you can easily end up with multiple anonymous classes that do the same thing. * They're automatically non-static, so that helper holds a reference to BrowserProvider. (No difference here, but that's not always the case -- <https://mail.mozilla.org/pipermail/mobile-firefox-dev/2013-November/000336.html>) * They can be hard to read when they involve multiple method definitions. That's more likely when implementing an interface than when subclassing, because by definition you must implement the entire interface. You might also be thinking of this pattern: public interface IFoo { public class FooImpl implements IFoo { } } which is a good choice in maybe 1% of the instances where people do that!

Comment on attachment 8344110 [details] [diff] [review] (Part 2 v2) Update BrowserProvider to use PerProfileDatabases Review of attachment 8344110 [details] [diff] [review]: ----------------------------------------------------------------- Let's keep rollin'!

(In reply to :Margaret Leibovic from comment #13) > This time properly implemented to account for that `isTest` flag... which I > don't like :( I didn't really want to propegate that flag over to > PerProfileDatabases, but if I didn't do that, I'd have to change around > getDatabaseHelperForProfile to somehow handle getting that special test > path, and this seemed like the past of least resistance for right now. If it doesn't apply to all CPs, you could do: TestAwarePerProfileDatabases extends PerProfileDatabases ... { @Override ... } Or just wait until we're using the services testing stuff, where we solved this problem, I think.

All green on try! We can fight the battle to get rid of that isTest parameter in another bug, perhaps when we've made more progress on the testing stuff. https://hg.mozilla.org/integration/fx-team/rev/f120be749410 https://hg.mozilla.org/integration/fx-team/rev/904545658c9b

https://hg.mozilla.org/mozilla-central/rev/f120be749410 https://hg.mozilla.org/mozilla-central/rev/904545658c9b