Bug 65839

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

Created attachment 103879 [details] WIP patch

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?)

Created attachment 103992 [details] WIP patch

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.

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

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.

Created attachment 104309 [details] WIP patch

Created attachment 104310 [details] WIP patch

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

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

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

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

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

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.

> any thoughts? Sounds great.

I guess this bug can now be marked as fixed?

Yes, you fixed it:) Thanks.