Bug 35048 – an 'undefined' return value from a statement error callback should be treated as 'true'

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

According to the Web SQL Database spec and http://www.mail-archive.com/[e-mail]/msg06679.html, if a statement error callback returns 'undefined' (doesn't return anything), we should treat it as 'not false', and roll back the transaction. Currently we treat it as 'false'.

[Dumitru Daniliuc]

Created attachment 48944 [details]
patch

[WebKit Review Bot]

Attachment 48944 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:104:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:105:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:108:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebCore/bindings/v8/custom/V8CustomVoidCallback.cpp:112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dumitru Daniliuc]

Created attachment 48946 [details]
patch

[Darin Adler]

Comment on attachment 48946 [details]
patch

What about null? Should that still be false?

[Dumitru Daniliuc]

Created attachment 48953 [details]
patch

(In reply to comment #4)
> (From update of attachment 48946 [details])
> What about null? Should that still be false?

fixed. treating all non-boolean values as 'true' (since the spec seems to divide all return values into 'false' and 'everything else'). also added more test cases to storage/statement-error-callback.html to test all kind of non-boolean return types (including 'null').

[Darin Adler]

Comment on attachment 48953 [details]
patch

> -    return result.toBoolean(exec);
> +    return !result.isBoolean() || result.toBoolean(exec);

While this will work, it's unnecessarily inefficient. The toBoolean function's job is to convert things from other types to boolean. The efficient way is to write:

    return result.isFalse();

The patch otherwise seems fine. r=me, but please make the fix I suggest above.

[Dumitru Daniliuc]

(In reply to comment #6)
> (From update of attachment 48953 [details])
> > -    return result.toBoolean(exec);
> > +    return !result.isBoolean() || result.toBoolean(exec);
> 
> While this will work, it's unnecessarily inefficient. The toBoolean function's
> job is to convert things from other types to boolean. The efficient way is to
> write:
> 
>     return result.isFalse();
> 
> The patch otherwise seems fine. r=me, but please make the fix I suggest above.

nice optimization! will change this in the JSC and V8 code before submitting.

[Dumitru Daniliuc]

Landed as r54981.