941357 - Consolidate copy/pasted profile-related logic in content providers

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

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.

Comment 2

[Richard Newman [:rnewman]]

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

Comment 3

[:Margaret Leibovic]

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

Comment 4

[:Margaret Leibovic]

Attachment: (Part 1) Create new PerProfileDatabases class

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 5

[Richard Newman [:rnewman]]

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;

Comment 6

[:Margaret Leibovic]

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

Comment 7

[Richard Newman [:rnewman]]

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

Comment 8

[:Margaret Leibovic]

Attachment: (Part 1 v2) Create new PerProfileDatabases class

Updated to address comments.

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

Comment 9

[:Margaret Leibovic]

Attachment: (Part 2) Update BrowserProvider to use PerProfileDatabases

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

Comment 10

[Richard Newman [:rnewman]]

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 11

[Richard Newman [:rnewman]]

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?

Comment 12

[:Margaret Leibovic]

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

Comment 13

[:Margaret Leibovic]

Attachment: (Part 2 v2) Update BrowserProvider to use PerProfileDatabases

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

Comment 14

[Richard Newman [:rnewman]]

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

[Richard Newman [:rnewman]]

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

Comment 16

[Richard Newman [:rnewman]]

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

Comment 17

[:Margaret Leibovic]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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