Bug 28737 – [V8] SQLTransactionCustom binding causing execute-sql-args.html layout test to fail.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

The bindings for SQLTransaction in V8 throw an exception if the optional parameters are passed as null, and also do not accept array-like objects as the sqlArgs parameter. Hence it fails the storage/execute-sql-args.html layout test.

[Ben Murdoch]

I've created a patch to add the required functionality to the V8 bindings, please see attached.

I've also made changes to the layout test:
- Moved the call to executeSql() to the list of calls expected to throw an exception (and updated the JSC binding to do that) as I think that V8 has the behaviour correct here. The spec says the SQL statement is required, so if omitted it seems correct to throw an exception.

- Removed the exception message body from the expected results as the format of exception message from JSC and V8 differ. That way the test is agnostic to JS engine.

[Ben Murdoch]

Created attachment 38673 [details]
Proposed patch

[Eric Seidel (no email)]

Comment on attachment 38673 [details]
Proposed patch

 47     if (args.size() == 0) {
should be args.isEmpty()

I probably would have written 'return "";', but maybe this is slightly faster/better:
+        return StringImpl::empty();

I'm no v8 expert, but these bindings look about as sane as V8 bindings get... which of course is wild raving verbose lunacy:
+                lengthGetter = sqlArgsObject->Get(v8::String::New("length"));
I mean, really?

Don't we have a throwError("foo") function by now which returns undefined so this doesn't have to be 2 lines?
+            V8Proxy::throwError(V8Proxy::TypeError, "sqlArgs should be array or object!");
+            return v8::Undefined();

This seems OK.

[Eric Seidel (no email)]

Comment on attachment 38673 [details]
Proposed patch

Since you're a committer now, I'll r+ and you can make modifications when you land.

[Ben Murdoch]

Thanks Eric. I've implemented your suggestions and will land the patch.

(In reply to comment #3)
> (From update of attachment 38673 [details])
>  47     if (args.size() == 0) {
> should be args.isEmpty()
> 
Fixed.

> I probably would have written 'return "";', but maybe this is slightly
> faster/better:
> +        return StringImpl::empty();
>
There's another site in V8Binding.cpp where we return StrimImpl::empty() instead of "", so I think I'll stick to that precedent.

> I'm no v8 expert, but these bindings look about as sane as V8 bindings get...
> which of course is wild raving verbose lunacy:
> +                lengthGetter = sqlArgsObject->Get(v8::String::New("length"));
> I mean, really?
Yeah, as far as I'm aware this is the procedure for querying properties ... 

> Don't we have a throwError("foo") function by now which returns undefined so
> this doesn't have to be 2 lines?
> +            V8Proxy::throwError(V8Proxy::TypeError, "sqlArgs should be array
> or object!");
> +            return v8::Undefined();
You're right, there is and I updated the code to use it instead.


> This seems OK.
Thanks!

[Ben Murdoch]

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/storage/execute-sql-args-expected.txt
Sending        LayoutTests/storage/execute-sql-args.html
Sending        WebCore/ChangeLog
Sending        WebCore/bindings/js/JSSQLTransactionCustom.cpp
Sending        WebCore/bindings/v8/V8Binding.cpp
Sending        WebCore/bindings/v8/custom/V8SQLTransactionCustom.cpp
Transmitting file data .......
Committed revision 47962.