WIP bindings: Add support for the record<K,V> WebIDL type.

WIP bindings: Add support for the record<K,V> WebIDL type.

Records (https://heycam.github.io/webidl/#idl-record) are ordered
associative arrays mapping instances of K to instances of V, where K must be
a ByteString, a DOMString or an USVString.

The current implementation represents records in C++ as
Vector<std::pair<k,v>>, which are converted from V8 objects via custom
toImpl<RecordName>() functions. To convert the C++ Vectors back to V8
objects, we piggyback on an already existing ToV8() overload for
Vector<std::pair<>>.

BUG=685754

[Raphael Kubo da Costa (rakuco), 2017-02-22 18:08:37]

raphael.kubo.da.costa@intel.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org, yukishiino@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-02-22 18:08:38]

PTAL at this WIP patch.

It's not finished, but I'd like to discuss the architecture of this change, as
well as ask some questions.

As for the architecture: the patch looks more complicated than I wanted, mostly
because a lot of the bindings scripts had to be changed in order to generate
individual <RecordName>.{cpp,h} bindings files. And those files are only being
generated because I need the custom V8->C++ conversion code coming from
v8_types.py and utilities.cpp.tmpl in order to convert the keys and values
correctly. If I was able to get rid of that Jinja substitution, it should be
possible to reduce the Python code quite a bit.

Questions:
- I'd appreciate suggestions on how to approach testing here. I can add some
operations to Internals.idl that take or return records, but the most
comprehensive test suite for this type at the moment is web-platform-tests'
fetch/api/headers/headers-record.html. However, we currently skip fetch/
entirely in the w-p-t imports. Should I just copy that test somewhere else for
now?
- There's also the problem that headers-record.html relies on Headers accepting
a record (our implementation currently takes a Dictionary), and I wanted to land
the changes to Headers after this CL. Should I put it all together here?
- Finally, another problem with headers-record.html is that all tests there
assume the union bindings code detects whether a sequence as passed differently.
We currently just do an if (v8Value->IsArray()) { call toImplArray(); }, whereas
the current WebIDL algorithm just checks whether @@iterator is present. Changing
that is quite easy, but toImplArray() and toMemberNativeArray() do not currently
handle objects with @@iterator at all and reject objects with custom iterators,
for example. In fact, they shouldn't exist at all and toImplSequence() should
always be used, but...
- toMemberNativeArray() and toImplArray() call V8TypeOf<T>::Type::toImpl() and
NativeTraits<T>::nativeValue() respectively to convert V8 data, which is quite
inconvenient. Would removing toImplSequence() in favor of duplicating its code
to toMemberNativeArray() and toImplArray() be OK (the only difference is in how
the values are converted to C++)?

[haraken, 2017-02-22 23:37:53]

