Bug 161576 – Add initial support for IDL union conversion

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Add initial support for IDL union conversion

[Sam Weinig]

Created attachment 287913 [details]
Patch

[Sam Weinig]

Let's see if this compiles anywhere else.

[WebKit Commit Bot]

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


ERROR: Source/WTF/wtf/Meta.h:43:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:53:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:53:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:61:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WTF/wtf/Meta.h:95:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:103:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:122:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:125:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:130:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:138:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:141:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:147:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:159:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:167:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:175:  try_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Meta.h:179:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:182:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:190:  try_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Meta.h:194:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:194:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:197:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:197:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:205:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:210:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:215:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:230:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:235:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:240:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:246:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:255:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:260:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:265:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:271:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:277:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:283:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:288:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:294:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:300:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:306:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:319:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:322:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:329:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:338:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:344:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:349:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:361:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:364:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:370:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:379:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:386:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:404:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:412:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:417:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:423:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:424:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:427:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:447:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:450:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:455:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:461:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:468:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:473:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:475:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:476:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:478:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:479:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:480:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:492:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:495:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:500:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:506:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/Meta.h:507:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:516:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:519:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:529:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:531:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/Meta.h:535:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:535:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/Meta.h:540:  Do not use unnamed namespaces in header files.  See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces for more information.  [build/namespaces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMConvert.h:327:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMConvert.h:364:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/bindings/js/JSDOMConvert.h:364:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/StdLibExtras.h:351:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/StdLibExtras.h:354:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/StdLibExtras.h:364:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 91 in 12 files


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

[Build Bot]

Comment on attachment 287913 [details]
Patch

Attachment 287913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2007335

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 287916 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287913 [details]
Patch

Attachment 287913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2007336

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 287917 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Sam Weinig]

Comment on attachment 287913 [details]
Patch

As I feared, this won't work out of the box on Windows. I'm going to try porting to https://github.com/edouarda/brigand, which purports to have MSVC support.

[Build Bot]

Comment on attachment 287913 [details]
Patch

Attachment 287913 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2007344

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 287919 [details]
Archive of layout-test-results from ews124 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Build Bot]

Comment on attachment 287913 [details]
Patch

Attachment 287913 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2008689

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 287923 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Sam Weinig]

Created attachment 288077 [details]
Patch

[Sam Weinig]

Let's try with the Brigand library instead, which claims MSVC support!

[Build Bot]

Comment on attachment 288077 [details]
Patch

Attachment 288077 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2021142

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 288084 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288077 [details]
Patch

Attachment 288077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2021157

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 288085 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288077 [details]
Patch

Attachment 288077 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2021158

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 288086 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288077 [details]
Patch

Attachment 288077 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2021186

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/append-on-Document.html
fast/dom/ChildNode-after.html
imported/w3c/web-platform-tests/dom/nodes/ParentNode-append.html
imported/w3c/web-platform-tests/dom/nodes/ChildNode-replaceWith.html
imported/w3c/web-platform-tests/dom/nodes/ChildNode-before.html
imported/w3c/web-platform-tests/dom/nodes/ParentNode-prepend.html
userscripts/mixed-case-stylesheet.html
js/dom/native-bindings-descriptors.html
fast/dom/ParentNode-append.html
fast/dom/Window/forbid-showModalDialog.html
imported/w3c/web-platform-tests/dom/nodes/prepend-on-Document.html
imported/w3c/web-platform-tests/dom/nodes/ChildNode-after.html
fast/dom/ChildNode-replaceWith.html
fast/dom/ParentNode-prepend.html
fast/dom/ChildNode-before.html

[Build Bot]

Created attachment 288090 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Sam Weinig]

Created attachment 288165 [details]
Patch

[Build Bot]

Comment on attachment 288165 [details]
Patch

Attachment 288165 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2027823

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 288177 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288165 [details]
Patch

Attachment 288165 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2027830

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 288178 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288165 [details]
Patch

Attachment 288165 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2027862

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 288179 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288165 [details]
Patch

Attachment 288165 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2027859

New failing tests:
js/dom/native-bindings-descriptors.html

[Build Bot]

Created attachment 288181 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Sam Weinig]

Created attachment 288188 [details]
Patch

[Sam Weinig]

Comment on attachment 288188 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWrapper.h:35
> -static const uint8_t JSNodeType = JSC::LastJSCObjectType + 1;
> +static const uint8_t JSDOMWrapperType = JSC::LastJSCObjectType + 1;
> +static const uint8_t JSNodeType = JSC::LastJSCObjectType + 2;
>  static const uint8_t JSDocumentWrapperType = JSC::LastJSCObjectType + 2;
>  static const uint8_t JSElementType = JSC::LastJSCObjectType + 3;

Oy! This is wrong. JSNodeType and JSDocumentWrapperType can't both be 2!

[Sam Weinig]

Created attachment 288206 [details]
Patch

[Chris Dumez]

Comment on attachment 288206 [details]
Patch

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

I do not mind this approach although I personally find the resulting code a bit hard to read. I wonder why we cannot write more of this in Perl.

> Source/WebCore/bindings/generic/IDLTypes.h:40
> +// IDLNull is a special type that serves as a base class for currently unsupported types.

Do you mean IDLUnsupportedType ?

> Source/WebCore/bindings/generic/IDLTypes.h:46
> +struct IDLAny : IDLUnsupportedType { };

In practice, I believe we use JSC::JSValue for IDL any type, no?

