Bug 71379 – [meta] EventConstructors.h should be automatically generated by IDL

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

Currently, EventConstructors.h is written manually and is growing. We should generate it by IDL.

[Kentaro Hara]

Created attachment 114534 [details]
WIP patch for V8

[WebKit Review Bot]

Attachment 114534 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/bindings/scripts/test/CPP/WebDOMTestEventConstructor.cpp:28:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:22:  Found WebCore config.h after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:24:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:129:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:154:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructor.cpp:193:  Extra space between return and WEBKIT_DOM_TEST_EVENT_CONSTRUCTOR  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestEventConstructorInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructorPrivate.h:21:  #ifndef header guard has wrong style, please use: WebKitDOMTestEventConstructorPrivate_h  [build/header_guard] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructorPrivate.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestEventConstructorPrivate.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 15 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

I uploaded a WIP patch to remove custom Event constructors from V8. After landing this patch, I will remove them from JSC.

I am planning to land the WIP patch in the following two steps:

[1] Generate an Event constructor by CodeGeneratorV8.pm. Add [CustomConstructor=Event] IDL to (only) Event and confirm that no regression occurs.
[2] Add [CustomConstructor=Event] IDL to all other Events. Remove V8EventConstructors.cpp.

This patch introduces two new IDLs:

- [CustomConstructor=Event] IDL indicates that CodeGenerator should generate an Event constructor.
- [InitializedAtConstructor] IDL on an attribute indicates that the attribute can be initialized by the constructor.

Any thoughts?

[Adam Barth]

Comment on attachment 114534 [details]
WIP patch for V8

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

> Source/WebCore/ChangeLog:12
> +        - [CustomConstructor=Event] IDL indicates that CodeGenerator should
> +        generate an Event constructor.

CustomConstructor makes me think there's hand-written code to support this feature.  Maybe ConstructorTemplate=Event?  SpecializedConstructor=Event?  I guess CustomConstructor=Event is ok too.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:312
> +    push(@headerContent, "class OptionsObject;\n") if ($dataNode->extendedAttributes->{"V8CustomConstructor"} && $dataNode->extendedAttributes->{"V8CustomConstructor"} eq "Event") || ($dataNode->extendedAttributes->{"CustomConstructor"} && $dataNode->extendedAttributes->{"CustomConstructor"} eq "Event");

Maybe great out this conditional into a helper function?  It seems like a lot of text that is repeated a few times.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1665
> +        if ($attribute->signature->extendedAttributes->{"InitializedAtConstructor"}) {

InitializedAtConstructor => InitializedByConstructor ?

> Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h:33
> +

Looks like you leaked a blank line here.

> Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h:84
> +

This blank line seems like an improvement though.

[Adam Barth]

Can we tell whether an interface is a subclass of Event?  That might be an easier way of triggering this behavior.  We can also wait until we've done the refactoring to load all the IDL into the code generator at once.

[Adam Barth]

This patch is really cool, BTW.

[Kentaro Hara]

Thank you very much, Adam! I'll reflect your comments to coming patches for review.

(In reply to comment #5)
> Can we tell whether an interface is a subclass of Event?  That might be an easier way of triggering this behavior. 

Yes. When $dataNode is an interface, we can know base classes of the interface by $dataNode->parents. This WIP patch is using it to initialize attributes of base classes (i.e. around line 1655 of CodeGeneratorV8.pm). Did you mean something like this? (I am not sure what you mean by "triggering this behavior".)

[Adam Barth]

> Did you mean something like this? (I am not sure what you mean by "triggering this behavior".)

I mean that all subclasses of Event should have this kind of constructor so that CustomConstructor=Event isn't needed.

[Kentaro Hara]

Created attachment 114906 [details]
WIP: An overview patch for JSC

[Kentaro Hara]

I uploaded a WIP patch that removes custom Event constructors from JSC. I am planning to land the WIP patch in the following two steps:

[1] Generate an Event constructor by CodeGeneratorJS.pm. Add [JSConstructorTemplate=Event] IDL to (only) Event and confirm that no regression occurs.
[2] Add [JSConstructorTemplate=Event] IDL to all other Events.
[3] Remove EventConstructors.h and JSEventConstructors.cpp.

[Kentaro Hara]

All the patches are landed. Now no custom Event constructors.