Bug 65839 – [meta] CodeGenerator*.pm should support [Constructor] IDL

• Component: DOM
• Status: RESOLVED FIXED

[Kentaro Hara]

CodeGeneratorJS.pm and CodeGeneratorV8.pm should support Web IDL [Constructor] attribute:
http://www.w3.org/TR/WebIDL/#Constructor

[Kentaro Hara]

Created attachment 103199 [details]
WIP patch

Dominicc: For now, this patch generates a code for a constructor of XMLHttpRequest, i.e. a specific code for one DOM object. I guess that the code that [Constructor] should generate by default would be the code of V8Proxy::constructDOMObject.

[Dominic Cooney]

Comment on attachment 103199 [details]
WIP patch

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

I love the direction this is going in. Some comments and questions inline.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1972
> +    if ($dataNode->extendedAttributes->{"CanBeConstructed"} && !$dataNode->extendedAttributes->{"CustomConstructor"} && !$dataNode->extendedAttributes->{"V8CustomConstructor"} && !$dataNode->extendedAttributes->{"Constructor"}) {

We may want to consider Constructor implies CanBeConstructed. I am not sure about what you’ve got here, since the one case—XMLHttpRequest—has CanBeConstructed, Constructor. What’s your thinking here?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1989
> +    if ($dataNode->extendedAttributes->{"Constructor"}) {

This is a solid start. Should we try doing two or three more constructors to generalize it a bit? (Arguments, for example.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1998
> +    ScriptExecutionContext* context = getScriptExecutionContext();

I expect many objects have a "context", eg XMLHttpRequest has ScriptExecutionContext; Node has Document; etc. Maybe we could support different kinds of contexts (hopefully there are only few) with metadata like ConstructorContext=ScriptExecutionContext|Document|etc.

> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:-1
> -/*

Yay, killing code. I love it.

[Kentaro Hara]

Created attachment 103879 [details]
WIP patch

[Dominic Cooney]

Comment on attachment 103879 [details]
WIP patch

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

This looks really good. Since this patch is pretty big we should work to get this in without expanding the scope any further yet.

You can edit the files in Source/WebCore/bindings/scripts/test/*.idl and run Tools/Scripts/run-bindings-tests to generate test output. There is a flag to reset the expected output too. This can be useful because it is possible to review the effect of changes to CodeGeneratorV8.pm; what effect metadata has.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1487
> +        push(@implContent, <<END);

Maybe just use a string literal?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1552
> +sub IsContainValue

Just call it ContainsValue

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1558
> +    my @valueList = split(/\|/, $orSeparatedValueString);

Does the | mean "either" or "and". Maybe | is not the best choice of separator?

> Source/WebCore/bindings/scripts/IDLParser.pm:186
> +        }

I wish the IDL parser was recursive descent too; I think this is still quite readable though.

> Source/WebCore/css/WebKitCSSMatrix.idl:34
> +        ConstructorWith=CreateException|DOMObject

You don’t really need to do this | thing. You could have separate attributes like:

Constructor(…)
ConstructWith=ScriptExecutionContext
ConstructorSetsDOMWrapper
ConstructorRaisesException

This is verbose, but it is clearer. Also, I think "DOMObject" might only apply to V8, not JSC (not sure) so we could make that one V8ConstructorSetsDOMWrapper.

Do you think that there are any constructors that have different attributes in these dimensions (eg one overload that throws exceptions and one that doesn’t?)

[Kentaro Hara]

Created attachment 103992 [details]
WIP patch

[Kentaro Hara]

Dominic: I confirmed that the patch passes all tests. Basically, this patch intends to replace 6 custom constructors (EventSource, FileReader, SharedWorker, WebKitCSSMatrix, Worker, XSLTProcessor) with the Web IDL [Constructor] extended attribute without changing their original behaviors. 

However, this patch changes an exception thrown by EventSource, SharedWorker and Worker constructors, when an argument for them is not valid. Specifically, without this patch, these constructors throw "throwError("Not enough arguments", V8Proxy::SyntaxError)" from constructorCallback(). On the other hand, with this patch, these constructors throw "throwError(SYNTAX_ERR)" from XXXXXX::create(). In other words, this patch moves the code for throwing an exception from constructorCallback() to inside XXXXXX::create(). (Currently, some constructors throw the syntax error from constructorCallback(), and others throw it from XXXXXX::create(). I want to make them consistent, so that they throw the syntax error from XXXXXX::create().)


Since this patch is too big to commit without regressions, I would like to commit it in the following steps. 

[1] Make the change of exceptions in EventSource constructor.
[2] Make the change of exceptions in SharedWorker constructor.
[3] Make the change of exceptions in Worker constructor.
[4] Implement the Web IDL Constructor extended attribute in IDLParser.pm and CodeGeneratorV8.pm. (How can I write the tests for this?)
[5] Replace EventSource constructor with the Web IDL Constructor extended attribute.
[6] Replace FileReader constructor with the Web IDL Constructor extended attribute.
[7] Replace SharedWorker constructor with the Web IDL Constructor extended attribute.
[8] Replace WebKitCSSMatrix constructor with the Web IDL Constructor extended attribute.
[9] Replace Worker constructor with the Web IDL Constructor extended attribute.
[10] Replace XSLTProcessor constructor with the Web IDL Constructor extended attribute.

[Dominic Cooney]

Sounds like a good plan. As you work on these, can you file each one as a bug that blocks this bug?

[Kentaro Hara]

Comment on attachment 103879 [details]
WIP patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1487
>> +        push(@implContent, <<END);
> 
> Maybe just use a string literal?

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1552
>> +sub IsContainValue
> 
> Just call it ContainsValue

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1558
>> +    my @valueList = split(/\|/, $orSeparatedValueString);
> 
> Does the | mean "either" or "and". Maybe | is not the best choice of separator?

Removed the separator, since the separator is not necessary any longer.

>> Source/WebCore/css/WebKitCSSMatrix.idl:34
>> +        ConstructorWith=CreateException|DOMObject
> 
> You don’t really need to do this | thing. You could have separate attributes like:
> 
> Constructor(…)
> ConstructWith=ScriptExecutionContext
> ConstructorSetsDOMWrapper
> ConstructorRaisesException
> 
> This is verbose, but it is clearer. Also, I think "DOMObject" might only apply to V8, not JSC (not sure) so we could make that one V8ConstructorSetsDOMWrapper.
> 
> Do you think that there are any constructors that have different attributes in these dimensions (eg one overload that throws exceptions and one that doesn’t?)

> Constructor(…)
> ConstructWith=ScriptExecutionContext
> ConstructorSetsDOMWrapper
> ConstructorRaisesException

Done. 

> Do you think that there are any constructors that have different attributes in these dimensions

I am not sure yet.

[Kentaro Hara]

Created attachment 104309 [details]
WIP patch

[Kentaro Hara]

Created attachment 104310 [details]
WIP patch

[Adam Barth]

OMG.  This is so exciting!  Thank you for working on this bug.

[Kentaro Hara]

I updated the WIP patch, since I found that the previous patch was violating the spec (Please see the comments by abarth and ap in bug 66274, bug 66275 and bug 66276). 

Now, I am planning to land this WIP patch in the following steps.

[1] Throw TypeError from the EventSource constructor (of V8 and JSC), when the number of arguments is not enough. 
[2] Throw TypeError from the SharedWorker constructor (of V8 and JSC), when the number of arguments is not enough. 
[3] Throw TypeError from the Worker constructor (of V8 and JSC), when the number of arguments is not enough. 
[4] Implement the Web IDL Constructor extended attribute in IDLParser.pm and CodeGeneratorV8.pm.
[5] Replace EventSource constructor of V8 with the Web IDL Constructor extended attribute.
[6] Replace FileReader constructor of V8 with the Web IDL Constructor extended attribute.
[7] Replace SharedWorker constructor of V8 with the Web IDL Constructor extended attribute.
[8] Replace WebKitCSSMatrix constructor of V8 with the Web IDL Constructor extended attribute.
[9] Replace Worker constructor of V8 with the Web IDL Constructor extended attribute.
[10] Replace XSLTProcessor constructor of V8 with the Web IDL Constructor extended attribute.

Dependency: [5][6][7][8][9][10] depends on [1][2][3][4].

[Kentaro Hara]

Created attachment 109583 [details]
Hey! WIP patch for JSC!

[Kentaro Hara]

Next round.

I uploaded a WIP patch for implementing [Constructor] IDL for JSC. I would be happy if you could take a rough look at it. Comments are appreciated.

I am planning to land this big patch in the following steps:

[1] Deprecate [CallWith=ScriptExecutionContext] for constructors, since it has the same meaning as [ConstructorWith=ScriptExecutionContext].
[2] Make a change in CodeGeneratorJS.pm to support [Constructor] IDL, [ConstructorWith=ScriptExecutionContext] and [ConstructorRaisesException].
[3] Replace a custom constructor of XSLTProcessor with [Constructor] IDL.
[4] Replace a custom constructor of XMLHttpRequest with [Constructor] IDL.
[5] Replace a custom constructor of FileReader with [Constructor] IDL.
[6] Replace a custom constructor of MessageChannel with [Constructor] IDL.
[7] Replace a custom constructor of EventSource with [Constructor] IDL.
[8] Introduce MAYBE_MISSING_PARAMETER() macro. Replace a custom constructor of WebKitCSSMatrix with [Constructor] IDL.

[WebKit Review Bot]

Comment on attachment 109583 [details]
Hey! WIP patch for JSC!

Attachment 109583 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9938331

New failing tests:
http/tests/inspector/network/network-xhr-same-url-as-main-resource.html
http/tests/inspector/network/network-disabling-check-no-memory-leak.html
http/tests/inspector/network/network-content-replacement-xhr.html
http/tests/inspector/search/search-in-concatenated-script.html
fast/events/event-fire-order.html
http/tests/inspector/resource-tree/resource-tree-no-xhrs.html
http/tests/inspector/network/network-xhr-sync.html
http/tests/inspector/network-preflight-options.html
fast/events/nested-event-remove-node-crash.html
http/tests/local/formdata/upload-events.html
fast/dom/Window/window-early-properties-xhr.html
http/tests/inspector/network/network-xhr-async.html
http/tests/inspector/network/network-initiator.html
http/tests/appcache/fallback.html
http/tests/inspector/network/network-sidebar-width.html
http/tests/local/formdata/form-data-with-unknown-file-extension.html
fast/dom/xmlhttprequest-constructor-in-detached-document.html
http/tests/inspector/network/network-disable-cache-xhrs.html

[Gustavo Noronha (kov)]

Comment on attachment 109583 [details]
Hey! WIP patch for JSC!

Attachment 109583 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9940296

[Gyuyoung Kim]

Comment on attachment 109583 [details]
Hey! WIP patch for JSC!

Attachment 109583 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9938377

[Kentaro Hara]

Created attachment 110132 [details]
WIP patch: just see if build succeeds

[Kentaro Hara]

The uploaded WIP patch passed all tests. I am planning to land this big patch in the following steps, but any thoughts?

[1] Deprecate [CallWith=ScriptExecutionContext] for constructors, since it has the same meaning as [ConstructorWith=ScriptExecutionContext].
[2] Make a change in CodeGeneratorJS.pm to support [Constructor] IDL, [ConstructorWith=ScriptExecutionContext] and [ConstructorRaisesException].
[3] Replace a custom constructor of XSLTProcessor with [Constructor] IDL.
[4] Replace a custom constructor of XMLHttpRequest with [Constructor] IDL.
[5] Replace a custom constructor of FileReader with [Constructor] IDL.
[6] Replace a custom constructor of MessageChannel with [Constructor] IDL.
[7] Replace a custom constructor of EventSource with [Constructor] IDL.
[8] Introduce MAYBE_MISSING_PARAMETER() macro. Replace a custom constructor of WebKitCSSMatrix with [Constructor] IDL.

[Adam Barth]

> any thoughts?

Sounds great.

[Tommy Widenflycht]

I guess this bug can now be marked as fixed?

[Kentaro Hara]

Yes, you fixed it:) Thanks.