725881 - Content provider for form history

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Allison Naaktgeboren :ally]

needed to write the forms sync engine, required for release.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes is looking into this. I made "tracking-fennec:11+" because all of our "want for release" are set that way. We can juggle those later.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Tests for this and password provider

These are really simple really dumb tests for this feature and the password provider. They add an entry using the ContentProvider and then use SQLiteBridge to check it was added. Then they update and delete the entry and check the results in both those cases as well.

Need to update the password tests to handle crypto. Probably should also add similar tests for the deleted passwords and delted formhistory tables. But its a start!

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

FormHistory provider. I want to give this another look through when I'm not tired, but works fine. Abstracts out the bits of password provider that are copied. That may not work as well with crypto in the way now that I think of it... will have to add some special machinery...

Comment 4

[Richard Newman [:rnewman]]

Wes, can we get the two authorities smushed together? It seems like a different query URI, not a different authority, is the right approach for this.

Comment 5

[David Chan [:dchan]]

Tracking this for a potential review from our side

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch

Still need to go back and tweak the authority rnewman... hopefully can in a bit, but wanted to save these somewhere because I have an awful habit of overwriting and ruining perfectly good patches.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Tests

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Deleted table

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Deleted table

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Deleted table

Patch for this with tests. These pass on desktop for me. The downgrade case was a bit more finicky than passwords because passwords only checks for some of its tables during downgrade (and throws if it can't find them). Form history checks for all of its tables.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Path 1/2 - Form history provider

This is the form history provider patch. It splits the password provider so that it now extends a GeckoProvider class which holds some basic sqlite calling stuff. I also removed some of the SyncColumn pieces because I'm not sure they're needed. I think there may be some more cruft in here as well since this all originally started as the bookmarks/history provider. Or maybe we really need that stuff?

I thought lucasr could look at this since he's got some know how in that area.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests for both of these providers. Really simple ones. Just insert, update, and delete, but its a start!

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 598083 [details] [diff] [review]
Tests

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

I'd like to have another look at this. It would be good if we could avoid duplicating a lot of code for every database test, even if that means having to move things around a bit.

::: build/mobile/robocop/FennecMochitestAssert.java.in
@@ +190,5 @@
> +          pass = !a.equals(b);
> +        }
> +        String aString = (a == null) ? "null" : a.toString();
> +        String bString = (b == null) ? "null" : b.toString();
> +    

There is a lot of shared logic between these 2 functions, so consider pulling something out (equalObject? objToString?).

@@ +247,5 @@
> +            pass = !a.equals(b);
> +        }
> +        String aString = (a == null) ? "null" : a.toString();
> +        String bString = (b == null) ? "null" : b.toString();
> +    

Same remark as before. They should be able to use the functions you pulled out above.

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +33,1 @@
>  

If we want to have SQLiteBridge emulate SQLiteDatabase as much as possible in the long run, I'd avoid exposing internals as much as possible. Better add a getColumnNames(). Even if its not in a Cursor.

::: mobile/android/base/tests/testFormHistory.java.in
@@ +64,5 @@
> +      uri = cr.insert(fhuri, cvs[0]);
> +      SqliteCompare("SELECT * FROM moz_formhistory", cvs);
> +      mAsserter.is("content://org.mozilla.fennec_wesj.db.formhistory/formhistory/1?profilePath=%2Fmnt%2Fsdcard%2Ftests%2Fprofile",
> +                   uri.toString(), "Insert returned null correctly");
> +

org.mozilla.fennec_wesj? This doesn't look right.

::: mobile/android/base/tests/testPasswordProvider.java.in
@@ +81,5 @@
> +    }
> +
> +    @SuppressWarnings({"unchecked", "non-varargs"})
> +    public void SqliteCompare(String sqlCommand, ContentValues[] cvs) {
> +        BaseTest.SqlResults r = querySql("signons.sqlite", sqlCommand);

Again, it would be cool if we could avoid the exact duplication here.

::: mobile/android/chrome/content/browser.js
@@ +995,3 @@
>        sendMessageToJava({gecko: { type: "Passwords:Init:Return" }});
> +    } else if (aTopic == "FormHistory:Init") {
> +      dump("INIT:XXXXXXXXXXXXXXXXXXXXXXXX");

Clean this up.

::: mozglue/android/SQLiteBridge.cpp
@@ +44,5 @@
>  #include "ElfLoader.h"
>  #endif
>  #include "SQLiteBridge.h"
>  
> +#define DEBUG 1

And this.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Form history provider

Grr. I really hate juggling these patches. Some of those pieces in the tests patch should have been here. Also adds a separate form history permission which I think sync told me they want.

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Tests

Addresses comments I think. Also realized that I should probably loop geoff or someone from there in since I'm changing their asserters.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Tests

And I should also qref before uploading...

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 598078 [details] [diff] [review]
Patch 2/2 - Deleted table

Simplifying this and putting in the #ifdef ANDROID I forgot....

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Deleted table

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Deleted table

Sorry for the churn. Forgot the pre-processor bits (again).

Comment 20

[Geoff Brown [:gbrown]]

Comment on attachment 598282 [details] [diff] [review]
Tests

jmaher: There are some changes to your asserter class here -- any concerns?

Comment 21

[Geoff Brown [:gbrown]]

Comment on attachment 598282 [details] [diff] [review]
Tests

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

I am mostly concerned about the potential race conditions and the failing assertion.

::: build/mobile/robocop/FennecMochitestAssert.java.in
@@ -210,2 +202,22 @@
> > -        boolean pass = a.equals(b);
> > +        boolean pass = checkObjectsEqual(a,b);
> > -        String diag = "got " + a.toString() + ", expected " + b.toString();
> > +        todo(pass, name, getEqualString(a,b,pass));
> > +    }
> > +
NaN more ...

nit - couldn't you just write:
  String diag = "got " + a + ", expected " + b;
? I think java takes care of the null case just fine.

::: mobile/android/base/tests/BaseTest.java.in
@@ -47,1 +55,1 @@
> >          i.putExtra("args", "-no-remote -profile " + (String)config.get("profile"));

nit - use mProfile here

::: mobile/android/base/tests/testFormHistory.java.in
@@ +9,5 @@
> +import android.content.Context;
> +import android.net.Uri;
> +import java.io.File;
> +import java.lang.ClassLoader;
> +import android.util.Log;

nit - remove unused imports

