mozStorageStatementRow needs to use mozIStorageStatement API

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Storage
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

3.77 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
There's no reason why it shouldn't already use it, and we end up duplicating code now.
(Assignee)

Comment 1

9 years ago
Created attachment 367283 [details] [diff] [review]
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.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #367283 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Attachment #367283 - Flags: review?(jorendorff)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review asuth][needs review jorendorff]
(Assignee)

Comment 2

9 years ago
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.
Flags: in-testsuite?
(Assignee)

Updated

9 years ago
Attachment #367283 - Flags: review?(jorendorff) → review?(bent.mozilla)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review asuth][needs review jorendorff] → [needs review asuth][needs review bent]
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;
    }
  }
Attachment #367283 - Flags: review?(bent.mozilla) → review-
(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 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.
Attachment #367283 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth][needs review bent] → [needs revised patch for bent]
(Assignee)

Comment 6

9 years ago
Created attachment 368569 [details] [diff] [review]
v1.1
Attachment #367283 - Attachment is obsolete: true
Attachment #368569 - Flags: review?(bent.mozilla)
(Assignee)

Updated

9 years ago
Whiteboard: [needs revised patch for bent] → [needs review bent]
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.
(Assignee)

Comment 8

9 years ago
Created attachment 368584 [details] [diff] [review]
v1.2

I think this is what you are looking for.
Attachment #368569 - Attachment is obsolete: true
Attachment #368584 - Flags: review?(bent.mozilla)
Attachment #368569 - Flags: review?(bent.mozilla)
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.
Attachment #368584 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 10

9 years ago
Created attachment 368605 [details] [diff] [review]
v1.3

per comment
Attachment #368584 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Summary: mozStorageStatementRow needs to use mozIStorageStatementAPI → mozStorageStatementRow needs to use mozIStorageStatement API
Whiteboard: [needs review bent] → [can land]
(Assignee)

Comment 11

9 years ago
http://hg.mozilla.org/mozilla-central/rev/514e4089924d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.