731491 - Extract shared ContentProvider superclass

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

There's a lot of code duplication going on.

Shared between providers could/should be:

* getReadableDatabase, getWritableDatabase
* onCreate
* A parameterized version of getDatabasePath
* getDatabaseHelperForProfile
* … DatabaseHelper (each would override onCreate, onUpgrade)
* Quite possibly all of the non-"inTransaction" delete etc. methods

Comment 1

[Richard Newman [:rnewman]]

Probably "PerProfileContentProvider" is a suitably Javaish name.

Comment 2

[Richard Newman [:rnewman]]

This looks like it's been done as GeckoProvider. There are some problems with it.


= The name should perhaps be improved. It has this as a member variable:

    private HashMap<String, SQLiteBridge> mDatabasePerProfile;

which should give a pointer as to its correct name.


= setLogTag and getLogTag… aiee! A mutable private member and a method call in every log invocation? I would suggest one of two improvements:

  1. Don't care that methods in GeckoProvider will log with a non-specific log tag. Just have a private static final String LOG_TAG = "GeckoProvider", and have each subclass define their own if they log.

  2. Integrate the log-level-aware logging from BrowserProvider.

I don't care what you do, but not what's currently in there :)


= Your subclasses should have @Override annotations when they override methods. Help the compiler to help you.

= Your accessors to mutable members should be synchronized.

= The correct pattern for this kind of abstract class is not:

  private Thing mDBName
  protected Thing getDBName() {
    return mDBName;
  }

with the subclass calling setDBName(). Do this instead:

  GeckoProvider:

  protected abstract Thing getDBName();


  FormHistoryProvider:

  @Override
  protected Thing getDBName() {
    return DB_FILENAME:
  }

Look! No member to synchronize, no need for the subclass to know the DB name in advance, no setter, no problems if you have more than a single level of hierarchy (each level will call setDBName!), and altogether better OO.

I am very happy to review class design patches like this in the future.

Comment 3

[Doug Turner (:dougt)]

not blocking fennec on refactoring.  Is this needed for something else?

Comment 4

[Richard Newman [:rnewman]]

(In reply to Doug Turner (:dougt) from comment #3)
> not blocking fennec on refactoring.  Is this needed for something else?

I wouldn't call it a blocker, and it doesn't block other work.

GeckoProvider as implemented is needlessly expensive and not thread safe, but I'm sure it ain't the only culprit in the tree!

Comment 5

[Wesley Johnston (:wesj)]

This would be a a good way for someone to get into work on Fennec.

Comment 6

[Richard Newman [:rnewman]]

And I offer myself as continued reviewer, particularly from a threading and services perspective.

Comment 7

[Federico Paolinelli]

Hi there, I was looking for a bug to cut my teeth on. I only downloaded and built at the moment. Can I grab this one?

Comment 8

[Wesley Johnston (:wesj)]

Yep! Getting things to build is often the hardest part, so congrats and getting there! Feel free to dig in and ask any questions you've got here (or on irc).

Comment 9

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring

This is my first attempt. I only refactored GeckoProvider and its subclasses which were PasswordProvider and FormHistoryProvider

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 734144 [details] [diff] [review]
Patch for GeckoProvider refactoring

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

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +149,5 @@
>      public void onPostQuery(Cursor cursor, Uri uri, SQLiteBridge db) { }
> +
> +
> +
> +

Go easy on the whitespace!

::: mobile/android/base/db/GeckoProvider.java.in
@@ +39,3 @@
>      private HashMap<String, SQLiteBridge> mDatabasePerProfile;
>      protected Context mContext = null;
> +    private final String LOG_TAG = "PerProfileProvider";

Using all caps here implies that this is static, which it isn't.

But note that the purpose of mLogTag being mutable was that a subclass would set it, and all of the logging in *this* file would magically start printing the subclass's name.

If you do what you've done here, you'll have new methods in FormHistoryProvider logging with that tag, and all of these methods logging as PerProfileProvider.

To do what we intend to do here you need to take a similar approach to getDBName, and implement a getLogTag method that the subclass will override.

But that's expensive, so let's just do this: add a logTag argument to PerProfileContentProvider's constructor, keep the member variable (assigned from the constructor, so it can be `protected final`), and call super(LOG_TAG) in the child classes.

@@ +124,5 @@
> +                db = getDB(mContext, dbPath);
> +                if (db != null){
> +                    mDatabasePerProfile.put(dbPath, db);
> +                }
> +            }