@@ +15,5 @@
> +import java.lang.reflect.InvocationTargetException;
> +import java.lang.reflect.Constructor;
> +import java.util.ArrayList;
> +
> +public class testFormHistory extends BaseTest {

nit - add a header comment, like that in testLoad

@@ +57,5 @@
> +      mAsserter.is(uri, null, "Insert returned null correctly");
> +
> +      // This should fail the first time round because there is no database
> +      // Wait for gecko to reply and then we'll try again
> +      contentEventExpecter = mActions.expectGeckoEvent("FormHistory:Init:Return");

Race here?

@@ +103,5 @@
> +              if (CursorMatches(resultRow, r.columns, cvs[i])) {
> +                found = true;
> +              }
> +            }
> +            mAsserter.is(found, true, "Password was found");

This assertion consistently fails for me (testing on a Galaxy S):

org.mozilla.fennec_mozdev.tests.testFormHistory:INSTRUMENTATION_RESULT: shortMsg=java.lang.NullPointerException
INSTRUMENTATION_RESULT: longMsg=java.lang.NullPointerException: Unable to pause activity {org.mozilla.fennec_mozdev/org.mozilla.fennec_mozdev.App}: java.lang.NullPointerException
INSTRUMENTATION_CODE: 0
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testFormHistory
2 INFO TEST-PASS | testFormHistory | Starting - true should equal true
3 INFO TEST-PASS | testFormHistory | Insert returned null correctly - null should equal null
4 INFO TEST-PASS | testFormHistory | List is correct length - 1 should equal 1
5 INFO TEST-UNEXPECTED-FAIL | testFormHistory | Password was found - got false, expected true
6 INFO TEST-END | testFormHistory | finished in 24250ms
7 INFO TEST-START | Shutdown
8 INFO Passed: 3
9 INFO Failed: 1
10 INFO Todo: 0
11 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:00:16.488632
INFO | automation.py | Reading PID log: /tmp/tmpVDUg73pidlog
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.

----

E/AndroidRuntime( 4233): FATAL EXCEPTION: main
E/AndroidRuntime( 4233): java.lang.RuntimeException: Unable to pause activity {org.mozilla.fennec_mozdev/org.mozilla.fennec_mozdev.App}: java.lang.NullPointerException
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.performPauseActivity(ActivityThread.java:3348)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.performPauseActivity(ActivityThread.java:3305)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.handlePauseActivity(ActivityThread.java:3288)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.access$2500(ActivityThread.java:125)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2040)
E/AndroidRuntime( 4233): 	at android.os.Handler.dispatchMessage(Handler.java:99)
E/AndroidRuntime( 4233): 	at android.os.Looper.loop(Looper.java:123)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.main(ActivityThread.java:4627)
E/AndroidRuntime( 4233): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime( 4233): 	at java.lang.reflect.Method.invoke(Method.java:521)
E/AndroidRuntime( 4233): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:858)
E/AndroidRuntime( 4233): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
E/AndroidRuntime( 4233): 	at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime( 4233): Caused by: java.lang.NullPointerException
E/AndroidRuntime( 4233): 	at org.mozilla.gecko.GeckoApp$SessionSnapshotRunnable.run(GeckoApp.java:576)
E/AndroidRuntime( 4233): 	at org.mozilla.gecko.GeckoApp.onSaveInstanceState(GeckoApp.java:561)
E/AndroidRuntime( 4233): 	at android.app.Activity.performSaveInstanceState(Activity.java:1036)
E/AndroidRuntime( 4233): 	at android.app.Instrumentation.callActivityOnSaveInstanceState(Instrumentation.java:1182)
E/AndroidRuntime( 4233): 	at android.app.ActivityThread.performPauseActivity(ActivityThread.java:3330)
E/AndroidRuntime( 4233): 	... 12 more
I/Process (22074): Sending signal. PID: 4233 SIG: 9
W/ActivityManager(22074): Error in app org.mozilla.fennec_mozdev running instrumentation ComponentInfo{org.mozilla.roboexample.test/android.test.InstrumentationTestRunner}:
W/ActivityManager(22074):   java.lang.NullPointerException
W/ActivityManager(22074):   java.lang.NullPointerException: Unable to pause activity {org.mozilla.fennec_mozdev/org.mozilla.fennec_mozdev.App}: java.lang.NullPointerException
I/ActivityManager(22074): Force stopping package org.mozilla.fennec_mozdev uid=10085
D/AndroidRuntime( 4227): Shutting down VM

::: mobile/android/base/tests/testPasswordProvider.java.in
@@ +8,5 @@
> +import android.database.Cursor;
> +import android.content.Context;
> +import android.net.Uri;
> +import java.io.File;
> +import android.util.Log;

nit - remove unused imports

