483157 - mozStorageStatementRow needs to use mozIStorageStatement API

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

There's no reason why it shouldn't already use it, and we end up duplicating code now.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

We don't actually have any tests for blobs returning anything, so I'll need to write one.  This, however, can be reviewed.

Comment 2

[Shawn Wilsher :sdwilsh]

OK, so the params object doesn't really let you bind arrays, which makes making a test a bit harder.  I filed bug 483318 to make it accept arrays, and I'll add the test in there.  Setting in-testsuite? for now to track.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 367283 [details] [diff] [review]
v1.0

>+            PRUint32 length;
>+            const jschar *sval = reinterpret_cast<const jschar *>(
>+                mStatement->AsSharedWString(idx, &length)
>+            );
>+            JSString *str = JS_NewUCStringCopyN(cx, sval, length / 2);

Just for the record I think you should fix the API to be specific, but change that variable name from 'length' to 'bytes' or something to be clear.

>+        else if (type == mozIStorageValueArray::VALUE_TYPE_BLOB) {

So I don't think you should do this this way with the autoptr and heap array, it's not necessary. One other thing to consider, you should be making sure that the ints you get from sqlite can actually fit in an jsval like that (see INT_FITS_IN_JSVAL). If you can't guarantee that somewhere else then you need to call JS_NewNumberValue and root a temp value to hold it. Try this?

  JSObject* obj = JS_NewArrayObject(cx, length, nsnull);
  if (!obj) {
    *_retval = PR_FALSE;
    return NS_OK;
  }
  // Must root array now, JS_NewNumberValue can cause GC.
  *vp = OBJECT_TO_JSVAL(obj);

  jsval val;
  JSAutoTempValueRooter tvr (cx, 1, &val);
  for (PRUint32 i = 0; i < length; i++) {
    if (!JS_NewNumberValue(cx, jsdouble(blob[i]), &val) ||
        !JS_SetElement(cx, obj, i, &val)) {
      *_retval = PR_FALSE;
      return NS_OK;
    }
  }

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #3)
> >+        else if (type == mozIStorageValueArray::VALUE_TYPE_BLOB) {
> 
> So I don't think you should do this this way with the autoptr and heap array,
> it's not necessary. One other thing to consider, you should be making sure that
> the ints you get from sqlite can actually fit in an jsval like that (see
> INT_FITS_IN_JSVAL). If you can't guarantee that somewhere else then you need to
> call JS_NewNumberValue and root a temp value to hold it. Try this?

Each blob byte is just an unsigned 8-bit integer.  An assertion could be added, but it seems very unlikely that the underlying assumptions would ever be violated.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 367283 [details] [diff] [review]
v1.0

I agree with bent that length in regards to strings should probably have units.  So lengthInBytes or nBytes or lenBytes or something would be preferable to length.

I'm fine with how the BLOB case works now, but I'm also fine if you roll bent style.  Since the JS array does get its sized initialized at creation in the bent case, it does seem more efficient in terms of heap allocations.  As per my previous comment, I think INT_TO_JSVAL is fine.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ugh, crap, I wasn't CCd so I didn't see Andrew's comments, and yeah, he's right - INT_TO_JSVAL should be fine. Still think you should avoid the heap allocation as you do in your most recent patch.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

I think this is what you are looking for.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 368584 [details] [diff] [review]
v1.2

>+            double dval;
>+            rv = mStatement->GetDouble(idx, &dval);

You're not checking success here. r=me with that fixed up.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

per comment

Comment 11

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/514e4089924d