Bug 71093 - [meta] CodeGenerator*.pm should support [NamedConstructor] IDL
Summary: [meta] CodeGenerator*.pm should support [NamedConstructor] IDL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on: 71659 71660 71669 71735 71756 71924 71925 73496 73498
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-27 23:24 PDT by Kentaro Hara
Modified: 2011-12-01 17:05 PST (History)
10 users (show)

See Also:


Attachments
WIP patch for V8 (100.89 KB, patch)
2011-11-04 19:33 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP patch for V8 to see if a build passes (104.73 KB, patch)
2011-11-06 10:53 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (46.84 KB, patch)
2011-11-29 04:09 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-10-27 23:24:45 PDT
CodeGeneratorJS.pm and CodeGeneratorV8.pm should support [NamedConstructor] IDL.
The spec: http://www.w3.org/TR/WebIDL/#NamedConstructor

Currently, HTMLAudioElement, HTMLImageElement and HTMLOptionElement have [NamedConstructor] and implement the constructors as custom constructors. We should generate the constructors automatically by the [NamedConstructor] IDL.

First I will make a patch for V8, and then for JSC.
Comment 1 Kentaro Hara 2011-11-04 19:33:02 PDT
Created attachment 113745 [details]
WIP patch for V8
Comment 2 WebKit Review Bot 2011-11-04 19:38:09 PDT
Attachment 113745 [details] did not pass style-queue:

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

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:21:  #ifndef header guard has wrong style, please use: WebKitDOMTestNamedConstructorPrivate_h  [build/header_guard] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.cpp:120:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:145:  Extra space between return and WEBKIT_DOM_TEST_NAMED_CONSTRUCTOR  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestNamedConstructorInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 13 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Kentaro Hara 2011-11-04 19:41:23 PDT
I uploaded a big WIP patch that implements the [NamedConstructor] IDL for V8. I am planning to land it in the following steps:

[1] Refactor CodeGeneratorV8.pm.
[2] Fix the wrong test results of fast/js/custom-constructors.html.
[3] Remove [CustomGetter] from HTMLAudioElementConstructor.
[4] Remove [CustomGetter] from HTMLOptionElementConstructor.
[5] Implement the [NamedConstructor] IDL in IDLParser.pm and CodeGeneratorV8.pm.
[6] Replace the custom constructor of HTMLAudioElement with the [NamedConstructor] IDL.
[7] Replace the custom constructor of HTMLOptionElement with the [NamedConstructor] IDL.

Any thoughts?
Comment 4 Adam Barth 2011-11-04 19:44:15 PDT
Sounds like a reasonable approach.  I can look at the work in progress patch, or I'm also happy to review the changes in the order you propose them (which sounds quite a bit easier to review).
Comment 5 Early Warning System Bot 2011-11-04 19:49:21 PDT
Comment on attachment 113745 [details]
WIP patch for V8

Attachment 113745 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10334204
Comment 6 Gyuyoung Kim 2011-11-04 20:08:26 PDT
Comment on attachment 113745 [details]
WIP patch for V8

Attachment 113745 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10332171
Comment 7 Gustavo Noronha (kov) 2011-11-04 20:13:59 PDT
Comment on attachment 113745 [details]
WIP patch for V8

Attachment 113745 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10334209
Comment 8 Kentaro Hara 2011-11-04 20:48:44 PDT
(In reply to comment #4)
> Sounds like a reasonable approach.  I can look at the work in progress patch, or I'm also happy to review the changes in the order you propose them (which sounds quite a bit easier to review).

Thank you very much, Adam!

After the WIP patch passes all builds and tests, I'll upload sub-patches with more explanations. I would be happy if you take a detailed look at those sub-patches. (Please use the WIP patch just for capturing the overall image of those patches.)
Comment 9 Kentaro Hara 2011-11-06 10:53:10 PST
Created attachment 113793 [details]
WIP patch for V8 to see if a build passes
Comment 10 WebKit Review Bot 2011-11-06 10:56:49 PST
Attachment 113793 [details] did not pass style-queue:

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

Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:21:  #ifndef header guard has wrong style, please use: WebKitDOMTestNamedConstructorPrivate_h  [build/header_guard] [5]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructorPrivate.h:28:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.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/WebKitDOMTestNamedConstructor.cpp:120:  Declaration has space between type name and * in GObjectClass *gobjectClass  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestNamedConstructor.cpp:145:  Extra space between return and WEBKIT_DOM_TEST_NAMED_CONSTRUCTOR  [whitespace/declaration] [3]
Source/WebCore/bindings/scripts/test/ObjC/DOMTestNamedConstructorInternal.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 13 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Kentaro Hara 2011-11-09 12:40:00 PST
OK. All V8-related patches were landed.

I will write a patch for JSC in a few weeks.
Comment 12 Kentaro Hara 2011-11-29 04:09:33 PST
Created attachment 116943 [details]
Patch
Comment 13 Kentaro Hara 2011-11-29 04:11:15 PST
I uploaded a WIP patch that implements the [NamedConstructor] IDL for JSC. I am planning to land it in the following steps:

[1] Implement the [NamedConstructor] IDL in CodeGeneratorV8.pm.
[2] Replace the custom constructor of HTMLAudioElement with the [NamedConstructor] IDL.
[3] Replace the custom constructor of HTMLOptionElement with the [NamedConstructor] IDL.
Comment 14 Kentaro Hara 2011-11-29 04:27:43 PST
(In reply to comment #13)
> I uploaded a WIP patch that implements the [NamedConstructor] IDL for JSC. I am planning to land it in the following steps:
> 
> [1] Implement the [NamedConstructor] IDL in CodeGeneratorV8.pm.

Correction: [1] Implement the [NamedConstructor] IDL in CodeGeneratorJS.pm.
Comment 15 Kentaro Hara 2011-12-01 17:05:58 PST
All related patches were landed. Now both JSC and V8 have the [NamedConstructor] IDL. Closing bug.