@@ +11,5 @@
> +import java.io.File;
> +import android.util.Log;
> +import java.util.ArrayList;
> +
> +public class testPasswordProvider extends BaseTest {

nit - A brief header comment - like that in testLoad - would be appreciated.

@@ +13,5 @@
> +import java.util.ArrayList;
> +
> +public class testPasswordProvider extends BaseTest {
> +    public void testPasswordProvider() {
> +      setTestType("mochitest");

nit - Use 4 space indents please.

@@ +24,5 @@
> +      contentEventExpecter.blockForEvent();
> +
> +      Uri pwuri;
> +      try {
> +        mAsserter.is(true, true, "Starting");

Consider using mAsserter.dumpLog instead.

@@ +54,5 @@
> +      }
> +      Uri uri = cr.insert(pwuri, cvs[0]);
> +      mAsserter.is(uri, null, "Insert returned null correctly");
> +
> +      // This should fail the first time round because there is no pw database

nit - It took me a minute to understand what "This" was here -- can you make it more obvious?

@@ +56,5 @@
> +      mAsserter.is(uri, null, "Insert returned null correctly");
> +
> +      // This should fail the first time round because there is no pw database
> +      // Wait for gecko to reply and then we'll try again
> +      contentEventExpecter = mActions.expectGeckoEvent("Passwords:Init:Return");

There is a race here, isn't there? If the cr.insert kicks off the event, move the expectGeckoEvent call before the cr.insert.

@@ +60,5 @@
> +      contentEventExpecter = mActions.expectGeckoEvent("Passwords:Init:Return");
> +      contentEventExpecter.blockForEvent();
> +
> +      uri = cr.insert(pwuri, cvs[0]);
> +      SqliteCompare("SELECT * FROM moz_logins", cvs);

Check that uri is non-null here?

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 598282 [details] [diff] [review]
Tests

My additional feedback is to make the files 4 space indentation, not 2 space or mixed.  Also we are adding the sqlQuery stuff to BaseTest, I think that would be better in FennecNativeActions.java.in

Comment 23

[Gian-Carlo Pascutto [:gcp]]

I ran into Bug 729086 while (attempting to) review this.

- checkObjectsEqual will fail if a is null
- You should be able to write checkObjectsNotEqual as just !checkObjectsEqual, if not for bugs like the above

- When I asked not to expose SQLiteBridge internals, I was sortof hoping you'd expose "String[] getColumnNames()", which is what Cursor is defined to have, instead of the ArrayList. This should make switching easier.

- Class fh, Uri fhuri, cvs: This is perhaps just a bit too terse. 

- public void SqliteCompare
- public boolean CursorMatches
- public void tearDown()

These are repeated and identical in both tests. No way to share them?

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 598276 [details] [diff] [review]
Patch 1/2 - Form history provider

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

I like the direction of the patch. Just needs cleanups, questions answered, and nits fixed. I'd love if this patch was split into smaller self-contained steps :-)

::: mobile/android/base/AndroidManifest.xml.in
@@ +169,5 @@
>  									android:protectionLevel="signature"/>
>  
> +        <provider android:name="@ANDROID_PACKAGE_NAME@.db.FormHistoryProvider"
> +                  android:authorities="@ANDROID_PACKAGE_NAME@.db.formhistory"
> +                  android:permission="@ANDROID_PACKAGE_NAME@.permissions.FORMHISTORY_PROVIDER"

Why do you need a separate permission here? Isn't it pretty much the same as permissions.BROWSER_PROVIDER?

::: mobile/android/base/db/BrowserContract.java.in
@@ +43,5 @@
>  public class BrowserContract {
>      public static final String AUTHORITY = "@ANDROID_PACKAGE_NAME@.db.browser";
>  
>      public static final String PASSWORDS_AUTHORITY = "@ANDROID_PACKAGE_NAME@.db.passwords";
> +    public static final String DELETED_PASSWORDS_AUTHORITY = "@ANDROID_PACKAGE_NAME@.db.deleted-passwords";

How is this DELETED_PASSWORDS_AUTHORITY related to this patch? Feels like it should go on a separate patch.

@@ +51,5 @@
>  
>      public static final Uri AUTHORITY_URI = Uri.parse("content://" + AUTHORITY);
>      
>      public static final Uri PASSWORDS_AUTHORITY_URI = Uri.parse("content://" + PASSWORDS_AUTHORITY);
> +    public static final Uri DELETED_PASSWORDS_AUTHORITY_URI = Uri.parse("content://" + DELETED_PASSWORDS_AUTHORITY);

Ditto.

@@ +59,5 @@
>  
>      public static final String DEFAULT_PROFILE = "default";
>  
>      public static final String PARAM_PROFILE = "profile";
> +    public static final String PARAM_PROFILE_PATH = "profilePath";

I think I need more background on why just using the profile name isn't enough here. What are the cases where profilePath is necessary?

@@ +206,5 @@
> +
> +    public static final class FormHistory implements CommonColumns, SyncColumns {
> +        private FormHistory() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(FORMHISTORY_AUTHORITY_URI, "formhistory");
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/formhistory";

Add empty lines between each property to be consistent with the rest of this file.

@@ +208,5 @@
> +        private FormHistory() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(FORMHISTORY_AUTHORITY_URI, "formhistory");
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/formhistory";
> +        public static final String ID = "id";
> +        public static final String FIELDNAME = "fieldname";

FIELD_NAME?

@@ +210,5 @@
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/formhistory";
> +        public static final String ID = "id";
> +        public static final String FIELDNAME = "fieldname";
> +        public static final String VALUE = "value";
> +        public static final String TIMESUSED = "timesUsed";

TIME_USED?

@@ +211,5 @@
> +        public static final String ID = "id";
> +        public static final String FIELDNAME = "fieldname";
> +        public static final String VALUE = "value";
> +        public static final String TIMESUSED = "timesUsed";
> +        public static final String FIRSTUSED = "firstUsed";

FIRST_USED?

@@ +212,5 @@
> +        public static final String FIELDNAME = "fieldname";
> +        public static final String VALUE = "value";
> +        public static final String TIMESUSED = "timesUsed";
> +        public static final String FIRSTUSED = "firstUsed";
> +        public static final String LASTUSED = "lastUsed";

LAST_USED?

@@ +217,5 @@
> +        public static final String GUID = "guid";
> +    }
> +
> +    public static final class DeletedFormHistory implements CommonColumns, SyncColumns {
> +        private DeletedFormHistory() {}

Empty line between all members of the class.

@@ +220,5 @@
> +    public static final class DeletedFormHistory implements CommonColumns, SyncColumns {
> +        private DeletedFormHistory() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(DELETED_FORMHISTORY_AUTHORITY_URI, "deleted-formhistory");
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/deleted-formhistory";
> +        public static final String ID = "id";

Same comment about empty lines between properties.

@@ +221,5 @@
> +        private DeletedFormHistory() {}
> +        public static final Uri CONTENT_URI = Uri.withAppendedPath(DELETED_FORMHISTORY_AUTHORITY_URI, "deleted-formhistory");
> +        public static final String CONTENT_TYPE = "vnd.android.cursor.dir/deleted-formhistory";
> +        public static final String ID = "id";
> +        public static final String GUID = "guid";

SyncColumns brings GUID already. Why overriding it?

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

No list of contributors is needed anymore?

@@ +44,5 @@
> +public class FormHistoryProvider extends GeckoProvider {
> +    static final String TABLE_FORMHISTORY = "moz_form_history";
> +    static final String TABLE_DELETED_FORMHISTORY = "moz_deleted_formhistory";
> +
> +    private static final int FORMHISTORY = 600;

This should be 100..1XX as those indexes only apply to the content provider you're dealing with here.

@@ +45,5 @@
> +    static final String TABLE_FORMHISTORY = "moz_form_history";
> +    static final String TABLE_DELETED_FORMHISTORY = "moz_deleted_formhistory";
> +
> +    private static final int FORMHISTORY = 600;
> +    private static final int DELETED_FORMHISTORY = 602;

101?

@@ +60,5 @@
> +        URI_MATCHER.addURI(BrowserContract.DELETED_FORMHISTORY_AUTHORITY, "deleted-formhistory", DELETED_FORMHISTORY);
> +
> +        FORMHISTORY_PROJECTION_MAP = new HashMap<String, String>();
> +
> +        URI_MATCHER.addURI(BrowserContract.DELETED_FORMHISTORY_AUTHORITY, "deleted-formhistory", DELETED_FORMHISTORY);

Duplicate?

@@ +69,5 @@
> +    @Override
> +    public boolean onCreate() {
> +        LOGTAG = "FormHistoryProvider";
> +        DATABASE_NAME = "formhistory.sqlite";
> +        DATABASE_VERSION = 4;

It's a bit odd that you're setting constant-like members here. The all-upper-case used here is a bit misleading. I'd suggest you to add setters/getters of those and use them here, for clarity reasons.

@@ +88,5 @@
> +        Log.d(LOGTAG, "URI has unrecognized type: " + uri);
> +        return null;
> +    }
> +
> +    public String getTable(Uri uri) {

I guess this method should return null on error? Declare a String table = null at the beginning where and set the value accordingly on each case, then return table.

@@ +92,5 @@
> +    public String getTable(Uri uri) {
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +            case DELETED_FORMHISTORY:
> +                return TABLE_DELETED_FORMHISTORY;

Add empty lines between case's

@@ +93,5 @@
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +            case DELETED_FORMHISTORY:
> +                return TABLE_DELETED_FORMHISTORY;
> +            case FORMHISTORY: {

The {}'s are not needed here.

@@ +101,5 @@
> +                throw new UnsupportedOperationException("Unknown table " + uri);
> +        }
> +    }
> +
> +    public String getSortOrder(Uri uri, String aRequested) {

Add @Override for clarity?

@@ +103,5 @@
> +    }
> +
> +    public String getSortOrder(Uri uri, String aRequested) {
> +        if (!TextUtils.isEmpty(aRequested))
> +            return aRequested;

Add empty line here.

@@ +111,5 @@
> +    public Uri getAuthUri(Uri uri) {
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +            case DELETED_FORMHISTORY:
> +                return BrowserContract.DELETED_FORMHISTORY_AUTHORITY_URI;

Add empty line between case's.

@@ +112,5 @@
> +        final int match = URI_MATCHER.match(uri);
> +        switch (match) {
> +            case DELETED_FORMHISTORY:
> +                return BrowserContract.DELETED_FORMHISTORY_AUTHORITY_URI;
> +            case FORMHISTORY: {

The {}'s are not needed here.

@@ +123,5 @@
> +
> +    public void setupDefaults(Uri uri, ContentValues values)
> +            throws IllegalArgumentException {
> +        int match = URI_MATCHER.match(uri);
> +        long now = System.currentTimeMillis();

Add empty line.

@@ +132,5 @@
> +                // Deleted entries must contain a guid
> +                if (!values.containsKey(FormHistory.GUID)) {
> +                    throw new IllegalArgumentException("Must provide a GUID for a deleted form history");
> +                }
> +                break;

Add empty line between case's.

@@ +134,5 @@
> +                    throw new IllegalArgumentException("Must provide a GUID for a deleted form history");
> +                }
> +                break;
> +            case FORMHISTORY: {
> +

Remove empty line here.

@@ +149,5 @@
> +                throw new UnsupportedOperationException("Unknown insert URI " + uri);
> +        }
> +    }
> +
> +    private String translateColumn(String column) {

Where is this method used? This is looking a bit hacky... Why do you need to "translate" column names?

@@ +157,5 @@
> +            return column;
> +    }
> +
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));

Does this init handle database migrations?

@@ +158,5 @@
> +    }
> +
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
> +    }

Add empty line here.

@@ +159,5 @@
> +
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
> +    }
> +    public void onPreInsert(ContentValues values, Uri uri) { }

Align/indent the methods just as usual, with the } on a new line.

@@ +160,5 @@
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
> +    }
> +    public void onPreInsert(ContentValues values, Uri uri) { }
> +    public void onPreUpdate(ContentValues values, Uri uri) { }

Empty line between methods.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +41,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public abstract class GeckoProvider extends ContentProvider {
> +    protected Context mContext = null;

Move mContext together with mDatabasePerProfile, just for grouping things better.

@@ +44,5 @@
> +public abstract class GeckoProvider extends ContentProvider {
> +    protected Context mContext = null;
> +    protected String LOGTAG = "GeckoPasswordsProvider";
> +    protected String DATABASE_NAME = "";
> +    protected int DATABASE_VERSION = 0;

You shouldn't name those members with all upper case as this gives the wrong idea about them. They are not constants.

@@ +67,5 @@
> +                    Log.e(LOGTAG, "Gecko is not running ");
> +                    mBridge = null;
> +                    throw new UnsupportedOperationException("Need to launch Gecko to set password database up");
> +                }
> +            }

Add empty line.

@@ +72,5 @@
> +            Log.i(LOGTAG, "dbNeedsSetup = " + dbNeedsSetup);
> +            if (dbNeedsSetup) {
> +                Log.i(LOGTAG, "Sending init to gecko");
> +                mBridge = null;
> +                initGecko();

Is here where database migrations are handled? I couldn't find where this happens in your patch.

@@ +76,5 @@
> +                initGecko();
> +            }
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(LOGTAG, "Error getting database", ex);
> +        }

Add empty line here.

@@ +98,5 @@
> +                try {
> +                    db = getDB(getContext(), getDatabasePath(profile, profilePath));
> +                } catch(UnsupportedOperationException ex) {
> +                    Log.i(LOGTAG, "Gecko has not set the database up yet");
> +                    return db;

Explicitly return null here for clarity?

@@ +99,5 @@
> +                    db = getDB(getContext(), getDatabasePath(profile, profilePath));
> +                } catch(UnsupportedOperationException ex) {
> +                    Log.i(LOGTAG, "Gecko has not set the database up yet");
> +                    return db;
> +                }

Add empty line here?

@@ +109,5 @@
> +
> +        return db;
> +    }
> +
> +    private String getDatabasePath(String profile, String profilePath) {

This is looking weird. Why isn't the profile name enough here? In which cases would you need to hardcode/override the profilePath with something else?

@@ +127,5 @@
> +            }
> +        }
> +
> +        Log.d(LOGTAG, "Using path: " + profileDir.getPath());
> +        String databasePath = new File(profileDir, DATABASE_NAME).getAbsolutePath();

Add empty line here.

@@ +137,5 @@
> +        String profilePath = null;
> +
> +        if (uri != null)
> +            profile = uri.getQueryParameter(BrowserContract.PARAM_PROFILE);
> +        if (uri != null)

Put these under the same if?

@@ +155,5 @@
> +    public String getType(Uri uri) {
> +        return null;
> +    }
> +
> +    public abstract String getTable(Uri uri);

Add empty lines between all members for better readability and consistency. Maybe move them to the end of the class?

@@ +161,5 @@
> +    public abstract Uri getAuthUri(Uri uri);
> +    public abstract void setupDefaults(Uri uri, ContentValues values)
> +            throws IllegalArgumentException;
> +    public abstract void initGecko();
> +    public abstract void onPreInsert(ContentValues values, Uri uri);

Are all those abstract methods actually used? For instance, I couldn't see any call to onPreInsert in your patch. Remove all unused abstract methods here.

@@ +173,5 @@
> +        if (db == null)
> +            return deleted;
> +
> +        String table = getTable(uri);
> +        if (table.equals(""))

Do you actually need to handle this case? I mean, if you don't have a matching table, is this an expected case or a bug? I guess it's the latter as you're throwing when no table name could be defined. In that case, no need to cover the empty table name here.

@@ +189,5 @@
> +    @Override
> +    public Uri insert(Uri uri, ContentValues values) {
> +        long id = -1;
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)

Aren't you hiding a bug if you do this null check here? Is db expected to be null in some cases? You should add comment explaining that if it's an expected situation.

@@ +192,5 @@
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)
> +            return null;
> +
> +        String table = getTable(uri);

