Bug 45237 – Add JSON parameter support to JS/V8 binding generator scripts

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

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 66593 [details]
Patch

Proposal patch

[Adam Barth]

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?

[Kinuko Yasuda]

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.

[Kinuko Yasuda]

Created attachment 67223 [details]
Patch

rebased.

[WebKit Review Bot]

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.

[Kinuko Yasuda]

(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...)

[Adam Barth]

> 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.

[Kinuko Yasuda]

(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.

[Oliver Hunt]

Is the spec really using json rather than the internal structured clone algorithm?

[Kinuko Yasuda]

Comment on attachment 67223 [details]
Patch

Removing from the review queue

[Kinuko Yasuda]

(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 [e-mail])

I'm going to poke around the idea and see if such a generalization makes more sense in other cases.

[David Levin]

(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.)

[Anne van Kesteren]

V8 integration is no longer a thing.