You can simplify this clause to avoid the early definition and use early return, which is our preferred coding style because it avoids deep nesting.


  synchronized (this) {
    final String dbPath = getDatabasePathForProfile(profile);
    SQLiteBridge db = mDatabasePerProfile.get(dbPath);
    if (db != null) {
      return db;
    }
    db = getDB(mContext, dbPath);
    if (db != null) { 
      mDatabasePerProfile.put(dbPath, db);
    }
    return db;
  }

But more importantly, it seems like you should be able to share logic between this method and getDatabaseForPath, no?

@@ +249,5 @@
>          final SQLiteBridge db = getDatabase(uri);
>          // If we can not get a SQLiteBridge instance, its likely that the database
>          // has not been set up and Gecko is not running. We return 0 and expect
>          // callers to try again later
> +        if (db == null){

Nit: space between ){.

@@ +334,5 @@
>      }
>  
> +    protected abstract String getDBName();
> +
> +    protected abstract int getDBVersion();

At least so far, these should (or at least can) be static.

Comment 11

[Federico Paolinelli]

(In reply to Richard Newman [:rnewman] from comment #10)
> Comment on attachment 734144 [details] [diff] [review]
> Patch for GeckoProvider refactoring
> 
> Review of attachment 734144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/db/FormHistoryProvider.java.in
> @@ +149,5 @@
> >      public void onPostQuery(Cursor cursor, Uri uri, SQLiteBridge db) { }
> > +
> > +
> > +
> > +
> 
> Go easy on the whitespace!
> 

Oops

> ::: mobile/android/base/db/GeckoProvider.java.in
> @@ +39,3 @@
> >      private HashMap<String, SQLiteBridge> mDatabasePerProfile;
> >      protected Context mContext = null;
> > +    private final String LOG_TAG = "PerProfileProvider";
> 
> Using all caps here implies that this is static, which it isn't.
> 
> But note that the purpose of mLogTag being mutable was that a subclass would
> set it, and all of the logging in *this* file would magically start printing
> the subclass's name.
> 
> If you do what you've done here, you'll have new methods in
> FormHistoryProvider logging with that tag, and all of these methods logging
> as PerProfileProvider.
> 
> To do what we intend to do here you need to take a similar approach to
> getDBName, and implement a getLogTag method that the subclass will override.
> 
> But that's expensive, so let's just do this: add a logTag argument to
> PerProfileContentProvider's constructor, keep the member variable (assigned
> from the constructor, so it can be `protected final`), and call
> super(LOG_TAG) in the child classes.

Well, to be honest I forgot the static. I am aware that doing that will result in PerProfileProvider logs but I thought it was fine as per your comment

"1. Don't care that methods in GeckoProvider will log with a non-specific log tag. Just have a private static final String LOG_TAG = "GeckoProvider", and have each subclass define their own if they log."

So I went for the easy way :-) In any case, the solution you are suggesting makes a lot more sense. I'll change it.

> 
> @@ +124,5 @@
> > +                db = getDB(mContext, dbPath);
> > +                if (db != null){
> > +                    mDatabasePerProfile.put(dbPath, db);
> > +                }
> > +            }
> 
> You can simplify this clause to avoid the early definition and use early
> return, which is our preferred coding style because it avoids deep nesting.
> 
> 
>   synchronized (this) {
>     final String dbPath = getDatabasePathForProfile(profile);
>     SQLiteBridge db = mDatabasePerProfile.get(dbPath);
>     if (db != null) {
>       return db;
>     }
>     db = getDB(mContext, dbPath);
>     if (db != null) { 
>       mDatabasePerProfile.put(dbPath, db);
>     }
>     return db;
>   }
> 
> But more importantly, it seems like you should be able to share logic
> between this method and getDatabaseForPath, no?

Right :-)