Same comment about handling empty table name above.

@@ +197,5 @@
> +        if (table.equals(""))
> +            return null;
> +
> +        try {
> +            setupDefaults(uri, values);

Is error on setting defaults a bug or an expected situation? If it's a bug, you should not hide it and let it happen (without causing any data loss of course).

@@ +206,5 @@
> +
> +        onPreInsert(values, uri);
> +
> +        try {
> +            id = db.insert(table, "", values);

I think you better set a column name as second argument here, no? Or just set it to null?

@@ +219,5 @@
> +    public int update(Uri uri, ContentValues values, String selection,
> +            String[] selectionArgs) {
> +        int updated = 0;
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)

Same comment about db == null above applies here.

@@ +222,5 @@
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)
> +            return updated;
> +
> +        String table = getTable(uri);

Ditto (see comment about table name above).

@@ +232,5 @@
> +        try {
> +            return db.update(table, values, selection, selectionArgs);
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(LOGTAG, "Error updating table", ex);
> +        }

Add empty line here.

@@ +233,5 @@
> +            return db.update(table, values, selection, selectionArgs);
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(LOGTAG, "Error updating table", ex);
> +        }
> +        return 0;

return updated here and remove the return call above? More readable I guess.

@@ +241,5 @@
> +    public Cursor query(Uri uri, String[] projection, String selection,
> +            String[] selectionArgs, String sortOrder) {
> +        Cursor cursor = null;
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)

