Bug 45237 - Add JSON parameter support to JS/V8 binding generator scripts
Summary: Add JSON parameter support to JS/V8 binding generator scripts
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-04 16:25 PDT by Kinuko Yasuda
Modified: 2011-10-24 14:49 PDT (History)
10 users (show)

See Also:


Attachments
Patch (22.84 KB, patch)
2010-09-04 16:28 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (87.03 KB, patch)
2010-09-07 17:31 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (86.32 KB, patch)
2010-09-10 12:38 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2010-09-04 16:25:13 PDT
Add a new extended attribute (e.g. AllowSON) to support passing parameters in JSON format.

Some spec/idl expect that the user might pass a parameter
in JSON format if the parameter's idl is simple.
For example, FileSystem API (Aug 25, 2010 draft) has a code example
where an object (that has two boolean attributes) is passed in JSON format.

http://dev.w3.org/2009/dap/file-system/file-dir-sys.html#examples-1
4.2 Flags interface, 4.2.2 Examples:
> lockFile = dataDir.getFile("lockfile.txt", {CREATE: true, EXCLUSIVE: true});

While such idl should be generally discouraged, if we need to go with this design it'd be better to have some generic support in the generator rather than having custom implementations for each.
Comment 1 Kinuko Yasuda 2010-09-04 16:28:49 PDT
Created attachment 66593 [details]
Patch

Proposal patch
Comment 2 Adam Barth 2010-09-04 17:06:59 PDT
Comment on attachment 66593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66593&action=prettypatch

> WebCore/bindings/scripts/CodeGeneratorJS.pm:235
> +    my $type = shift;
> +    return ("CREATE" => "boolean", "EXCLUSIVE" => "boolean") if $type eq "Flags";
This logic shouldn't be in the code generator.  You'll need to provide this information as input to the code generator.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:240
> +    # For testing.
> +    return ("intAttr" => "long",
> +            "stringAttr" => "DOMString",
> +            "testObjAttr" => "TestObj") if $type eq "TestJSONObj";
Why do we have special code to support testing?  Doesn't that defeat the purpose of testing?
Comment 3 Kinuko Yasuda 2010-09-07 17:31:35 PDT
Created attachment 66799 [details]
Patch

How about this?  Changed the code to use codeGenerator->ParseInterface instead of having a copy of interface definitions in the generator code.
Comment 4 Kinuko Yasuda 2010-09-10 12:38:50 PDT
Created attachment 67223 [details]
Patch

rebased.
Comment 5 WebKit Review Bot 2010-09-10 12:41:15 PDT
Attachment 67223 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:199:  webkit_dom_test_json_obj_set_property is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:199:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:206:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:227:  webkit_dom_test_json_obj_get_property is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:227:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:234:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:266:  webkit_dom_test_json_obj_constructed is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:273:  webkit_dom_test_json_obj_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:275:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:326:  webkit_dom_test_json_obj_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:341:  Extra space between return and WEBKIT_DOM_TEST_JSON_OBJ  [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:342:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/bindings/scripts/test/V8/V8TestJSONObj.h:26:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestJSONObj.cpp:30:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:39:  parent_instance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:43:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 37 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Kinuko Yasuda 2010-09-10 13:00:15 PDT
(In reply to comment #5)
> Attachment 67223 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> Last 3072 characters of output:
>   Use 0 instead of NULL.  [readability/null] [5]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:199:  webkit_dom_test_json_obj_set_property is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:199:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:206:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:227:  webkit_dom_test_json_obj_get_property is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:227:  prop_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:234:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:266:  webkit_dom_test_json_obj_constructed is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:273:  webkit_dom_test_json_obj_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:275:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:326:  webkit_dom_test_json_obj_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:341:  Extra space between return and WEBKIT_DOM_TEST_JSON_OBJ  [whitespace/declaration] [3]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.cpp:342:  Use 0 instead of NULL.  [readability/null] [5]
> WebCore/bindings/scripts/test/V8/V8TestJSONObj.h:26:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
> WebCore/bindings/scripts/test/CPP/WebDOMTestJSONObj.cpp:30:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:39:  parent_instance is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:43:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> WebCore/bindings/scripts/test/GObject/WebKitDOMTestJSONObj.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 37 in 21 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

All of them are for generated code.  (I don't know how to suppress them...)
Comment 7 Adam Barth 2010-09-10 13:01:26 PDT
> All of them are for generated code.  (I don't know how to suppress them...)

Yeah, we need to teach check-webkit-style about those files.
Comment 8 Kinuko Yasuda 2010-09-10 13:23:56 PDT
(Adding some more people who know binding code well)

Hmm this patch worked for me but calling $codeGenerator->ParseInterface() in the code generation really slows down the process.
Comment 9 Oliver Hunt 2010-10-27 13:29:24 PDT
Is the spec really using json rather than the internal structured clone algorithm?
Comment 10 Kinuko Yasuda 2010-11-03 15:34:49 PDT
Comment on attachment 67223 [details]
Patch

Removing from the review queue
Comment 11 Kinuko Yasuda 2010-11-03 15:37:08 PDT
(In reply to comment #9)
> Is the spec really using json rather than the internal structured clone algorithm?

(In reply to comment #9)
> Is the spec really using json rather than the internal structured clone algorithm?

The spec reads using json parameter rather than the internal structured clone algorithm.
(As far as I believe, @ericu would be able to tell the truth)


I have the feeling that probably adding a CustomConversion attribute to parameter attributes would make more sense.   Then the developer would need to write a small custom code that only handles the parameter rather than writing a custom code for the entire method.  (This was suggested by antonm@chromium.org)

I'm going to poke around the idea and see if such a generalization makes more sense in other cases.
Comment 12 David Levin 2010-11-03 16:38:05 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > Is the spec really using json rather than the internal structured clone algorithm?
>
> The spec reads using json parameter rather than the internal structured clone algorithm.
> (As far as I believe, @ericu would be able to tell the truth)

btw, I don't understand the comment because I don't a text representation for structured clone only an algorithm that explains how to do it. (If there was a text representation, then that may make more sense.)