> 
> @@ +249,5 @@
> >          final SQLiteBridge db = getDatabase(uri);
> >          // If we can not get a SQLiteBridge instance, its likely that the database
> >          // has not been set up and Gecko is not running. We return 0 and expect
> >          // callers to try again later
> > +        if (db == null){
> 
> Nit: space between ){.

Oops. I tried me to adhere to the spacing style but I missed this.

> 
> @@ +334,5 @@
> >      }
> >  
> > +    protected abstract String getDBName();
> > +
> > +    protected abstract int getDBVersion();
> 
> At least so far, these should (or at least can) be static.

Mmmm I am not sure I fully understood what you mean here. 
Do you mean the methods should be static? From my understanding overriding static methods would result in hiding the superclass class ones. 
Here we want to implement the behaviour in the sub classes (as you suggested for the log tags).
If I set it static it can be abstract anymore. 
On the other hand, if I override it as plain static I am afraid that calls of getDBName() from the base class would result in calling the superclass implementation and not the one that we want to be called which is not what we want.
Am I missing something?


One last thing. Is it ok to change the access level of the other abstract methods from public to protected? I just noticed the discrepancy between them.

Comment 12

[Federico Paolinelli]

While trying to share the logic between getDatabaseForPath and getDatabaseForProfile I noticed something that looks odd.

   private SQLiteBridge getDatabaseForPath(String profilePath) {
        SQLiteBridge db = null;
        synchronized (this) {
            db = mDatabasePerProfile.get(profilePath);
            if (db != null){
                return db;
            }
            File profileDir = new File(profilePath, getDBName());
            final String dbPath = profileDir.getPath();
            db = getDB(mContext, dbPath);
            if (db != null){
                mDatabasePerProfile.put(dbPath, db);
            }
        }

        return db;
    }

Basically it looks for an entry with "profilePath" key but when the lookup fails it adds it with another key (profileDir.path()) .

This same behaviuor was already there before my first refactor (just better hidden). Is it something that you wanted? Because I would say that no record is ever added with profilePath and the search always fails.

And more: does it make sense to have the path of the file as the key of the map instead of the profile / the profilePath ? 
If you look getDatabaseForProfile we are creating a File object (and calling getPath()) just to get the path to use for the lookup on the map. This is done for every call to the content provider. I would say using profile / profilePath as key is more efficient and is as unique as is the path we get from it but again, I want to be sure that I am not missing anything.

Comment 13

[Federico Paolinelli]

Update: it looks like that PARAM_PROFILE_PATH is never used while performing queries, so the piece of code:

        profilePath = uri.getQueryParameter(BrowserContract.PARAM_PROFILE_PATH);

        // Testing will specify the absolute profile path
        if (profilePath != null) {
            return getDatabaseForPath(profilePath);
        }

would never run. The comment says that it is used for testing, but the only place I found it is in the row:

testMigration.java:            String profilePath = (String)browserContract.getField("PARAM_PROFILE_PATH").get(null);

but again, profilePath is never used.