Ditto.

@@ +245,5 @@
> +        if (db == null)
> +            return cursor;
> +
> +        String table = getTable(uri);
> +        if (table.equals(""))

Ditto.

@@ +255,5 @@
> +
> +        try {
> +            cursor = db.query(table, projection, selection, selectionArgs, null, null, sortOrder, null);
> +            onPostQuery(cursor, uri);
> +            cursor.setNotificationUri(getContext().getContentResolver(), getAuthUri(uri));

A bit of background on why this is necessary would be helpful here. Add a comment explaining.

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +45,2 @@
>      static final String TABLE_PASSWORDS = "moz_logins";
> +    static final String TABLE_DELETED_PASSWORDS = "moz_deleted_logins";

This shouldn't be landing with this patch I guess?

@@ +89,5 @@
>      @Override
>      public boolean onCreate() {
> +        LOGTAG = "GeckoPasswordsProvider";
> +        DATABASE_NAME = "signons.sqlite";
> +        DATABASE_VERSION = 5;

See same comment about those constant-like members on FormHistoryProvider.

@@ +113,4 @@
>          final int match = URI_MATCHER.match(uri);
>          switch (match) {
>              case DELETED_PASSWORDS:
>                  return TABLE_DELETED_PASSWORDS;

Empty line between case's.

@@ +115,5 @@
>              case DELETED_PASSWORDS:
>                  return TABLE_DELETED_PASSWORDS;
>              case PASSWORDS: {
>                  return TABLE_PASSWORDS;
>              }

The {}'s are not needed here.

@@ +128,5 @@
>  
>          final int match = URI_MATCHER.match(uri);
>          switch (match) {
>              case DELETED_PASSWORDS:
>                  return DEFAULT_DELETED_PASSWORDS_SORT_ORDER;

Empty lines between case's.

@@ +131,5 @@
>              case DELETED_PASSWORDS:
>                  return DEFAULT_DELETED_PASSWORDS_SORT_ORDER;
>              case PASSWORDS: {
>                  return DEFAULT_PASSWORDS_SORT_ORDER;
>              }

The {}'s are not needed here.

@@ +137,5 @@
>                  throw new UnsupportedOperationException("Unknown delete URI " + uri);
>          }
>      }
>  
> +    public Uri getAuthUri(Uri uri) {

What's this authUri about?

@@ +142,4 @@
>          final int match = URI_MATCHER.match(uri);
>          switch (match) {
>              case DELETED_PASSWORDS:
>                  return BrowserContract.DELETED_PASSWORDS_AUTHORITY_URI;

Empty line between case's.

@@ +144,5 @@
>              case DELETED_PASSWORDS:
>                  return BrowserContract.DELETED_PASSWORDS_AUTHORITY_URI;
>              case PASSWORDS: {
>                  return BrowserContract.PASSWORDS_AUTHORITY_URI;
>              }

The {}'s are not needed here.

@@ +206,5 @@
>      }
> +
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Passwords:Init", null));
> +    }

Empty line here.

@@ +207,5 @@
> +
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Passwords:Init", null));
> +    }
> +    public void onPreInsert(ContentValues values, Uri uri) { };

Align/indent those empty methods are usual with } on a new line. Empty line between methods.

::: mobile/android/chrome/content/browser.js
@@ +221,5 @@
>      Services.obs.addObserver(this, "FullScreen:Exit", false);
>      Services.obs.addObserver(this, "Viewport:Change", false);
>      Services.obs.addObserver(this, "SearchEngines:Get", false);
>      Services.obs.addObserver(this, "Passwords:Init", false);
> +    Services.obs.addObserver(this, "FormHistory:Init", false);

No need to remove the observer anywhere?

Comment 25

[Richard Newman [:rnewman]]

(In reply to Lucas Rocha (:lucasr) from comment #24)

> Why do you need a separate permission here? Isn't it pretty much the same as
> permissions.BROWSER_PROVIDER?

I asked for that for granularity's sake. I wouldn't necessarily want to give a bookmarks widget access to my form history, or my passwords.

"One permission per authority" seems like a reasonable rule of thumb.

> No list of contributors is needed anymore?

Not for MPL2.0.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Form history provider

> I like the direction of the patch. Just needs cleanups, questions answered,
> and nits fixed. I'd love if this patch was split into smaller self-contained
> steps :-)
I'm trying! I just tend to forget to switch patches a lot in those cases and I usually wind up with a mess.

