Bug 65839

Summary: [meta] CodeGenerator*.pm should support [Constructor] IDL
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, arv, dglazkov, dominicc, donggwan.kim, gustavo, haraken, tommyw, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 66454, 66455, 66456, 66536, 67381, 67412, 67414, 67447, 67458, 67459, 67879, 69074, 69801, 70101, 70206, 70208, 70212, 70214, 70215, 73064    
Bug Blocks:    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
WIP patch
none
Hey! WIP patch for JSC!
none
WIP patch: just see if build succeeds none

Description Kentaro Hara 2011-08-07 21:23:51 PDT
CodeGeneratorJS.pm and CodeGeneratorV8.pm should support Web IDL [Constructor] attribute:
http://www.w3.org/TR/WebIDL/#Constructor
Comment 1 Kentaro Hara 2011-08-07 21:44:05 PDT
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.
Comment 2 Dominic Cooney 2011-08-08 07:08:53 PDT
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.
Comment 3 Kentaro Hara 2011-08-14 04:26:20 PDT
Created attachment 103879 [details]
WIP patch
Comment 4 Dominic Cooney 2011-08-14 18:30:20 PDT
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?)
Comment 5 Kentaro Hara 2011-08-15 18:58:18 PDT
Created attachment 103992 [details]
WIP patch
Comment 6 Kentaro Hara 2011-08-15 18:58:28 PDT
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.
Comment 7 Dominic Cooney 2011-08-15 19:06:28 PDT
Sounds like a good plan. As you work on these, can you file each one as a bug that blocks this bug?
Comment 8 Kentaro Hara 2011-08-15 19:19:17 PDT
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.
Comment 9 Kentaro Hara 2011-08-18 00:36:25 PDT
Created attachment 104309 [details]
WIP patch
Comment 10 Kentaro Hara 2011-08-18 00:38:27 PDT
Created attachment 104310 [details]
WIP patch
Comment 11 Adam Barth 2011-08-18 00:45:14 PDT
OMG.  This is so exciting!  Thank you for working on this bug.
Comment 12 Kentaro Hara 2011-08-18 00:53:03 PDT
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].
Comment 13 Kentaro Hara 2011-10-03 23:19:47 PDT
Created attachment 109583 [details]
Hey! WIP patch for JSC!
Comment 14 Kentaro Hara 2011-10-03 23:29:46 PDT
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.
Comment 15 WebKit Review Bot 2011-10-04 01:03:01 PDT
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
Comment 16 Gustavo Noronha (kov) 2011-10-04 01:35:47 PDT
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
Comment 17 Gyuyoung Kim 2011-10-04 03:42:26 PDT
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
Comment 18 Kentaro Hara 2011-10-07 06:02:25 PDT
Created attachment 110132 [details]
WIP patch: just see if build succeeds
Comment 19 Kentaro Hara 2011-10-07 20:57:30 PDT
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.
Comment 20 Adam Barth 2011-10-07 22:00:00 PDT
> any thoughts?

Sounds great.
Comment 21 Tommy Widenflycht 2013-01-03 01:05:14 PST
I guess this bug can now be marked as fixed?
Comment 22 Kentaro Hara 2013-01-03 01:09:45 PST
Yes, you fixed it:) Thanks.