483154 - mozStorageStatementParams should use mozIStorageStatement API instead of calling SQLite directly

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Shawn Wilsher :sdwilsh]

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.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This one was a lot easier than the row one...

Comment 2

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

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.

Comment 3

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

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.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

This should address the review comments.

Comment 5

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

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?

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

It wouldn't be 0 in any sane calling situation, but let's be safe anyway.

Comment 7

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

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

Looks ok to me!

Comment 8

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/dee8dab15435