> Why do you need a separate permission here? Isn't it pretty much the same as
> permissions.BROWSER_PROVIDER?
Richard answered this, but sync requested it.

> How is this DELETED_PASSWORDS_AUTHORITY related to this patch? Feels like it
> should go on a separate patch.

Removed.

> I think I need more background on why just using the profile name isn't
> enough here. What are the cases where profilePath is necessary?

This is required for testing. For tests, the profile isn't in our data directory, something Gecko deals with fine, but our ContentProviders don't. I've revised this code a lot.

> Add empty lines between each property to be consistent with the rest of this
> file.

Done. I have a separate patch to remove most of them.
 
> FIELD_NAME?
> TIME_USED?
> FIRST_USED?
> LAST_USED?

Done.

> SyncColumns brings GUID already. Why overriding it?
We don't support SyncColumns here. I removed it along with CommonColumns which I think we don't need either?

> This should be 100..1XX as those indexes only apply to the content provider
> you're dealing with here.

Done.

> It's a bit odd that you're setting constant-like members here. The
> all-upper-case used here is a bit misleading. I'd suggest you to add
> setters/getters of those and use them here, for clarity reasons.

I agree. Cleaned this up.

> I guess this method should return null on error? Declare a String table =
> null at the beginning where and set the value accordingly on each case, then
> return table.

We should either succeed or throw I think, but this will now return null if we slip through.

> Where is this method used? This is looking a bit hacky... Why do you need to
> "translate" column names?

This was left over from the original port from BrowserProvider. I've been nervous that for some reason Sync expects us to do this, but rnewman tells me that he doesn't know of any reason, so I removed it now.

> Does this init handle database migrations?
Gecko should do the migration for us when we init the service (i.e. when we send Passwords:Init or FormHistory:Init). This should occur if the database doesn't exist, or if it exists but is the wrong version. We test the former in these tests. It would be nice and not to hard to test the later as well.

> You shouldn't name those members with all upper case as this gives the wrong
> idea about them. They are not constants.
Fixed. Thanks :)

> Is here where database migrations are handled? I couldn't find where this
> happens in your patch.
See above.

> Explicitly return null here for clarity?

> This is looking weird. Why isn't the profile name enough here? In which
> cases would you need to hardcode/override the profilePath with something
> else?

I cleaned this up a bit. We now have getDatabaseForProfile and getDatabaseForPath methods that we call into.

> Are all those abstract methods actually used? For instance, I couldn't see
> any call to onPreInsert in your patch. Remove all unused abstract methods
> here.

These are needed by other patches. It seemed silly to not create them here...

> Do you actually need to handle this case? I mean, if you don't have a
> matching table, is this an expected case or a bug? I guess it's the latter
> as you're throwing when no table name could be defined. In that case, no
> need to cover the empty table name here.

Good point. Removed these table checks.