For these reasons, I would say that it is safe to remove PARAM_PROFILE_PATH and all its occourrences (there are a lot of "if (profilePath != null) {" that can be removed.

Again, please correct me if I am missing something since I am pretty new to this code.

Comment 14

[Wesley Johnston (:wesj)]

Its used by tests, but we don't use the const name there (because I would have had to reflect it out, and I was lazy). See for example: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testFormHistory.java.in#52

Comment 15

[Federico Paolinelli]

(In reply to Federico Paolinelli from comment #12)
> And more: does it make sense to have the path of the file as the key of the
> map instead of the profile / the profilePath ? 
> If you look getDatabaseForProfile we are creating a File object (and calling
> getPath()) just to get the path to use for the lookup on the map. This is
> done for every call to the content provider. I would say using profile /
> profilePath as key is more efficient and is as unique as is the path we get
> from it but again, I want to be sure that I am not missing anything.

This is wrong as well. The key of the map is the path + the db name. In any case I think creating a File() object every time could be avoided (building a key concatenating profile and dbname?)

Comment 16

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring (second version)

The attached version contains all the suggested changes. 

Still missing: 
No action for the previous comment

@@ +334,5 @@
>      }
>  
> +    protected abstract String getDBName();
> +
> +    protected abstract int getDBVersion();

At least so far, these should (or at least can) be static.

------------------

If this code is really performance critical I would suggest to use a rw lock in place of the synchronize(this). When the map is populated it would result in only read locks. 

As per my previous comment, File() object creation could be avoided using a different map key. In any case this would require a string concatenation that of course requires creating an object.

Comment 17

[Richard Newman [:rnewman]]

Comment on attachment 736397 [details] [diff] [review]
Patch for GeckoProvider refactoring (second version)

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

Hi Federico, sorry for delay in responding.

Commit message should be

  Bug 731491 - Rework GeckoProvider. r=rnewman


> This is wrong as well. The key of the map is the path + the db name.

Yes. In my opinion this is unnecessary: it would be fine to mix full paths (for tests) and partial paths (for real code) in the same map:

  1234354.profile/foo.db: <db>
  /storage/sd/some/test.db: <db>

They'll never collide, because this class doesn't manage profiles with the exact same name underneath different storage roots (profiles should never be copied with the same name), and of course full and relative paths are separate sets.

So if you feel strongly enough, you can do the extra work to use profile + dbName as the key into the map for the profile case, only calculating the full path in order to open the DB. Test code that provides the full profile path can use the path string case. 

Assuming, that is, that wesj doesn't disagree. Wes?

Otherwise, this is looking good.

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +93,5 @@
> +    protected int getDBVersion(){
> +        return DB_VERSION;
> +    }
> +
> +

Nit: surplus newline.

@@ +215,5 @@
>                      result = NSSBridge.decrypt(mContext, initialValue);
>                  }
>              }
>          } catch (Exception ex) {
> +            Log.e(mLogTag, "Error in NSSBridge");

Should be LOG_TAG.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +39,3 @@
>      private HashMap<String, SQLiteBridge> mDatabasePerProfile;
>      protected Context mContext = null;
> +    protected final String mLogTag;

It might be better to mark this as private.

@@ +136,5 @@
> +            if (db != null) {
> +                return db;
> +            }
> +            db = getDB(mContext, dbPath);
> +            if (db != null){

Space between ) and {.

@@ +144,4 @@
>          return db;
>      }
>  
> + 

Trailing whitespace, but just kill two of these three newlines.

Comment 18

[Federico Paolinelli]

> 
> Hi Federico, sorry for delay in responding.
> 

No problem

> Commit message should be
> 
>   Bug 731491 - Rework GeckoProvider. r=rnewman
> 
>

I did not get this.
 
> > This is wrong as well. The key of the map is the path + the db name.
> 
> Yes. In my opinion this is unnecessary: it would be fine to mix full paths
> (for tests) and partial paths (for real code) in the same map:
> 
>   1234354.profile/foo.db: <db>
>   /storage/sd/some/test.db: <db>
> 
> They'll never collide, because this class doesn't manage profiles with the
> exact same name underneath different storage roots (profiles should never be
> copied with the same name), and of course full and relative paths are
> separate sets.
> 
> So if you feel strongly enough, you can do the extra work to use profile +
> dbName as the key into the map for the profile case, only calculating the
> full path in order to open the DB. Test code that provides the full profile
> path can use the path string case. 
> 
> Assuming, that is, that wesj doesn't disagree. Wes?
> 
> Otherwise, this is looking good.
>

I feel strong! Do I need wes approval before submitting the patch (again)?
 

> @@ +215,5 @@
> >                      result = NSSBridge.decrypt(mContext, initialValue);
> >                  }
> >              }
> >          } catch (Exception ex) {
> > +            Log.e(mLogTag, "Error in NSSBridge");
> 
> Should be LOG_TAG.
> 



Well, that was intentional. It would allow a child class of PasswordsProvider to provide its own tag, the same way we did for PerProfileContentProvider. That's the reason why I left mLogTag as protected. It won't be hard to change that anyway :-)

Comment 19

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring (third version)

Hope this is the final version. 

I am not even sure if re-asking for feedback is the right thing to do in this case.

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 740097 [details] [diff] [review]
Patch for GeckoProvider refactoring (third version)