(I'll defer this review to yukishiino@ or bashi@.)

Partial answers inline:

On 2017/02/22 18:08:38, Raphael Kubo da Costa (rakuco) wrote:
> PTAL at this WIP patch.
> 
> It's not finished, but I'd like to discuss the architecture of this change, as
> well as ask some questions.
> 
> As for the architecture: the patch looks more complicated than I wanted,
mostly
> because a lot of the bindings scripts had to be changed in order to generate
> individual <RecordName>.{cpp,h} bindings files. And those files are only being
> generated because I need the custom V8->C++ conversion code coming from
> v8_types.py and utilities.cpp.tmpl in order to convert the keys and values
> correctly. If I was able to get rid of that Jinja substitution, it should be
> possible to reduce the Python code quite a bit.
> 
> Questions:
> - I'd appreciate suggestions on how to approach testing here. I can add some
> operations to Internals.idl that take or return records, but the most
> comprehensive test suite for this type at the moment is web-platform-tests'
> fetch/api/headers/headers-record.html. However, we currently skip fetch/
> entirely in the w-p-t imports. Should I just copy that test somewhere else for
> now?

Can you use a similar mechanism to DictionaryTest.idl?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/D...

> - There's also the problem that headers-record.html relies on Headers
accepting
> a record (our implementation currently takes a Dictionary), and I wanted to
land
> the changes to Headers after this CL. Should I put it all together here?

Sounds fine. I want to keep this CL as small as possible.

> - Finally, another problem with headers-record.html is that all tests there
> assume the union bindings code detects whether a sequence as passed
differently.
> We currently just do an if (v8Value->IsArray()) { call toImplArray(); },
whereas
> the current WebIDL algorithm just checks whether @@iterator is present.
Changing
> that is quite easy, but toImplArray() and toMemberNativeArray() do not
currently
> handle objects with @@iterator at all and reject objects with custom
iterators,
> for example. In fact, they shouldn't exist at all and toImplSequence() should
> always be used, but...
> - toMemberNativeArray() and toImplArray() call V8TypeOf<T>::Type::toImpl() and
> NativeTraits<T>::nativeValue() respectively to convert V8 data, which is quite
> inconvenient. Would removing toImplSequence() in favor of duplicating its code
> to toMemberNativeArray() and toImplArray() be OK (the only difference is in
how
> the values are converted to C++)?

Sounds fine to me.

[bashi, 2017-02-23 00:43:00]

Thanks for the CL!

On 2017/02/22 23:37:53, haraken wrote:
> (I'll defer this review to yukishiino@ or bashi@.)
> 
> Partial answers inline:
> 
> On 2017/02/22 18:08:38, Raphael Kubo da Costa (rakuco) wrote:
> > PTAL at this WIP patch.
> > 
> > It's not finished, but I'd like to discuss the architecture of this change,
as
> > well as ask some questions.
> > 
> > As for the architecture: the patch looks more complicated than I wanted,
> mostly
> > because a lot of the bindings scripts had to be changed in order to generate
> > individual <RecordName>.{cpp,h} bindings files. And those files are only
being
> > generated because I need the custom V8->C++ conversion code coming from
> > v8_types.py and utilities.cpp.tmpl in order to convert the keys and values
> > correctly. If I was able to get rid of that Jinja substitution, it should be
> > possible to reduce the Python code quite a bit.

Could you elaborate why you couldn't just put V8 -> C++ conversion code under
Source/bindings/core/v8? Looks like record<K, V> will be converted to
{Heap}Vector<std::pair<K, V>>, so maybe we can write a helper function
toImplRecord() like toImplSequence()?

You can move codes from v8_types.py and utilities.cpp.tmpl to V8Binding.h if
needed.

> > 
> > Questions:
> > - I'd appreciate suggestions on how to approach testing here. I can add some
> > operations to Internals.idl that take or return records, but the most
> > comprehensive test suite for this type at the moment is web-platform-tests'
> > fetch/api/headers/headers-record.html. However, we currently skip fetch/
> > entirely in the w-p-t imports. Should I just copy that test somewhere else
for
> > now?
> 
> Can you use a similar mechanism to DictionaryTest.idl?
Yeah, I'd prefer to have a dedicated test for record types rather than depending
on customers' tests.

> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/testing/D...
> 
> > - There's also the problem that headers-record.html relies on Headers
> accepting
> > a record (our implementation currently takes a Dictionary), and I wanted to
> land
> > the changes to Headers after this CL. Should I put it all together here?
> 
> Sounds fine. I want to keep this CL as small as possible.
> 
> > - Finally, another problem with headers-record.html is that all tests there
> > assume the union bindings code detects whether a sequence as passed
> differently.
> > We currently just do an if (v8Value->IsArray()) { call toImplArray(); },
> whereas
> > the current WebIDL algorithm just checks whether @@iterator is present.
> Changing
> > that is quite easy, but toImplArray() and toMemberNativeArray() do not
> currently
> > handle objects with @@iterator at all and reject objects with custom
> iterators,
> > for example. In fact, they shouldn't exist at all and toImplSequence()
should
> > always be used, but...
> > - toMemberNativeArray() and toImplArray() call V8TypeOf<T>::Type::toImpl()
and
> > NativeTraits<T>::nativeValue() respectively to convert V8 data, which is
quite
> > inconvenient. Would removing toImplSequence() in favor of duplicating its
code
> > to toMemberNativeArray() and toImplArray() be OK (the only difference is in
> how
> > the values are converted to C++)?
> 
> Sounds fine to me.

I think we need to be careful about merging them. We should make sure there
isn't performance regression. Probably we can just change union bindings code to
use toImplSeqence()?

[Raphael Kubo da Costa (rakuco), 2017-02-23 08:13:53]

On 2017/02/23 00:43:00, bashi wrote:
> > > As for the architecture: the patch looks more complicated than I wanted,
> > mostly
> > > because a lot of the bindings scripts had to be changed in order to
generate
> > > individual <RecordName>.{cpp,h} bindings files. And those files are only
> being
> > > generated because I need the custom V8->C++ conversion code coming from
> > > v8_types.py and utilities.cpp.tmpl in order to convert the keys and values
> > > correctly. If I was able to get rid of that Jinja substitution, it should
be
> > > possible to reduce the Python code quite a bit.
> 
> Could you elaborate why you couldn't just put V8 -> C++ conversion code under
> Source/bindings/core/v8? Looks like record<K, V> will be converted to
> {Heap}Vector<std::pair<K, V>>, so maybe we can write a helper function
> toImplRecord() like toImplSequence()?
> 
> You can move codes from v8_types.py and utilities.cpp.tmpl to V8Binding.h if
> needed.

I tried doing this before resorting to generating bindings for each record type.
The problem with a generic toImplRecord() is knowing how to convert the key and
value JS types to C++ ones. For example: ByteString, DOMString and USVString all
map to WTF::String, but each must be converted differently (e.g. toByteString(),
toUSVString()). The same applies to float/restricted float etc. This information
is all in v8_types.py at the moment, and I don't see how to easily move it to
V8Binding.h without having to pass a lot of IDL information to it. One would
have to essentially move v8_value_to_cpp_value(),
v8_value_to_cpp_value_array_or_sequence(), v8_value_to_local_cpp_value() and
helper functions and variables to C++.

> I think we need to be careful about merging them. We should make sure there
> isn't performance regression. Probably we can just change union bindings code
to
> use toImplSeqence()?

I think doing so would convert most callers to toImplSequence() automatically,
which essentially means performing the merge, doesn't it?

[Raphael Kubo da Costa (rakuco), 2017-02-23 18:06:10]

I've uploaded a new version of the patch with a layout test for the new record
type. I think it covers a lot of different possibilities, including unions and
nullable records.

This allows us to focus on supporting the new IDL type and worry about
headers-records.html later. Consequently, if you prefer we can also defer and
move the discussion about @@iterator and toImplArray() to a bug.

The biggest issue at the moment is the fact that we still generate per-record
type bindings, as I mentioned in my first message.

[bashi, 2017-02-24 00:17:30]

On 2017/02/23 18:06:10, Raphael Kubo da Costa (rakuco) wrote:
> I've uploaded a new version of the patch with a layout test for the new record
> type. I think it covers a lot of different possibilities, including unions and
> nullable records.
> 
> This allows us to focus on supporting the new IDL type and worry about
> headers-records.html later. Consequently, if you prefer we can also defer and
> move the discussion about @@iterator and toImplArray() to a bug.

Thanks for doing this. +1 to focus on the biggest problem (generating per-record
types).

> 
> The biggest issue at the moment is the fact that we still generate per-record
> type bindings, as I mentioned in my first message.

Have you tried using NativeValueTraits<T>::nativeValue()? UUIC you need to
generate record types only for V8 -> C++ conversion. NativeValueTraits<T> may
not have enough coverage but I think we can extend it as needed by hand or by
the code generator. The code generator already generates NativeValueTraits for
dictionary and unions. For nullable types I think we can add a helper template
function that wraps NativeValueTraits<T>::nativeValue().

[Raphael Kubo da Costa (rakuco), 2017-02-24 08:05:52]

On 2017/02/24 00:17:30, bashi wrote:
> Have you tried using NativeValueTraits<T>::nativeValue()? UUIC you need to
> generate record types only for V8 -> C++ conversion. NativeValueTraits<T> may
> not have enough coverage but I think we can extend it as needed by hand or by
> the code generator. The code generator already generates NativeValueTraits for
> dictionary and unions. For nullable types I think we can add a helper template
> function that wraps NativeValueTraits<T>::nativeValue().

I tried doing this in the beginning. The problem is the same I mentioned before:
some IDL types map to the same C++ type. This is particularly tricky for the
string types that are used as record key types. For example, a
record<ByteString, V> must call toByteString() and throw a TypeError if an
invalid string was passed, whereas a record<DOMString, V> shouldn't do any
validation. But how do you choose the behavior with a single
NativeValueTraits<String>::nativeValue() overload? Ditto for float/unrestricted
float and others.

[bashi, 2017-02-24 08:38:14]

On 2017/02/24 08:05:52, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/02/24 00:17:30, bashi wrote:
> > Have you tried using NativeValueTraits<T>::nativeValue()? UUIC you need to
> > generate record types only for V8 -> C++ conversion. NativeValueTraits<T>
may
> > not have enough coverage but I think we can extend it as needed by hand or
by
> > the code generator. The code generator already generates NativeValueTraits
for
> > dictionary and unions. For nullable types I think we can add a helper
template
> > function that wraps NativeValueTraits<T>::nativeValue().
> 
> I tried doing this in the beginning. The problem is the same I mentioned
before:
> some IDL types map to the same C++ type. This is particularly tricky for the
> string types that are used as record key types. For example, a
> record<ByteString, V> must call toByteString() and throw a TypeError if an
> invalid string was passed, whereas a record<DOMString, V> shouldn't do any
> validation. But how do you choose the behavior with a single
> NativeValueTraits<String>::nativeValue() overload? Ditto for
float/unrestricted
> float and others.

Probably we can add a helper (template) function which takes two type parameters
and uses some kind of template technique slike SFINAE? This would be almost the
same as moving logic from .py and .tmpl to .cpp/.h, but if strings and numbers
are only instances of this problem, it would be better than generating record
types.

At least we should generate only V8 -> C++ conversion logic and most of logic in
record_class.cpp.tmpl should be defined statistically.

[Raphael Kubo da Costa (rakuco), 2017-02-24 17:12:16]

I've uploaded a new patch set that implements a mostly static toImplRecord().
The required template changes are in V8BindingRecord.h for now instead of in
V8Binding.h and ToV8.h because I don't want to have to rebuild thousands of
files every time I touch them.

With that said, the gains don't look very big: the conversion functions in
record_class.cpp.tmpl take a very ugly bool&, and the same bindings scripts are
still being changed.

I'm still interested in something template-based for the V8->C++ conversions,
but I think having that working would be a big undertaking: the bindings code
would have to look more like WebKit's, where most of their conversion
information is in C++ classes with a uniform interface. The situation in Blink
looks more complicated, as the conversion functions that need to be called vary
among (and so does the way they must be called): string types, numeric types,
different interface types, ScriptValue, Window, Promises, ArrayBuffers, callback
functions etc.

[bashi, 2017-02-27 09:13:15]

On 2017/02/24 17:12:16, Raphael Kubo da Costa (rakuco) wrote:
> I've uploaded a new patch set that implements a mostly static toImplRecord().
> The required template changes are in V8BindingRecord.h for now instead of in
> V8Binding.h and ToV8.h because I don't want to have to rebuild thousands of
> files every time I touch them.
> 
> With that said, the gains don't look very big: the conversion functions in
> record_class.cpp.tmpl take a very ugly bool&, and the same bindings scripts
are
> still being changed.
> 
> I'm still interested in something template-based for the V8->C++ conversions,
> but I think having that working would be a big undertaking: the bindings code
> would have to look more like WebKit's, where most of their conversion
> information is in C++ classes with a uniform interface. The situation in Blink
> looks more complicated, as the conversion functions that need to be called
vary
> among (and so does the way they must be called): string types, numeric types,
> different interface types, ScriptValue, Window, Promises, ArrayBuffers,
callback
> functions etc.

Thank you for being patient and keep updating the CL. We really appreciate it.

However, I still don't want to generate record types. IDL(V8) -> C++ conversion
isn't specific to record<K, V>. We should have a generic way to handle this kind
of issue.

Let me think about other options. In the meanwhile, can you split this CL into
small ones? Probably we can start with adding IdlRecordType in idl_types.py and
record_node_to_type() in idl_definitions.py

[Raphael Kubo da Costa (rakuco), 2017-02-27 09:26:33]

On 2017/02/27 09:13:15, bashi wrote:
> Thank you for being patient and keep updating the CL. We really appreciate it.

Thanks for the review. I didn't think I was ever going to say this, but hacking
the bindings code has actually been fun :-)

> However, I still don't want to generate record types. IDL(V8) -> C++
conversion
> isn't specific to record<K, V>. We should have a generic way to handle this
kind
> of issue.

I agree, I think the same problem affects at least sequences as well. One
possibility that comes to mind (I haven't checked how feasible it is) would be
gathering all types that need V8->C++ conversions (from unions, records,
interfaces etc) and generating a
gen/blink/bindings/{core,modules}/v8/ToImpl.{cpp,h} that offers conversion
functions with a uniform interface, to be invoked from the templates.

> In the meanwhile, can you split this CL into
> small ones? Probably we can start with adding IdlRecordType in idl_types.py
and
> record_node_to_type() in idl_definitions.py

Sure!

[Raphael Kubo da Costa (rakuco), 2017-02-27 18:33:37]

On 2017/02/27 09:26:33, Raphael Kubo da Costa (rakuco) wrote:
> I agree, I think the same problem affects at least sequences as well. One
> possibility that comes to mind (I haven't checked how feasible it is) would be
> gathering all types that need V8->C++ conversions (from unions, records,
> interfaces etc) and generating a
> gen/blink/bindings/{core,modules}/v8/ToImpl.{cpp,h} that offers conversion
> functions with a uniform interface, to be invoked from the templates.

I sort of toyed with this idea today. It's possible to generalize
NativeValueTraits a little bit and add NativeValueTraits wrappers for most calls
in v8_types.py's V8_VALUE_TO_CPP_VALUE. Even if we don't even consider records
in this context, there's still the problem of using NativeValueTraits for
sequences, as we do not generate any bindings files for them. I think having a
bindings-generated ToImpl.{cpp,h} for these conversions might indeed be a good
idea.

[bashi, 2017-02-27 23:20:13]

On 2017/02/27 18:33:37, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/02/27 09:26:33, Raphael Kubo da Costa (rakuco) wrote:
> > I agree, I think the same problem affects at least sequences as well. One
> > possibility that comes to mind (I haven't checked how feasible it is) would
be
> > gathering all types that need V8->C++ conversions (from unions, records,
> > interfaces etc) and generating a
> > gen/blink/bindings/{core,modules}/v8/ToImpl.{cpp,h} that offers conversion
> > functions with a uniform interface, to be invoked from the templates.
> 
> I sort of toyed with this idea today. It's possible to generalize
> NativeValueTraits a little bit and add NativeValueTraits wrappers for most
calls
> in v8_types.py's V8_VALUE_TO_CPP_VALUE. Even if we don't even consider records
> in this context, there's still the problem of using NativeValueTraits for
> sequences, as we do not generate any bindings files for them. I think having a
> bindings-generated ToImpl.{cpp,h} for these conversions might indeed be a good
> idea.

Yeah, I was thinking a similar idea but wasn't sure it's feasible/possible. I
haven't tried the idea by myself but it seems you've already tried it:) If we
can generate ToImpl{Core,Modules}.{cpp,h}, that would be a better idea!

[Raphael Kubo da Costa (rakuco), 2017-03-01 18:52:52]

I've uploaded a new version of the patch incorporating the comments Yuki made in
https://codereview.chromium.org/2725673002/.

The changes to bindings/scripts are a lot smaller, at the expense of a lot of
changes to the bindings test expectations caused by having to add
NativeValueTraits to the dictionary and union templates.

Things still work, but I'm not convinced this looks nice: IDLTypes.h is
basically disconnected from all the rest of the bindings code except for
records, and I guess it needs a lot of testing now that the other data types are
not exercising those code paths any longer.

Please take a look.

[bashi, 2017-03-01 23:59:55]

On 2017/03/01 18:52:52, Raphael Kubo da Costa (rakuco) wrote:
> I've uploaded a new version of the patch incorporating the comments Yuki made
in
> https://codereview.chromium.org/2725673002/.
> 
> The changes to bindings/scripts are a lot smaller, at the expense of a lot of
> changes to the bindings test expectations caused by having to add
> NativeValueTraits to the dictionary and union templates.
> 
> Things still work, but I'm not convinced this looks nice: IDLTypes.h is
> basically disconnected from all the rest of the bindings code except for
> records, and I guess it needs a lot of testing now that the other data types
are
> not exercising those code paths any longer.
> 
> Please take a look.

Yeah, ideally we should use IDLTypes.h everywhere we need IDL -> V8 conversions,
but I like yukishiino@'s plan (i.e. your PS# 5). Your concern makes sense, but
we also want to be conservative about changes that affect performance. Record<K,
V> is a new thing so we don't need to worry about the performance too much at
this moment. I think this is a right way forward.

[bashi, 2017-03-02 00:04:38]

If yukishiino@ is happy with this approach, could you split this CL into small
ones? Maybe we can start with adding IDLTypes.h.

[Yuki, 2017-03-02 07:47:28]

Overall, looks great.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:19: namespace idl {
Do we have a consensus to introduce a new nested namespace "idl"?
If not, I'd recommend to get agreement on platform-architecture-dev@chromium.org
.

By the way, I'd slightly prefer "webidl" just in case that we'll have another
IDL.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (left):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:18: static T
nativeValue(v8::Isolate*,
If you'd like to remove this declaration, could you put a comment instead?

The empty struct seems meaningless.  It's unfriendly for code readers.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:906: WARN_UNUSED_RESULT
CORE_EXPORT v8::MaybeLocal<v8::String> getStringValueInArray(
nit: I'm on the fence whether the name of |getStringValueInArray| is too generic
in global namespace or not.

Both of String and Array could be understood as WTF::String or WTF::Vector for
example.  If this is a helper function in local namespace, I'm fine.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:968:
exceptionState.rethrowV8Exception(block.Exception());
Not directly related to your CL, we may want an utility to do this
automatically.

    v8::TryCatch tryCatch(isolate);
    ExceptionStateRethrowScope rethrowScope(exceptionState, tryCatch);

    if (failure) {
      // rethrowScope's dtor automatically does the job.
      return VectorType();
    }

We're seeing this pattern too often.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl:34: using
ImplType = {{cpp_class}};
nit: Can we have this declaration as the default definition of ImplType in
NativeValueTraits.h ?

[Raphael Kubo da Costa (rakuco), 2017-03-02 08:16:24]

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:19: namespace idl {
On 2017/03/02 07:47:28, Yuki(slow_til_mar08) wrote:
> Do we have a consensus to introduce a new nested namespace "idl"?
> If not, I'd recommend to get agreement on
mailto:platform-architecture-dev@chromium.org
> .
> 
> By the way, I'd slightly prefer "webidl" just in case that we'll have another
> IDL.

We don't, I introduced this namespace while writing the code to see if it would
work. I'm also OK with following what WebKit does and just calling the types
"IDLFoo" if you prefer.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl:34: using
ImplType = {{cpp_class}};
On 2017/03/02 07:47:28, Yuki(slow_til_mar08) wrote:
> nit: Can we have this declaration as the default definition of ImplType in
> NativeValueTraits.h ?

I tried doing that earlier, but any template specialization hides the definition
so it didn't work and I had to add those back.

[Yuki, 2017-03-02 13:00:25]

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:19: namespace idl {
On 2017/03/02 08:16:24, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/02 07:47:28, Yuki(slow_til_mar08) wrote:
> > Do we have a consensus to introduce a new nested namespace "idl"?
> > If not, I'd recommend to get agreement on
> mailto:platform-architecture-dev@chromium.org
> > .
> > 
> > By the way, I'd slightly prefer "webidl" just in case that we'll have
another
> > IDL.
> 
> We don't, I introduced this namespace while writing the code to see if it
would
> work. I'm also OK with following what WebKit does and just calling the types
> "IDLFoo" if you prefer.

I'm fine with either way.  "IDLFoo" seems okay to me, too.

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl (right):

https://codereview.chromium.org/2709983004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/dictionary_v8.h.tmpl:34: using
ImplType = {{cpp_class}};
On 2017/03/02 08:16:24, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/02 07:47:28, Yuki(slow_til_mar08) wrote:
> > nit: Can we have this declaration as the default definition of ImplType in
> > NativeValueTraits.h ?
> 
> I tried doing that earlier, but any template specialization hides the
definition
> so it didn't work and I had to add those back.

I guess that the following would work.

  template <typename T>
  struct NativeValueTraitsBase {
    using ImplType = T;
  };

  template <typename T>
  struct NativeValueTraits : public NativeValueTraitsBase<T> {
     // ...
  };

  template <>
  struct NativeValueTraits<ActualType> : public
NativeValueTraitsBase<ActualType> {
    // ...
  };

Having said that, it's not shorter than the current CL, although we can easily
add more things in NativeValueTraitsBase.  I'm fine with either way.

[Raphael Kubo da Costa (rakuco), 2017-03-09 15:34:06]

Closing this one; the bits and pieces are being landed separately, the last of
which is https://codereview.chromium.org/2732093003/