> Source/WebCore/bindings/generic/IDLTypes.h:76
> +template<typename T> struct IDLInterfaceType : IDLType<Ref<T>> { 

I am not 100% clear why Interfaces use Ref<T> here and not RefPtr<T> or T*.

> Source/WebCore/bindings/generic/IDLTypes.h:80
> +template<typename T> struct IDLDictionary : IDLType<Optional<T>> {

Why is dictionary using Optional?

> Source/WebCore/bindings/generic/IDLTypes.h:84
> +template<typename T> struct IDLEnumeration : IDLType<Optional<T>> {

Why is enumeration using Optional?

> Source/WebCore/bindings/generic/IDLTypes.h:92
> +template<typename T> struct IDLNullable : IDLType<Optional<typename T::ImplementationType>> {

It seems odd to use Optional<> for nullable. If T is a wrapper type for e.g., we normally pass a raw pointer. For string types, we could pass a String (with null String() if null).

> Source/WebCore/bindings/js/JSDOMConvert.h:136
> +template<> struct Converter<IDLDOMString> : DefaultConverter<String> {

It is sad that we duplicate converters (i.e. Converter<IDLDOMString> & Converter<String>).

> Source/WebCore/bindings/js/JSDOMConvert.h:345
> +struct Converter<IDLUnion<T...>> : DefaultConverter<typename IDLUnion<T...>::ImplementationType>

It seems the algorithm implemented here in C++ is very similar to the one I implemented in Perl for the overload resolution algorithm.

What are the benefits of writing this in C++ rather than in Perl? I may be missing something but it looks to me that writing this in C++ leads to more complicated code.

[Sam Weinig]

(In reply to comment #35)
> Comment on attachment 288206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288206&action=review
> 
> I do not mind this approach although I personally find the resulting code a
> bit hard to read. I wonder why we cannot write more of this in Perl.
> 
> > Source/WebCore/bindings/generic/IDLTypes.h:40
> > +// IDLNull is a special type that serves as a base class for currently unsupported types.
> 
> Do you mean IDLUnsupportedType ?

Yes.

> 
> > Source/WebCore/bindings/generic/IDLTypes.h:46
> > +struct IDLAny : IDLUnsupportedType { };
> 
> In practice, I believe we use JSC::JSValue for IDL any type, no?

Yes.

> 
> > Source/WebCore/bindings/generic/IDLTypes.h:76
> > +template<typename T> struct IDLInterfaceType : IDLType<Ref<T>> { 
> 
> I am not 100% clear why Interfaces use Ref<T> here and not RefPtr<T> or T*.

For my initial purposes, (Node or DOMString) Ref<T> worked, but I could see it not being ideal in some places. I do think it should be the cononical non-null representation of an interface type.

> 
> > Source/WebCore/bindings/generic/IDLTypes.h:80
> > +template<typename T> struct IDLDictionary : IDLType<Optional<T>> {
> 
> Why is dictionary using Optional?

Good point.

> 
> > Source/WebCore/bindings/generic/IDLTypes.h:84
> > +template<typename T> struct IDLEnumeration : IDLType<Optional<T>> {
> 
> Why is enumeration using Optional?

Good point.

> 
> > Source/WebCore/bindings/generic/IDLTypes.h:92
> > +template<typename T> struct IDLNullable : IDLType<Optional<typename T::ImplementationType>> {
> 
> It seems odd to use Optional<> for nullable. If T is a wrapper type for
> e.g., we normally pass a raw pointer. For string types, we could pass a
> String (with null String() if null).

We could certainly rework this as 

template<typename T> struct IDLNullable : IDLType<typename T::OptionalType> { };

and have each IDLType declare it's optional representation.  I might like that.  There was something nice about having it all be uniform, but, probably not practical.

 
> > Source/WebCore/bindings/js/JSDOMConvert.h:136
> > +template<> struct Converter<IDLDOMString> : DefaultConverter<String> {
> 
> It is sad that we duplicate converters (i.e. Converter<IDLDOMString> &
> Converter<String>).

The idea was to eventually only have Converter<IDLDOMString>, and remove Converter<String>. This would make it clear, when doing conversions, which type of IDL conversion you wanted (DOMString, USVString, ByteString) in the type.

> 
> > Source/WebCore/bindings/js/JSDOMConvert.h:345
> > +struct Converter<IDLUnion<T...>> : DefaultConverter<typename IDLUnion<T...>::ImplementationType>
> 
> It seems the algorithm implemented here in C++ is very similar to the one I
> implemented in Perl for the overload resolution algorithm.
> 
> What are the benefits of writing this in C++ rather than in Perl? I may be
> missing something but it looks to me that writing this in C++ leads to more
> complicated code.

I think the biggest benefit is having all the conversion code in one place. We could have written lots of the converters in perl, but have opted for C++ to keep them together. Plus, it was really fun.

[Sam Weinig]

Created attachment 290385 [details]
Patch

[Chris Dumez]

Comment on attachment 290385 [details]
Patch

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

r=me if the bots are happy with a couple of comments.

> Source/WebCore/bindings/js/JSNodeOrString.cpp:44
> +    using InterfaceTypeList = typename ConverterType::InterfaceTypeList;

All this block seems unused?

> Source/WebCore/bindings/js/JSNodeOrString.cpp:49
> +    using StringTypeList = typename ConverterType::StringTypeList;

All this block seems unused?

> Source/WebCore/bindings/js/JSNodeOrString.cpp:59
> +    Vector<typename NodeOrStringType::ImplementationType> result;

This gets resolved as a Vector<std::experimental::variant<Ref<Node>, String>>, right?

[Sam Weinig]

Committed r206691: <http://trac.webkit.org/changeset/206691>