Bug 28737 - [V8] SQLTransactionCustom binding causing execute-sql-args.html layout test to fail.
Summary: [V8] SQLTransactionCustom binding causing execute-sql-args.html layout test t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-26 06:11 PDT by Ben Murdoch
Modified: 2009-09-02 04:55 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch (10.50 KB, patch)
2009-08-27 10:36 PDT, Ben Murdoch
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 2009-08-26 06:11:38 PDT
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.
Comment 1 Ben Murdoch 2009-08-27 10:34:57 PDT
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.
Comment 2 Ben Murdoch 2009-08-27 10:36:16 PDT
Created attachment 38673 [details]
Proposed patch
Comment 3 Eric Seidel (no email) 2009-09-02 03:00:01 PDT
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.
Comment 4 Eric Seidel (no email) 2009-09-02 03:00:37 PDT
Comment on attachment 38673 [details]
Proposed patch

Since you're a committer now, I'll r+ and you can make modifications when you land.
Comment 5 Ben Murdoch 2009-09-02 04:02:03 PDT
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!
Comment 6 Ben Murdoch 2009-09-02 04:55:54 PDT
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.