mozStorageStatementParams should use mozIStorageStatement API instead of calling SQLite directly

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:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

6.22 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
There's really no reason to call SQLite directly here - we can let the storage statement class handle this (it's already setup to do so), and then we don't have to worry about duplicating logic.
(Assignee)

Comment 1

9 years ago
Created attachment 367317 [details] [diff] [review]
v1.0

This one was a lot easier than the row one...
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #367317 - Flags: review?(jorendorff)
Attachment #367317 - Flags: review?(bugmail)
(Assignee)

Updated

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

Updated

9 years ago
Attachment #367317 - 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 367317 [details] [diff] [review]
v1.0

>         name.Append(NS_ConvertUTF16toUTF8(nsDependentString((PRUnichar *)::JS_GetStringChars(str),
>                                                             ::JS_GetStringLength(str))));

Not your new code, but there's no need for that nsDependentString, just hand NS_ConvertUTF16toUTF8 the pointer and length. In the next hunk too.

>+        if (NS_FAILED(rv)) {
>             *_retval = PR_FALSE;
>             return NS_ERROR_INVALID_ARG;
>         }
> ...
>+    else {
>         *_retval = PR_FALSE;
>     }

No need to set *_retval if you're going to return a failure code. XPConnect won't even look at it. In the next hunk too.

>         PRBool success = ::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),

So you've already gotten the string chars and length above, why not save and reuse?

>+    if (idx >= mParamCount) {
>         *_retval = PR_FALSE;
>         return NS_OK;
>     }

So that is actually a silent, uncatchable exception right there. Is that intended? If not you should return a failure nsresult.
Attachment #367317 - Flags: review?(bugmail) → review+
Comment on attachment 367317 [details] [diff] [review]
v1.0

>diff --git a/storage/src/mozStorageStatementParams.cpp b/storage/src/mozStorageStatementParams.cpp
>     // is it out of range?
>-    if (idx < 0 || idx >= (int)mParamCount) {
>+    if (idx >= mParamCount) {
>         *_retval = PR_FALSE;
>         return NS_OK;
>     }

I agree with bent; the silent failure of assignment is probably not desired here.  Suggest NS_ERROR_ILLEGAL_VALUE.
Whiteboard: [needs review asuth][needs review bent] → [needs review bent]
(Assignee)

Comment 4

9 years ago
Created attachment 368577 [details] [diff] [review]
v1.1

This should address the review comments.
Attachment #367317 - Attachment is obsolete: true
Attachment #368577 - Flags: review?(bent.mozilla)
Attachment #367317 - Flags: review?(bent.mozilla)
Comment on attachment 368577 [details] [diff] [review]
v1.1

> +    NS_ENSURE_ARG_MAX(idx, mParamCount - 1);

Er, this will be wrong if mParamCount is 0 because mParamCount is unsigned. Can it be 0 here?
(Assignee)

Comment 6

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

It wouldn't be 0 in any sane calling situation, but let's be safe anyway.
Attachment #368577 - Attachment is obsolete: true
Attachment #368583 - Flags: review?(bent.mozilla)
Attachment #368577 - Flags: review?(bent.mozilla)
Attachment #368583 - Flags: review?(bent.mozilla) → review+
Comment on attachment 368583 [details] [diff] [review]
v1.2

Looks ok to me!
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent] → [can land]
(Assignee)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/dee8dab15435
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.