Closed Bug 726821 Opened 8 years ago Closed 8 years ago

SQLiteBridge should return a cursor

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 13
Tracking Status
fennec 13+ ---

People

(Reporter: wesj, Assigned: gcp)

References

Details

Attachments

(5 files)

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).
Assignee: nobody → gpascutto
Blocks: 727264
Attachment #599676 - Flags: review?(blassey.bugs)
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.
Attachment #599677 - Flags: review?(blassey.bugs)
tracking-fennec: --- → 13+
Priority: -- → P2
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
Attachment #599673 - Flags: review?(blassey.bugs) → review+
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.
Attachment #599674 - Flags: review?(blassey.bugs) → review+
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
Attachment #599675 - Flags: review?(blassey.bugs) → review+
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
Attachment #599676 - Flags: review?(blassey.bugs) → review+
Comment on attachment 599677 [details] [diff] [review]
Patch 1-4 Amalgamated

no need to review code I've already reviewed
Attachment #599677 - Flags: review?(blassey.bugs)
Depends on: 1092844
You need to log in before you can comment on or make changes to this bug.