726821 - SQLiteBridge should return a cursor

Status: RESOLVED FIXED

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

Description

[Wesley Johnston (:wesj)]

Right now the bridge returns an ArrayList which we (sometimes) convert to a cursor. It also sends and fills in a second ArrayList for columns. Ideally, we'd just send a cursor that could handle both.

We also need a separate data bit for changed or inserted rows. That would help differentiating between queries that return zero results and queries that should not return a result (i.e. updates, inserts, and deletes).

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Add an extra argument to SQLite call. Cleanup some stuff.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Update profile migration to use the Cursor API.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 3. Push down Cursor construction into native code.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 4. Fix reference caching in native code.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1-4 Amalgamated

Some of the patches in the above series fix things introduced by the previous in the series. If you prefer, this is a folded patch.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599673 [details] [diff] [review]
Patch 1. Add an extra argument to SQLite call. Cleanup some stuff.

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

::: mobile/android/base/sqlite/SQLiteBridge.java
@@ +35,5 @@
> +    private Long[] mQueryResults;
> +
> +    // Values remembered after a query.
> +    private int kResultInsertRowId = 0;
> +    private int kResultRowsChanged = 1;

style nit #1:
k == const, please make these static const
style nit #2:
its certainly not consistent, but ALL_CAPS is more common in our java files for constants than the k prefix.

::: mozglue/android/SQLiteBridge.cpp
@@ +124,5 @@
>      if (initialized) return;
>  
>      objectClass     = jenv->FindClass("java/lang/Object");
>      stringClass     = jenv->FindClass("java/lang/String");
> +    longClass       = jenv->FindClass("java/lang/Long");

not needed if you use jlongarray

@@ +166,5 @@
>                                                        jstring jDb,
>                                                        jstring jQuery,
>                                                        jobjectArray jParams,
>                                                        jobject jColumns,
> +                                                      jobjectArray jQueryRes,

use jLongArray

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599674 [details] [diff] [review]
Patch 2. Update profile migration to use the Cursor API.

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

::: mobile/android/base/sqlite/MatrixBlobCursor.java
@@ +31,5 @@
> + * and MatrixCursor forgot to override it. This was fixed
> + * at some point but old devices are still SOL.
> + * Oh, and everything in MatrixCursor is private instead of
> + * protected, so we need to entirely duplicate it here,
> + * instad of just being able to add the missing method.

I'm going to assume everything here is copied from android sources and just review getblob(). If that's not the case, please re-request review.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599675 [details] [diff] [review]
Patch 3. Push down Cursor construction into native code.

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

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

remove

@@ +126,5 @@
>  
> +    objectClass       = jenv->FindClass("java/lang/Object");
> +    stringClass       = jenv->FindClass("java/lang/String");
> +    byteBufferClass   = jenv->FindClass("java/nio/ByteBuffer");
> +    cursorClass       = jenv->FindClass("org/mozilla/gecko/sqlite/MatrixBlobCursor");

all these need to be global ref'd, for example:

objectClass = (jclass) jenv->NewGlobalRef(jenv->FindClass("java/lang/Object"));

@@ +127,5 @@
> +    objectClass       = jenv->FindClass("java/lang/Object");
> +    stringClass       = jenv->FindClass("java/lang/String");
> +    byteBufferClass   = jenv->FindClass("java/nio/ByteBuffer");
> +    cursorClass       = jenv->FindClass("org/mozilla/gecko/sqlite/MatrixBlobCursor");
> +    jNull             = jenv->NewGlobalRef(NULL);

why do you need this? just use NULL where you use jNull

@@ +261,5 @@
> +        for (int i = 0; i < cols; i++) {
> +            const char* colName = f_sqlite3_column_name(ppStmt, i);
> +            jstring jStr = jenv->NewStringUTF(colName);
> +            jenv->SetObjectArrayElement(jStringArray, i, jStr);
> +            jenv->DeleteLocalRef(jStr);

no need to delete the local ref, it gets deleted when jStr goes out of scope.

@@ +273,5 @@
> +            asprintf(&errorMsg, "Can't allocate MatrixBlobCursor\n");
> +            goto error_close;
> +        }
> +
> +        jenv->DeleteLocalRef(jStringArray);

same here, not needed

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599676 [details] [diff] [review]
Patch 4. Fix reference caching in native code.

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

::: mozglue/android/SQLiteBridge.cpp
@@ +147,5 @@
> +    cursorClass = (jclass)jenv->NewGlobalRef(lCursorClass);
> +    jenv->DeleteLocalRef(lObjectClass);
> +    jenv->DeleteLocalRef(lStringClass);
> +    jenv->DeleteLocalRef(lByteBufferClass);
> +    jenv->DeleteLocalRef(lCursorClass);

these DeleteLocalRef calls are unnecessary

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 599677 [details] [diff] [review]
Patch 1-4 Amalgamated

no need to review code I've already reviewed

Comment 11

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff36792efde9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6935243310
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf80d3cebf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bebeca2270ee

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ff36792efde9
https://hg.mozilla.org/mozilla-central/rev/3e6935243310
https://hg.mozilla.org/mozilla-central/rev/0bf80d3cebf5
https://hg.mozilla.org/mozilla-central/rev/bebeca2270ee