Hope I can get to this between meetings…

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 740097 [details] [diff] [review]
Patch for GeckoProvider refactoring (third version)

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

Just some null check details to make sure we've got right...

::: mobile/android/base/db/GeckoProvider.java.in
@@ +51,5 @@
>          }
>  
> +        synchronized (this) {
> +            Collection<SQLiteBridge> bridges = mDatabasePerProfile.values();
> +            Iterator<SQLiteBridge> it = bridges.iterator();

for (SQLiteBridge bridge : mDatabasePerProfile.values()) {
    if (bridge != null ) {
        try {
            bridge.close();
        } catch (Exception ex) {
        }
    }
}
mDatabasePerProfile = null;

@@ +117,5 @@
> +    private String getDatabasePathForProfile(String profile, String dbName) {
> +        File profileDir = GeckoProfile.get(mContext, profile).getDir();
> +        if (profileDir == null) {
> +            return null;
> +        }

I'm fairly happy from reading GeckoProfile.java that this cannot occur, and profileDir will always be something so long as mContext != null.

Having this able to return null means that you need a check after line 142. Please read GeckoProfile.java to make sure, add a comment here, and either change this method to throw, or add that guard in getDatabaseForProfile.

@@ +139,5 @@
> +            if (db != null) {
> +                return db;
> +            }
> +            final String dbPath = getDatabasePathForProfile(profile, dbName);
> +            db = getDB(mContext, dbPath);

dbPath can be null here. You should check for that, and log/fail appropriately.

(Another reason to write a JavaDoc for these methods that can silently fail and return null...)

@@ +148,4 @@
>          return db;
>      }
>  
>      private SQLiteBridge getDatabaseForPath(String profilePath) {

Nit: time to rename this to `getDatabaseForProfilePath`?

@@ +153,5 @@
> +        final String dbPath = profileDir.getPath();
> +        return getDatabaseForDbPath(dbPath);
> +    }
> +
> +    private SQLiteBridge getDatabaseForDbPath(String dbPath) {

Nit: let's be consistent and call this `getDatabaseForDBPath`. And can we add a JavaDoc comment explaining that it can return null?

Comment 22

[Federico Paolinelli]

(In reply to Richard Newman [:rnewman] from comment #21)

> Comment on attachment 740097 [details] [diff] [review]

> I'm fairly happy from reading GeckoProfile.java that this cannot occur, and
> profileDir will always be something so long as mContext != null.
> 
> Having this able to return null means that you need a check after line 142.
> Please read GeckoProfile.java to make sure, add a comment here, and either
> change this method to throw, or add that guard in getDatabaseForProfile.
>> (Another reason to write a JavaDoc for these methods that can silently fail
> and return null...)
> 
> @@ +148,4 @@
> >          return db;
> >      }
> >  
> >      private SQLiteBridge getDatabaseForPath(String profilePath) {
> 
> Nit: time to rename this to `getDatabaseForProfilePath`?
> 
> @@ +153,5 @@
> > +        final String dbPath = profileDir.getPath();
> > +        return getDatabaseForDbPath(dbPath);
> > +    }
> > +
> > +    private SQLiteBridge getDatabaseForDbPath(String dbPath) {
> 
> Nit: let's be consistent and call this `getDatabaseForDBPath`. And can we
> add a JavaDoc comment explaining that it can return null?



I'm not a big fan of javadoc-ing private methods (nor I feel my english adequate), but I'll try to add them.
GeckoProfile.get(mContext, profile).getDir() will return a not null value as long as mContext is not null AND we do not get IOExceptions into its getDir() method. 

Follows (another) patch....

Comment 23

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring (4th version)

Not null checks added. 

Must admit I don't feel _completely_ sure about the javadocs. Hope I did not miss any other newline / space.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 740458 [details] [diff] [review]
Patch for GeckoProvider refactoring (4th version)

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

Looks good! Just whitespace and punctuation nits.

Please make sure mochitest-robotium passes (either locally or on Try) before landing.

Thanks for taking the time to iterate on this!

::: mobile/android/base/db/GeckoProvider.java.in
@@ +108,4 @@
>  
>          return bridge;
>      }
> +    

Nit: trailing whitespace.

@@ +109,5 @@
>          return bridge;
>      }
> +    
> +    /**
> +     * Returns the absolute path of a database file depending on the specified profile and dbName

Nit: end with period.

@@ +114,5 @@
> +     * @param profile
> +     *          the profile whose dbPath must be returned
> +     * @param dbName
> +     *          the name of the db file whose absolute path must be returned
> +     * @return the absolute path of the db file or <code>null</code> if it was not possible to retrieve a valid path

Nit: end with period.

@@ +118,5 @@
> +     * @return the absolute path of the db file or <code>null</code> if it was not possible to retrieve a valid path
> +     *
> +     */
> +    private String getDatabasePathForProfile(String profile, String dbName) {
> +        // profileDir depends on GeckoProfile.get() implementation, we double check it to be consinstent

Change this comment to

  // Depends on the vagaries of GeckoProfile.get, so null check for safety.

@@ +130,5 @@
> +    }
> +
> +    /**
> +     * Returns a SQLiteBridge object according to the specified profile id and to the name of db related to the
> +     * current provider instance

End sentence with a period.

@@ +133,5 @@
> +     * Returns a SQLiteBridge object according to the specified profile id and to the name of db related to the
> +     * current provider instance
> +     * @param profile
> +     *          the id of the profile to be used to retrieve the related SQLiteBridge
> +     * @return the <code>SQLiteBridge</code> related to the specified profile id or <code>null</code> if it was 

Trailing whitespace.

@@ +134,5 @@
> +     * current provider instance
> +     * @param profile
> +     *          the id of the profile to be used to retrieve the related SQLiteBridge
> +     * @return the <code>SQLiteBridge</code> related to the specified profile id or <code>null</code> if it was 
> +     *         not possible to retrieve a valid SQLiteBridge

End with a period.

@@ +153,5 @@
> +                return db;
> +            }
> +
> +            final String dbPath = getDatabasePathForProfile(profile, dbName);
> +            if (dbPath == null) {   

Nit: trailing whitespace.

@@ +168,5 @@
>      }
>  
> +    /**
> +     * Returns a SQLiteBridge object according to the specified profile path and to the name of db related to the
> +     * current provider instance

End with period.

@@ +172,5 @@
> +     * current provider instance
> +     * @param profilePath
> +     *          the profilePath to be used to retrieve the related SQLiteBridge
> +     * @return the <code>SQLiteBridge</code> related to the specified profile path or <code>null</code> if it was
> +     *         not possible to retrieve a valid <code>SQLiteBridge</code>

End with period.

@@ +187,5 @@
> +     *          the path of the file to be used to retrieve the related SQLiteBridge
> +     * @return the <code>SQLiteBridge</code> related to the specified file path or <code>null</code> if it was  
> +     *         not possible to retrieve a valid <code>SQLiteBridge</code>
> +     *
> +     */

Same three comments for this block :D

The main value here is the "can return null in these circumstances" part, for the record.

@@ +208,5 @@
> +     * Returns a SQLiteBridge object to be used to perform operations on the given <code>Uri</code>
> +     * @param uri
> +     *          the <code>Uri</code> to be used to retrieve the related SQLiteBridge
> +     * @return a <code>SQLiteBridge</code> object to be used on the given uri or <code>null</code> if it was 
> +     *         not possible to retrieve a valid <code>SQLiteBridge</code>

Same.

Comment 25

[Federico Paolinelli]

(In reply to Richard Newman [:rnewman] from comment #24)
> Comment on attachment 740458 [details] [diff] [review]
> Patch for GeckoProvider refactoring (4th version)
> 
> Review of attachment 740458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Just whitespace and punctuation nits.
> 
> Please make sure mochitest-robotium passes (either locally or on Try) before
> landing.

I ran only robotium tests before (hope adding some javadoc did not break anything). I'll run them again (along with the mochitests).

However I guess you (or somebody else) will need to land this patch for me since I don't have the permission to push changes. Should (may) I add the "checkin-needed" keyword when I am done?


> 
> @@ +187,5 @@
> > +     *          the path of the file to be used to retrieve the related SQLiteBridge
> > +     * @return the <code>SQLiteBridge</code> related to the specified file path or <code>null</code> if it was  
> > +     *         not possible to retrieve a valid <code>SQLiteBridge</code>
> > +     *
> > +     */
> 
> Same three comments for this block :D
> 
> The main value here is the "can return null in these circumstances" part,
> for the record.
> 

I am not sure I understood. Are you asking me to specify what the circumstances are? Because it's kind of vague. I can rely on the "gecko not initialized yet" comments for one condition, but the other one would be something like "if GeckoProfile suddenly starts returning a null dir", which do not seem to add any further information.

Comment 26

[Richard Newman [:rnewman]]

(In reply to Federico Paolinelli from comment #25)

> However I guess you (or somebody else) will need to land this patch for me
> since I don't have the permission to push changes. Should (may) I add the
> "checkin-needed" keyword when I am done?

In that case, please upload a final patch, so whoever does the checkin can land it. Yes, you can use checkin-needed.

> I am not sure I understood. Are you asking me to specify what the
> circumstances are?

No, just add punctuation.

Comment 27

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring (5th version)

Last changes as requested. Should be fine

Comment 28

[Ryan VanderMeulen [:RyanVM]]

applying 731491
patching file mobile/android/base/Makefile.in
Hunk #1 FAILED at 238
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/Makefile.in.rej
unable to find 'mobile/android/base/db/FormHistoryProvider.java.in' for patching
3 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/db/FormHistoryProvider.java.in.rej
unable to find 'mobile/android/base/db/PasswordsProvider.java.in' for patching
5 out of 5 hunks FAILED -- saving rejects to file mobile/android/base/db/PasswordsProvider.java.in.rej
patching file mobile/android/base/db/PerProfileContentProvider.java.in
Hunk #1 FAILED at 29
Hunk #2 FAILED at 121
Hunk #3 FAILED at 199
Hunk #4 FAILED at 220
Hunk #5 FAILED at 257
Hunk #6 FAILED at 282
Hunk #7 FAILED at 321
7 out of 7 hunks FAILED -- saving rejects to file mobile/android/base/db/PerProfileContentProvider.java.in.rej
mobile/android/base/db/GeckoProvider.java.in not tracked!
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 731491

Comment 29

[Federico Paolinelli]

Attachment: Patch for GeckoProvider refactoring (6th version)

Added the changes needed to merge back. The preprocessing of the .java.in files was removed in the meanwhile. Should work now.

Comment 30

[Federico Paolinelli]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> applying 731491
> patching file mobile/android/base/Makefile.in
> Hunk #1 FAILED at 238
> 1 out of 1 hunks FAILED -- saving rejects to file
> mobile/android/base/Makefile.in.rej
> unable to find 'mobile/android/base/db/FormHistoryProvider.java.in' for
> patching
> 3 out of 3 hunks FAILED -- saving rejects to file
> mobile/android/base/db/FormHistoryProvider.java.in.rej
> unable to find 'mobile/android/base/db/PasswordsProvider.java.in' for
> patching
> 5 out of 5 hunks FAILED -- saving rejects to file
> mobile/android/base/db/PasswordsProvider.java.in.rej
> patching file mobile/android/base/db/PerProfileContentProvider.java.in
> Hunk #1 FAILED at 29
> Hunk #2 FAILED at 121
> Hunk #3 FAILED at 199
> Hunk #4 FAILED at 220
> Hunk #5 FAILED at 257
> Hunk #6 FAILED at 282
> Hunk #7 FAILED at 321
> 7 out of 7 hunks FAILED -- saving rejects to file
> mobile/android/base/db/PerProfileContentProvider.java.in.rej
> mobile/android/base/db/GeckoProvider.java.in not tracked!
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working dir
> errors during apply, please fix and refresh 731491

My fault. Did not update before submitting the patch and did not notice that some filenames changed. The version I added should be ok. I'll re-add checkin-needed.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/101bd6c508b9

Thanks for the patch, Federico! This bug will be resolved once it is merged over to mozilla-central, probably later today.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/101bd6c508b9