> @@ +189,5 @@
> > +    @Override
> > +    public Uri insert(Uri uri, ContentValues values) {
> > +        long id = -1;
> > +        final SQLiteBridge db = getDatabase(uri);
> > +        if (db == null)
> 
> Aren't you hiding a bug if you do this null check here? Is db expected to be
> null in some cases? You should add comment explaining that if it's an
> expected situation.

Hmm... right now db will be null if we can't get it for some reason. Any reason really. Its a bit... broad. But I think most of them are expected reasons. If there is no database at this location yet (i.e. SQLiteBridge will throw an exception), or the database that is there is the wrong version. We're logging the reason for the failure. Callers will still not know why we're failing. They should just receive null for inserts or queries (maybe query should return an empty cursor instead?) and zero for deletes or updates that fail. I think we can try to address that in a follow up if Sync wants something more advanced?

> Is error on setting defaults a bug or an expected situation? If it's a bug,
> you should not hide it and let it happen (without causing any data loss of
> course).

I think we're ok here as well. SetupDefaults() should throw if it runs into errors. Removed this check.

> I think you better set a column name as second argument here, no? Or just
> set it to null?

I set it to null. SQLiteBridge doesn't support NullColumnParameter yet.

> > + cursor.setNotificationUri(getContext().getContentResolver(), getAuthUri(uri));
> A bit of background on why this is necessary would be helpful here. Add a
> comment explaining.

Heh. I don't think we actually really need this (yet). I removed it. We could use this to send notifications if Gecko updated the database though, right?

> > +    public Uri getAuthUri(Uri uri) {
> What's this authUri about?

Removed. Vestigial piece from some earlier thing.

> No need to remove the observer anywhere?

I guess we could assume this will only happen once per sessions and remove it after we get the message once. I'm not sure what that gains us, but I did it.

Comment 27

[Justin Dolske [:Dolske]]

Comment on attachment 598291 [details] [diff] [review]
Patch 2/2 - Deleted table

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

Just 2 teeny-tiny nits. Woot!

1) Have the upgrade test look for an existing entry, just to ensure the DB still works and the test doesn't accidentally pass if we reset the DB during the upgrade.

2) you probably should also have a test_db_update_v4b.js, which is basically the same except it's an v4 DB that's had its schema set to 3. (which is what happens if an older firefox touches the DB after it's been upgraded). Basically this is just a safety check to make sure when the v4 "upgrade" runs on it nothing explodes even though there's not actually work that happens.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 600090 [details] [diff] [review]
Patch 1/2 - Form history provider

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

Looking pretty good. I'd just like to see another go before giving r+.

::: mobile/android/base/AndroidManifest.xml.in
@@ +164,5 @@
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"/>
>  
>          <provider android:name="@ANDROID_PACKAGE_NAME@.db.PasswordsProvider"
>                    android:authorities="@ANDROID_PACKAGE_NAME@.db.passwords"
>                    android:permission="@ANDROID_PACKAGE_NAME@.permissions.BROWSER_PROVIDER"

Not in this patch but: if the plan is to add granularity to the permissions, I'd expect the passwords provider to have its own permission.

::: mobile/android/base/db/BrowserContract.java.in
@@ +53,5 @@
>      
>      public static final Uri PASSWORDS_AUTHORITY_URI = Uri.parse("content://" + PASSWORDS_AUTHORITY);
>  
>      public static final Uri DELETED_PASSWORDS_AUTHORITY_URI = Uri.parse("content://" + DELETED_PASSWORDS_AUTHORITY);
> +    public static final Uri FORMHISTORY_AUTHORITY_URI = Uri.parse("content://" + FORMHISTORY_AUTHORITY);

FORM_HISTORY_AUTHORITY maybe?

@@ +203,5 @@
>  
>          public static final String TIME_DELETED = "timeDeleted";
>      }
> +
> +    public static final class FormHistory implements CommonColumns {

Do you actually need FormHistory to implement CommonColumns?

@@ +225,5 @@
> +
> +        public static final String GUID = "guid";
> +    }
> +
> +    public static final class DeletedFormHistory implements CommonColumns {

Same here. Why do you need CommonColumns here if you have a local ID property defined below?

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +49,5 @@
> +    private static final int DELETED_FORMHISTORY = 101;
> +
> +    private static final UriMatcher URI_MATCHER;
> +
> +    private static HashMap<String, String> FORMHISTORY_PROJECTION_MAP;

FORM_HISTORY_PROJECTION_MAP?

@@ +50,5 @@
> +
> +    private static final UriMatcher URI_MATCHER;
> +
> +    private static HashMap<String, String> FORMHISTORY_PROJECTION_MAP;
> +    private static HashMap<String, String> DELETED_FORMHISTORY_PROJECTION_MAP;

DELETED_FORM_HISTORY_PROJECTION_MAP?

@@ +54,5 @@
> +    private static HashMap<String, String> DELETED_FORMHISTORY_PROJECTION_MAP;
> +
> +    static {
> +        URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);
> +

Remove empty line here?

@@ +56,5 @@
> +    static {
> +        URI_MATCHER = new UriMatcher(UriMatcher.NO_MATCH);
> +
> +        URI_MATCHER.addURI(BrowserContract.FORMHISTORY_AUTHORITY, "formhistory", FORMHISTORY);
> +        URI_MATCHER.addURI(BrowserContract.DELETED_FORMHISTORY_AUTHORITY, "deleted-formhistory", DELETED_FORMHISTORY);

Add empty line here?

@@ +65,5 @@
> +    @Override
> +    public boolean onCreate() {
> +        setLogTag("FormHistoryProvider");
> +        setDBName("formhistory.sqlite");
> +        setDBVersion(4);

Where does this version number come from? Maybe make it private constant for better visibility? Add empty line here?

@@ +81,5 @@
> +            case DELETED_FORMHISTORY:
> +                return DeletedFormHistory.CONTENT_TYPE;
> +        }
> +
> +        Log.d(getLogTag(), "URI has unrecognized type: " + uri);

I think you should throw something like UnsupportedOperationException here, no?

@@ +108,5 @@
> +    public String getSortOrder(Uri uri, String aRequested) {
> +        if (!TextUtils.isEmpty(aRequested))
> +            return aRequested;
> +
> +        return "";

Returning null is probably fine here.

@@ +131,5 @@
> +                if (!values.containsKey(FormHistory.GUID)) {
> +                    String guid = Utils.generateGuid();
> +                    values.put(FormHistory.GUID, guid);
> +                }
> +                String nowString = new Long(now).toString();

Unused?

::: mobile/android/base/db/GeckoProvider.java.in
@@ +41,5 @@
> +import android.text.TextUtils;
> +import android.util.Log;
> +
> +public abstract class GeckoProvider extends ContentProvider {
> +    private String LogTag = "GeckoPasswordsProvider";

mLogTag for consistency? I'd expect the default logtag to be GeckoProvider, no?

@@ +42,5 @@
> +import android.util.Log;
> +
> +public abstract class GeckoProvider extends ContentProvider {
> +    private String LogTag = "GeckoPasswordsProvider";
> +    private String dbName = "";

mDbName?

@@ +43,5 @@
> +
> +public abstract class GeckoProvider extends ContentProvider {
> +    private String LogTag = "GeckoPasswordsProvider";
> +    private String dbName = "";
> +    private int dbVersion = 0;

mDbVersion?

@@ +113,5 @@
> +            Log.d(LogTag, "No profile provided, using default");
> +            profile = BrowserContract.DEFAULT_PROFILE;
> +        }
> +
> +        SQLiteBridge db = mDatabasePerProfile.get(profile);

I think this .get(profile) call has be inside the synchronized block to avoid races, no?

@@ +160,5 @@
> +    private SQLiteBridge getDatabase(Uri uri) {
> +        String profile = null;
> +        String profilePath = null;
> +
> +        if (uri != null) {

What are the cases where uri is null?

@@ +168,5 @@
> +
> +        // Testing will specify the absolute profile path
> +        if (profilePath != null)
> +          return getDatabaseForPath(profilePath);
> +        return getDatabaseForProfile(profile);

Looks much cleaner now, nice!

@@ +174,5 @@
> +
> +    @Override
> +    public boolean onCreate() {
> +        mContext = getContext();
> +        mDatabasePerProfile = new HashMap<String, SQLiteBridge>();

Enclose this in a synchronized block? Just in case.

@@ +190,5 @@
> +        final SQLiteBridge db = getDatabase(uri);
> +        if (db == null)
> +            return deleted;
> +
> +        String table = getTable(uri);

Any reason for not using getTable(uri) directly in the db.delete() call? Because it might throw and you want to avoid hiding a bug here I guess?

@@ +204,5 @@
> +
> +    @Override
> +    public Uri insert(Uri uri, ContentValues values) {
> +        long id = -1;
> +        final SQLiteBridge db = getDatabase(uri);

Add empty line here.

@@ +214,5 @@
> +
> +        String table = getTable(uri);
> +
> +        setupDefaults(uri, values);
> +        onPreInsert(values, uri);

Given that both GeckoProvider subclasses have empty onPreInsert() implementations, why do you this at all?

@@ +229,5 @@
> +    @Override
> +    public int update(Uri uri, ContentValues values, String selection,
> +            String[] selectionArgs) {
> +        int updated = 0;
> +        final SQLiteBridge db = getDatabase(uri);

Add an empty line here.

@@ +238,5 @@
> +            return updated;
> +
> +        String table = getTable(uri);
> +
> +        onPreUpdate(values, uri);

Same here. Both GeckoProvider subclasses have empty onPreUpdate(). Why do you need it?

@@ +244,5 @@
> +        try {
> +            updated = db.update(table, values, selection, selectionArgs);
> +        } catch(SQLiteBridgeException ex) {
> +            Log.e(LogTag, "Error updating table", ex);
> +        }

Add empty line here.

@@ +252,5 @@
> +    @Override
> +    public Cursor query(Uri uri, String[] projection, String selection,
> +            String[] selectionArgs, String sortOrder) {
> +        Cursor cursor = null;
> +        final SQLiteBridge db = getDatabase(uri);

Add empty line here.

@@ +261,5 @@
> +            return cursor;
> +
> +        String table = getTable(uri);
> +        if (table.equals(""))
> +            return cursor;

Why checking for empty table only in query?

@@ +265,5 @@
> +            return cursor;
> +
> +        sortOrder = getSortOrder(uri, sortOrder);
> +        if (TextUtils.isEmpty(sortOrder))
> +            return cursor;

A bit too harsh no? I mean, the query should be fine without the sortOrder, right? Maybe just add a proper logging to let the called know that no sort order has been used?

@@ +269,5 @@
> +            return cursor;
> +
> +        try {
> +            cursor = db.query(table, projection, selection, selectionArgs, null, null, sortOrder, null);
> +            onPostQuery(cursor, uri);

Same here. GeckoProvider subclasses have empty implementations of onPostQuery(). Why do you need it?

::: mobile/android/base/db/PasswordsProvider.java.in
@@ +87,5 @@
>      @Override
>      public boolean onCreate() {
> +        setLogTag("GeckoPasswordsProvider");
> +        setDBName("signons.sqlite");
> +        setDBVersion(5);

Same here. Move that to a private constant for better visibility?

@@ +97,5 @@
>          final int match = URI_MATCHER.match(uri);
>  
>          switch (match) {
>              case PASSWORDS:
>                  return Passwords.CONTENT_TYPE;

Empty line between cases.

@@ +102,5 @@
>              case DELETED_PASSWORDS:
>                  return DeletedPasswords.CONTENT_TYPE;
>          }
>  
> +        Log.d(getLogTag(), "URI has unrecognized type: " + uri);

Same here. I think it's the case to throw here, no?

@@ +136,2 @@
>              default:
>                  throw new UnsupportedOperationException("Unknown delete URI " + uri);

Maybe a bit too harsh to throw here? Not sure.

Comment 29

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Form history provider

> Not in this patch but: if the plan is to add granularity to the permissions,
> I'd expect the passwords provider to have its own permission.
That's done in the crypto patches.

> FORM_HISTORY_AUTHORITY maybe?
I adjusted these.

> Same here. Why do you need CommonColumns here if you have a local ID
> property defined below?
Whoops. I removed this before and lost it somewhere (see how bad I am at multiple patches!)

> What are the cases where uri is null?

I have no idea. We check for that in BrowserProvider where all this started.

> Any reason for not using getTable(uri) directly in the db.delete() call?
> Because it might throw and you want to avoid hiding a bug here I guess?

Not really. Lets try!

> Given that both GeckoProvider subclasses have empty onPreInsert()
> implementations, why do you this at all?

These are needed in subsequent patches (i.e. Password crypto).

Comment 30

[Wesley Johnston (:wesj)]

Attachment: Tests

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 600458 [details] [diff] [review]
Patch 1/2 - Form history provider

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

::: mobile/android/base/db/FormHistoryProvider.java.in
@@ +146,5 @@
> +    public void initGecko() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("FormHistory:Init", null));
> +    }
> +
> +    public void onPreInsert(ContentValues values, Uri uri) {

I still think you should only add those (empty) methods once you actually implement them in a following patch.

::: mobile/android/base/db/GeckoProvider.java.in
@@ +47,5 @@
> +    private int mDBVersion = 0;
> +    private HashMap<String, SQLiteBridge> mDatabasePerProfile;
> +    protected Context mContext = null;
> +
> +    protected void setLogTag(String aLogTag) {

nit: We don't usually prefix args with 'a' in Java code. Not a big deal, just something to keep in mind.

@@ +166,5 @@
> +        profilePath = uri.getQueryParameter(BrowserContract.PARAM_PROFILE_PATH);
> +
> +        // Testing will specify the absolute profile path
> +        if (profilePath != null)
> +          return getDatabaseForPath(profilePath);

Add empty line liner.

Comment 32

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/289cd639ff34
https://hg.mozilla.org/integration/mozilla-inbound/rev/00929ec60991

Comment 33

[Geoff Brown [:gbrown]]

Comment on attachment 600479 [details] [diff] [review]
Tests

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

r+ with the logging issue resolved.

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +373,5 @@
> +            ensureSQLiteLibsLoaded.invoke(null, resourcePath);
> +            r.results = (ArrayList<Object[]>)query.invoke(bridge, sql);
> +            r.columns = (String[])getColumns.invoke(bridge);
> +        } catch(ClassNotFoundException ex) {
> +            Log.e(LOGTAG, "Error getting class", ex);

We are trying not to use Log directly in test or test harness code, since those messages will not appear in the mochitest log file. Use mAsserter.is(false, true, etc) or mAsserter.dumpLog or FennecNativeDriver.log instead.

Comment 34

[David Chan [:dchan]]

Comment on attachment 598291 [details] [diff] [review]
Patch 2/2 - Deleted table

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +387,4 @@
>              stmt = this.dbCreateStatement(query, params);
>              stmt.executeStep();
>              this.sendIntNotification("removeEntriesByTimeframe", beginTime, endTime);
>          } catch (e) {

Should there be a rollback here?

Comment 35

[Wesley Johnston (:wesj)]

Whoops. yep. Patch coming....

Comment 36

[Wesley Johnston (:wesj)]

Attachment: Follow up fix

Comment 37

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee92bbe9e78

Comment 38

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/00929ec60991
https://hg.mozilla.org/mozilla-central/rev/289cd639ff34
https://hg.mozilla.org/mozilla-central/rev/4ee92bbe9e78

Comment 39

[Lucas Rocha (:lucasr)]

Why hasn't the tests patch landed?

Comment 40

[Wesley Johnston (:wesj)]

I wanted to push it to try and make sure things were ok. Then I got distracted with the crash bug and since its on the bottom of my stack, I'm waiting for it to be ok before pushing (hopefully today)....

Comment 41

[Richard Newman [:rnewman]]

Would you prefer to reopen, or file a followup, for deciding how best to proceed with tracking records by modification time?

Comment 42

[Wesley Johnston (:wesj)]

Filed Bug 733422. Thanks for reminding me.

Comment 43

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32220d7085e0

Comment 44

[Matt Brubeck (:mbrubeck)]

Follow-up fix was backed out on inbound to investigate XUL Fennec crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5940fe8d1af9

Comment 45

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f34baebe1865

landing things more piecemeal this time to avoid having to do a large backout.

Comment 46

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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