bindings: Add C++ versions of WebIDL types and generalize NativeValueTraits.

bindings: Add C++ versions of WebIDL types and generalize NativeValueTraits.

This being introduced primarily in order to support the record<K,V> WebIDL
type, as it needs to be able to convert arbitrary JS objects into their C++
counterparts without all the information available to the IDL code generator
at build-time.

The changes are two-fold:

1. IDLTypes.h contains C++ classes corresponding to the basic WebIDL
   types. At the moment they are limited to booleans, numbers, strings,
   promises and sequences. All classes inherit from IDLBase, and provide
   information about their corresponding C++ types (e.g. an IDLOctet
   corresponds to an uint8_t).

2. NativeValueTraits was expanded to cope with both the existing template
   specializations which essentially have the type one expects nativeValue()
   to return (e.g. AtomicString, double etc) as well as the new variations
   introduced for the IDL types above: when NativeValueTraits is specialized
   for an IDLBase-based type, all type information is derived from it.

The idea behind 1) and 2) is that NativeValueTraits<T>::nativeValue() should
be a standard interface for converting any JS value to C++ by interpreting
the JS value as a T.

Wherever possible, v8_types.py now generates invocations to
NativeValueTraits<T>::nativeValue() instead of direct calls to type-specific
conversion functions. It was not done when the conversion functions
themselves take arguments that do not match nativeValue()'s signature. For
example, converting to a Promise or an IDL interface does not require an
ExceptionState, so using nativeValue() would introduce unnecessary overhead
by requiring an ExceptionState to be created and passed.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2730183003
Cr-Commit-Position: refs/heads/master@{#455045}
Committed: https://chromium.googlesource.com/chromium/src/+/504550f038ae920fbc2bee9797f298189262e4fc

[Raphael Kubo da Costa (rakuco), 2017-03-04 18:55:40]

PTAL. This implements Yuki's suggestion in
https://codereview.chromium.org/2709983004/ to split up the work, with the twist
that I'm converting some of the v8_types.py calls to e.g. toUint32() into
nativeValue() calls. I'm only doing that where no new ExceptionState needs to be
created, so the end result should be the same and there should be no performance
overhead. I can also undo that part of the change if you prefer.

The rest is straight-forward: IDLTypes.h (without records), changes to
NativeValueTraits.h, unit tests and updates to the bindings test expectations.

[haraken, 2017-03-05 07:08:27]

yukishiino: Would you take a look at this?

[Yuki, 2017-03-06 07:39:42]

This CL looks great!
Makes the codebase a lot cleaner.

I have minor comments only.

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:56: struct MaybeWrapped {
MaybeWrapped and NeedsHeapVector are used only inside IDLSequence, right?

If so, could you move these helper classes into IDLSequence as nested classes?
These names are short and somewhat general, so I don't want to introduce these
names in the global namespace.

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:8: #include
<v8.h>
Don't change this #include.
Our new convention is to treat v8.h as an user-defined header, not as a system
header.
So, this should be "v8/include/v8.h".

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
I think that we don't need |Args| here.
We can add IntegerConversionConfiguration without Args here.

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8BindingForIDLTypes.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8BindingForIDLTypes.h:6: #define
V8BindingForIDLTypes_h
I think that "NativeValueTraitsImpl.h" is more an understandable / more straight
forward name.

[Raphael Kubo da Costa (rakuco), 2017-03-06 07:47:22]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:56: struct MaybeWrapped {
On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> MaybeWrapped and NeedsHeapVector are used only inside IDLSequence, right?
> 
> If so, could you move these helper classes into IDLSequence as nested classes?
> These names are short and somewhat general, so I don't want to introduce these
> names in the global namespace.

They're only used by IDLSequence at the moment, but IDLRecord also needs them. I
can either duplicate the code in both IDL classes or try to come up with less
general names for them (suggestions welcome :-)

[Yuki, 2017-03-06 07:58:19]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:56: struct MaybeWrapped {
On 2017/03/06 07:47:22, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> > MaybeWrapped and NeedsHeapVector are used only inside IDLSequence, right?
> > 
> > If so, could you move these helper classes into IDLSequence as nested
classes?
> > These names are short and somewhat general, so I don't want to introduce
these
> > names in the global namespace.
> 
> They're only used by IDLSequence at the moment, but IDLRecord also needs them.
I
> can either duplicate the code in both IDL classes or try to come up with less
> general names for them (suggestions welcome :-)

I don't think that this is a super great idea, but we can put them inside
another class.
For example, we can put them inside IDLBaseHelper, or a new dedicated
namespace-like class, e.g. IDLTypesHelper or IDLTypesUtil.

Well, we don't yet have a general consensus about nested namespaces, but
namespace-like STATIC_ONLY classes are okay (for historical reasons?), I think. 
Putting a prefix (like IDLTypes) is okay, too.

[bashi, 2017-03-06 08:04:54]

I'll defer to yukishiino@'s review. I just want to say this is a great CL :)
Once we have generalized nativeValue() we can simplify our codebase one-by-one
(e.g. I think we can remove DictionaryHelper).

[Raphael Kubo da Costa (rakuco), 2017-03-06 08:06:07]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> I think that we don't need |Args| here.
> We can add IntegerConversionConfiguration without Args here.

|Args| is not needed indeed. I left it there to keep it somewhat similar to what
was there before and also because having only T would break NativeValueTraits<T>
in V8BindingForIDLTypes.h. Would "<typename T, typename ...>" be better just to
make this the primary template?

[Yuki, 2017-03-06 08:14:19]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:26: template
<typename T, typename U = void>
Why do we need |U| here?

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 08:06:07, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> > I think that we don't need |Args| here.
> > We can add IntegerConversionConfiguration without Args here.
> 
> |Args| is not needed indeed. I left it there to keep it somewhat similar to
what
> was there before and also because having only T would break
NativeValueTraits<T>
> in V8BindingForIDLTypes.h. Would "<typename T, typename ...>" be better just
to
> make this the primary template?

Why and how does it break?  Is there any NativeValueTraits that takes two
template arguments?
I don't understand what would break.

[Raphael Kubo da Costa (rakuco), 2017-03-06 08:36:52]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:26: template
<typename T, typename U = void>
On 2017/03/06 08:14:19, Yuki(slow_til_mar08) wrote:
> Why do we need |U| here?

For the other NativeValueTraitsBase<> declaration below to work (SFINAE).

If a type inherits from IDLBase, std::enable_if<> below expands into a valid
declaration and it's used. If a type does not inherit from IDLBase,
std::enable_if<>::type doesn't exist and the template will expand into an
invalid code. If I remove |U|, I get build failures such as this one:

../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:33:8:
error: too many template arguments for class template 'NativeValueTraitsBase'
struct NativeValueTraitsBase<
       ^
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:27:8: note:
template is declared here
struct NativeValueTraitsBase {
       ^

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 08:14:19, Yuki(slow_til_mar08) wrote:
> On 2017/03/06 08:06:07, Raphael Kubo da Costa (rakuco) wrote:
> > On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> > > I think that we don't need |Args| here.
> > > We can add IntegerConversionConfiguration without Args here.
> > 
> > |Args| is not needed indeed. I left it there to keep it somewhat similar to
> what
> > was there before and also because having only T would break
> NativeValueTraits<T>
> > in V8BindingForIDLTypes.h. Would "<typename T, typename ...>" be better just
> to
> > make this the primary template?
> 
> Why and how does it break?  Is there any NativeValueTraits that takes two
> template arguments?
> I don't understand what would break.

The build failure looks like this:

In file included from gen/blink/bindings/core/v8/V8DOMPointInit.cpp:16:
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
error: class template partial specialization does not specialize any template
argument; to define the primary template, remove the template argument list
struct NativeValueTraits<T> : public NativeValueTraitsBase<T> {
       ^                ~~~
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
error: redefinition of 'NativeValueTraits'
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:61:8: note:
previous definition is here
struct NativeValueTraits : public NativeValueTraitsBase<T> {
       ^

because there's
  template <typename T>
  class NativeValueTraits : public NativeValueTraitsBase<T> {
  };

and V8BindingForIDLTypes.h has

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

so the latter doesn't look like a specialization of the former.

[Yuki, 2017-03-06 09:03:42]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:26: template
<typename T, typename U = void>
On 2017/03/06 08:36:52, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 08:14:19, Yuki(slow_til_mar08) wrote:
> > Why do we need |U| here?
> 
> For the other NativeValueTraitsBase<> declaration below to work (SFINAE).
> 
> If a type inherits from IDLBase, std::enable_if<> below expands into a valid
> declaration and it's used. If a type does not inherit from IDLBase,
> std::enable_if<>::type doesn't exist and the template will expand into an
> invalid code. If I remove |U|, I get build failures such as this one:
> 
> ../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:33:8:
> error: too many template arguments for class template 'NativeValueTraitsBase'
> struct NativeValueTraitsBase<
>        ^
> ../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:27:8:
note:
> template is declared here
> struct NativeValueTraitsBase {
>        ^

I see.

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 08:36:52, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 08:14:19, Yuki(slow_til_mar08) wrote:
> > On 2017/03/06 08:06:07, Raphael Kubo da Costa (rakuco) wrote:
> > > On 2017/03/06 07:39:42, Yuki(slow_til_mar08) wrote:
> > > > I think that we don't need |Args| here.
> > > > We can add IntegerConversionConfiguration without Args here.
> > > 
> > > |Args| is not needed indeed. I left it there to keep it somewhat similar
to
> > what
> > > was there before and also because having only T would break
> > NativeValueTraits<T>
> > > in V8BindingForIDLTypes.h. Would "<typename T, typename ...>" be better
just
> > to
> > > make this the primary template?
> > 
> > Why and how does it break?  Is there any NativeValueTraits that takes two
> > template arguments?
> > I don't understand what would break.
> 
> The build failure looks like this:
> 
> In file included from gen/blink/bindings/core/v8/V8DOMPointInit.cpp:16:
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
> error: class template partial specialization does not specialize any template
> argument; to define the primary template, remove the template argument list
> struct NativeValueTraits<T> : public NativeValueTraitsBase<T> {
>        ^                ~~~
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
> error: redefinition of 'NativeValueTraits'
> ../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:61:8:
note:
> previous definition is here
> struct NativeValueTraits : public NativeValueTraitsBase<T> {
>        ^
> 
> because there's
>   template <typename T>
>   class NativeValueTraits : public NativeValueTraitsBase<T> {
>   };
> 
> and V8BindingForIDLTypes.h has
> 
>   // Generated IDL interfaces.
>   template <typename T>
>   struct NativeValueTraits<T> : public NativeValueTraitsBase<T> {
>     // ...
>   };
> 
> so the latter doesn't look like a specialization of the former.

This seems a wrong solution.  I now see an issue that you need to specialize
NativeValueTraits for all V8Types, but adding another template parameter seems
wrong to me.

What we'd like to say is:

template <typename T>
struct NativeValueTraits<T where T inherits from ScriptWrappable / T has a
corresponding V8T>

We need to translate this into code.  For example,

template<typename T>
struct NativeValueTraits<V8TypeOf<T>::nativeType> : public ... {};
// This doesn't work because there is no |nativeType|.

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

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 09:03:42, Yuki(slow_til_mar08) wrote:
> This seems a wrong solution.  I now see an issue that you need to specialize
> NativeValueTraits for all V8Types, but adding another template parameter seems
> wrong to me.
> 
> What we'd like to say is:
> 
> template <typename T>
> struct NativeValueTraits<T where T inherits from ScriptWrappable / T has a
> corresponding V8T>
> 
> We need to translate this into code.  For example,
> 
> template<typename T>
> struct NativeValueTraits<V8TypeOf<T>::nativeType> : public ... {};
> // This doesn't work because there is no |nativeType|.

We're quite limited in what we can do here if the primary NativeValueTraits
template only takes one parameter T.

If I change the V8BindingForIDLTypes.h specialization to:

template <typename T>
struct NativeValueTraits<typename V8TypeOf<T>::Type>
    : public NativeValueTraitsBase<T> {

the build fails with

../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
error: class template partial specialization contains a template parameter that
cannot be deduced; this partial specialization will never be used
[-Wunusable-partial-specialization]
struct NativeValueTraits<typename V8TypeOf<T>::Type>
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:302:20:
note: non-deducible template parameter 'T'
template <typename T>
                   ^

which is the same error I message I get with

template <typename T>
struct NativeValueTraits<typename std::enable_if<std::is_class<typename
V8TypeOf<T>::Type>::value, T>::type>
    : public NativeValueTraitsBase<T> {

Adding template arguments is impossible, and so is adding more arguments to the
specialization.

For SFINAE to work here, the primary template would need at least one more
argument, for example:

template <typename T, typename U = void>
struct NativeValueTraits : public NativeValueTraitsBase<T> {};

template <typename T>
struct NativeValueTraits<T,
                         typename std::enable_if<std::is_class<typename
V8TypeOf<T>::Type>::value>::type>
    : public NativeValueTraitsBase<T> {
  // ...
};

which is what we achieve at the moment without the std::enable_if<>.

[Yuki, 2017-03-06 10:13:03]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 09:51:38, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 09:03:42, Yuki(slow_til_mar08) wrote:
> > This seems a wrong solution.  I now see an issue that you need to specialize
> > NativeValueTraits for all V8Types, but adding another template parameter
seems
> > wrong to me.
> > 
> > What we'd like to say is:
> > 
> > template <typename T>
> > struct NativeValueTraits<T where T inherits from ScriptWrappable / T has a
> > corresponding V8T>
> > 
> > We need to translate this into code.  For example,
> > 
> > template<typename T>
> > struct NativeValueTraits<V8TypeOf<T>::nativeType> : public ... {};
> > // This doesn't work because there is no |nativeType|.
> 
> We're quite limited in what we can do here if the primary NativeValueTraits
> template only takes one parameter T.
> 
> If I change the V8BindingForIDLTypes.h specialization to:
> 
> template <typename T>
> struct NativeValueTraits<typename V8TypeOf<T>::Type>
>     : public NativeValueTraitsBase<T> {
> 
> the build fails with
> 
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
> error: class template partial specialization contains a template parameter
that
> cannot be deduced; this partial specialization will never be used
> [-Wunusable-partial-specialization]
> struct NativeValueTraits<typename V8TypeOf<T>::Type>
>        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:302:20:
> note: non-deducible template parameter 'T'
> template <typename T>
>                    ^
> 
> which is the same error I message I get with
> 
> template <typename T>
> struct NativeValueTraits<typename std::enable_if<std::is_class<typename
> V8TypeOf<T>::Type>::value, T>::type>
>     : public NativeValueTraitsBase<T> {
> 
> Adding template arguments is impossible, and so is adding more arguments to
the
> specialization.
> 
> For SFINAE to work here, the primary template would need at least one more
> argument, for example:
> 
> template <typename T, typename U = void>
> struct NativeValueTraits : public NativeValueTraitsBase<T> {};
> 
> template <typename T>
> struct NativeValueTraits<T,
>                          typename std::enable_if<std::is_class<typename
> V8TypeOf<T>::Type>::value>::type>
>     : public NativeValueTraitsBase<T> {
>   // ...
> };
> 
> which is what we achieve at the moment without the std::enable_if<>.

What about the following?

template <typename CppType, typename V8Type = void>
struct NativeValueTraits;

Then, V8Type = V8Type<T>::type makes sense and deduct a template parameter, also
{CppType=bool, V8Type=void} would make a kind of sense because bool is a
primitive type.

Anyway, if we needed a hack for some purpose, I'd like to be explict.  Adding
"typename... Args" without a comment seems too implicit.

[Raphael Kubo da Costa (rakuco), 2017-03-06 10:40:36]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 10:13:03, Yuki(slow_til_mar08) wrote:
> On 2017/03/06 09:51:38, Raphael Kubo da Costa (rakuco) wrote:
> > On 2017/03/06 09:03:42, Yuki(slow_til_mar08) wrote:
> > > This seems a wrong solution.  I now see an issue that you need to
specialize
> > > NativeValueTraits for all V8Types, but adding another template parameter
> seems
> > > wrong to me.
> > > 
> > > What we'd like to say is:
> > > 
> > > template <typename T>
> > > struct NativeValueTraits<T where T inherits from ScriptWrappable / T has a
> > > corresponding V8T>
> > > 
> > > We need to translate this into code.  For example,
> > > 
> > > template<typename T>
> > > struct NativeValueTraits<V8TypeOf<T>::nativeType> : public ... {};
> > > // This doesn't work because there is no |nativeType|.
> > 
> > We're quite limited in what we can do here if the primary NativeValueTraits
> > template only takes one parameter T.
> > 
> > If I change the V8BindingForIDLTypes.h specialization to:
> > 
> > template <typename T>
> > struct NativeValueTraits<typename V8TypeOf<T>::Type>
> >     : public NativeValueTraitsBase<T> {
> > 
> > the build fails with
> > 
> >
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
> > error: class template partial specialization contains a template parameter
> that
> > cannot be deduced; this partial specialization will never be used
> > [-Wunusable-partial-specialization]
> > struct NativeValueTraits<typename V8TypeOf<T>::Type>
> >        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:302:20:
> > note: non-deducible template parameter 'T'
> > template <typename T>
> >                    ^
> > 
> > which is the same error I message I get with
> > 
> > template <typename T>
> > struct NativeValueTraits<typename std::enable_if<std::is_class<typename
> > V8TypeOf<T>::Type>::value, T>::type>
> >     : public NativeValueTraitsBase<T> {
> > 
> > Adding template arguments is impossible, and so is adding more arguments to
> the
> > specialization.
> > 
> > For SFINAE to work here, the primary template would need at least one more
> > argument, for example:
> > 
> > template <typename T, typename U = void>
> > struct NativeValueTraits : public NativeValueTraitsBase<T> {};
> > 
> > template <typename T>
> > struct NativeValueTraits<T,
> >                          typename std::enable_if<std::is_class<typename
> > V8TypeOf<T>::Type>::value>::type>
> >     : public NativeValueTraitsBase<T> {
> >   // ...
> > };
> > 
> > which is what we achieve at the moment without the std::enable_if<>.
> 
> What about the following?
> 
> template <typename CppType, typename V8Type = void>
> struct NativeValueTraits;
> 
> Then, V8Type = V8Type<T>::type makes sense and deduct a template parameter,
also
> {CppType=bool, V8Type=void} would make a kind of sense because bool is a
> primitive type.
> 
> Anyway, if we needed a hack for some purpose, I'd like to be explict.  Adding
> "typename... Args" without a comment seems too implicit.

How about being even more explicit with something like this:

template <typename T, typename SFINAEHelper = void>  // SFINAEHelper can be used
by template specializations that need to introspect T.
struct NativeValueTraits : public ... {
};

[Yuki, 2017-03-06 10:43:04]

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:60: template
<typename T, typename... Args>
On 2017/03/06 10:40:36, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 10:13:03, Yuki(slow_til_mar08) wrote:
> > On 2017/03/06 09:51:38, Raphael Kubo da Costa (rakuco) wrote:
> > > On 2017/03/06 09:03:42, Yuki(slow_til_mar08) wrote:
> > > > This seems a wrong solution.  I now see an issue that you need to
> specialize
> > > > NativeValueTraits for all V8Types, but adding another template parameter
> > seems
> > > > wrong to me.
> > > > 
> > > > What we'd like to say is:
> > > > 
> > > > template <typename T>
> > > > struct NativeValueTraits<T where T inherits from ScriptWrappable / T has
a
> > > > corresponding V8T>
> > > > 
> > > > We need to translate this into code.  For example,
> > > > 
> > > > template<typename T>
> > > > struct NativeValueTraits<V8TypeOf<T>::nativeType> : public ... {};
> > > > // This doesn't work because there is no |nativeType|.
> > > 
> > > We're quite limited in what we can do here if the primary
NativeValueTraits
> > > template only takes one parameter T.
> > > 
> > > If I change the V8BindingForIDLTypes.h specialization to:
> > > 
> > > template <typename T>
> > > struct NativeValueTraits<typename V8TypeOf<T>::Type>
> > >     : public NativeValueTraitsBase<T> {
> > > 
> > > the build fails with
> > > 
> > >
> >
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:303:8:
> > > error: class template partial specialization contains a template parameter
> > that
> > > cannot be deduced; this partial specialization will never be used
> > > [-Wunusable-partial-specialization]
> > > struct NativeValueTraits<typename V8TypeOf<T>::Type>
> > >        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> >
>
../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:302:20:
> > > note: non-deducible template parameter 'T'
> > > template <typename T>
> > >                    ^
> > > 
> > > which is the same error I message I get with
> > > 
> > > template <typename T>
> > > struct NativeValueTraits<typename std::enable_if<std::is_class<typename
> > > V8TypeOf<T>::Type>::value, T>::type>
> > >     : public NativeValueTraitsBase<T> {
> > > 
> > > Adding template arguments is impossible, and so is adding more arguments
to
> > the
> > > specialization.
> > > 
> > > For SFINAE to work here, the primary template would need at least one more
> > > argument, for example:
> > > 
> > > template <typename T, typename U = void>
> > > struct NativeValueTraits : public NativeValueTraitsBase<T> {};
> > > 
> > > template <typename T>
> > > struct NativeValueTraits<T,
> > >                          typename std::enable_if<std::is_class<typename
> > > V8TypeOf<T>::Type>::value>::type>
> > >     : public NativeValueTraitsBase<T> {
> > >   // ...
> > > };
> > > 
> > > which is what we achieve at the moment without the std::enable_if<>.
> > 
> > What about the following?
> > 
> > template <typename CppType, typename V8Type = void>
> > struct NativeValueTraits;
> > 
> > Then, V8Type = V8Type<T>::type makes sense and deduct a template parameter,
> also
> > {CppType=bool, V8Type=void} would make a kind of sense because bool is a
> > primitive type.
> > 
> > Anyway, if we needed a hack for some purpose, I'd like to be explict. 
Adding
> > "typename... Args" without a comment seems too implicit.
> 
> How about being even more explicit with something like this:
> 
> template <typename T, typename SFINAEHelper = void>  // SFINAEHelper can be
used
> by template specializations that need to introspect T.
> struct NativeValueTraits : public ... {
> };

Looks good.  Could you add a comment which code uses SFINAEHelper?

[Raphael Kubo da Costa (rakuco), 2017-03-06 11:23:07]

I've uploaded a new version of the patch incorporating Yuki's comments:
- V8BindingForIDLTypes.h is now NativeValueTraitsImpl.h
- MaybeWrapped and NeedsHeapVector have been moved into a class called
IDLTypesHelper (I've also added some documentation).
- v8.h is now included as "v8/include/v8.h" again.
- I've changed template typenames to SFINAEHelper in NativeValueTraits.h, and
documented how it is supposed to be used by NativeValueTraits specializations.
- I'm making use of it for the IDL interfaces specialization (I think it was
actually broken before).

[Raphael Kubo da Costa (rakuco), 2017-03-06 11:23:22]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 11:23:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuki, 2017-03-06 11:39:00]

LGTM!!

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:84: static inline
typename NativeValueTraitsBase<T>::ImplType
nit: inline keyword here doesn't make much sense. Harmless, though.

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: struct
NativeValueTraits<String> {
Just a question, do you have any plan to move these specializations out to
NativeValueTraitsImpl.h?

[Raphael Kubo da Costa (rakuco), 2017-03-06 11:43:49]

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: struct
NativeValueTraits<String> {
On 2017/03/06 11:39:00, Yuki(slow_til_mar08) wrote:
> Just a question, do you have any plan to move these specializations out to
> NativeValueTraitsImpl.h?

That could be done in the future, yes, but if possible I'd rather get rid of all
the specializations in this file (i.e. all callers would use NativeValueTraits
with IDL{Byte,DOM,USV}String). It's pretty doable once sequences are converted
to IDLSequence, but doing this requires some more work and goes back to the
question of getting rid of toImplArray and toMemberNativeArray in favor of
toImplSequence while not degrading performance.

[Yuki, 2017-03-06 11:45:56]

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: struct
NativeValueTraits<String> {
On 2017/03/06 11:43:49, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/06 11:39:00, Yuki(slow_til_mar08) wrote:
> > Just a question, do you have any plan to move these specializations out to
> > NativeValueTraitsImpl.h?
> 
> That could be done in the future, yes, but if possible I'd rather get rid of
all
> the specializations in this file (i.e. all callers would use NativeValueTraits
> with IDL{Byte,DOM,USV}String). It's pretty doable once sequences are converted
> to IDLSequence, but doing this requires some more work and goes back to the
> question of getting rid of toImplArray and toMemberNativeArray in favor of
> toImplSequence while not degrading performance.

Sounds a nice plan.  I see.

[Raphael Kubo da Costa (rakuco), 2017-03-06 11:53:04]

haraken@, I'd appreciate your l-g-t-m to cover
modules/serviceworkers/ForeignFetchRespondWithObserver.cpp

[haraken, 2017-03-06 12:56:33]

LGTM with comments. This is a good step forward. I'm really looking forward to
seeing you remove the existing ad-hoc type specialization code.

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:55: struct IDLTypesHelper
{

I'd prefer not introducing IDLTypesHelper though. It is essentially a syntax
sugar for WTF::IsGarbageCollectedType::value. Maybe can we improve the template
in WTF instead?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:60: struct MaybeWrapped {

MaybeWrapped => NeedsMember ?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:71: std::is_class<typename
V8TypeOf<T>::Type>::value;

What is std::is_class<typename V8TypeOf<T>::Type>::value doing?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLTypesBase.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypesBase.h:18: struct
IDLBaseHelper : public IDLBase {

Why do we need to distinguish IDLBaseHelper from IDLBase? Is it related to the
circular header inclusion you mentioned in the above?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:68: //   static
inline bool nativeValue(v8::Isolate* isolate,

Why are we returning bool while the type is an integer?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:83: //
nativeValue() method that takes the 3 arguments below.

Just to confirm: If someone forgets to specialize nativeValue() for some types,
she will hit a compile error, right?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:197: return
nativeValue<V8StringResourceMode::DefaultMode>(isolate, value,

Just to confirm: Using the DefaultMode doesn't change the existing behavior,
right?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:205:
V8StringResource<Mode> s(value);

s => string

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:269: return
nativeValue(isolate, value);

Is this used? If it's unused, we can add NOT_REACHED().

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:305: typename
V8TypeOf<T>::Type>::value>::type>

Add a comment about when enble_if ... returns true.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:315: return
V8TypeOf<T>::Type::toImplWithTypeCheck(isolate, value);

Just for the record, we should be careful not to use ::nativeValue in places
where we can just use toImpl (instead of toImplWithTypeCheck).

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: struct
NativeValueTraits<String> {
On 2017/03/06 11:45:56, Yuki(slow_til_mar08) wrote:
> On 2017/03/06 11:43:49, Raphael Kubo da Costa (rakuco) wrote:
> > On 2017/03/06 11:39:00, Yuki(slow_til_mar08) wrote:
> > > Just a question, do you have any plan to move these specializations out to
> > > NativeValueTraitsImpl.h?
> > 
> > That could be done in the future, yes, but if possible I'd rather get rid of
> all
> > the specializations in this file (i.e. all callers would use
NativeValueTraits
> > with IDL{Byte,DOM,USV}String). It's pretty doable once sequences are
converted
> > to IDLSequence, but doing this requires some more work and goes back to the
> > question of getting rid of toImplArray and toMemberNativeArray in favor of
> > toImplSequence while not degrading performance.
> 
> Sounds a nice plan.  I see.

Shall we add a TODO comment?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (left):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:477:
V8_VALUE_TO_CPP_VALUE = {

Are you planning to remove the remaining things in V8_VALUE_TO_CPP_VALUE? Add a
TODO comment?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:369:
'bindings/core/v8/NativeValueTraitsImpl.h'])

When should we include NativeValueTraits.h and when should we include
NativeValueTraitsImpl.h?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:574: trait_type =
idl_type.name

What types hit this branch?

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:78:
return {{cpp_class}}::create(ScriptState::current(isolate), value);

If this nativeValue() is expected to be not used, we can add NOT_REACHED().

[commit-bot: I haz the power, 2017-03-06 13:30:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 13:30:08]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-03-06 14:29:17]

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/IDLTypesBase.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypesBase.h:18: struct
IDLBaseHelper : public IDLBase {
On 2017/03/06 12:56:32, haraken wrote:
> 
> Why do we need to distinguish IDLBaseHelper from IDLBase? Is it related to the
> circular header inclusion you mentioned in the above?

Having IDLBase itself be a template class does not make much semantic sense:
IDLSequence and IDLRecord would need to inherit from IDLBase<T>, for some T that
is unrelated to them.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:83: //
nativeValue() method that takes the 3 arguments below.
On 2017/03/06 12:56:33, haraken wrote:
> 
> Just to confirm: If someone forgets to specialize nativeValue() for some
types,
> she will hit a compile error, right?

Correct. If there's no specialization for a given type T that's used by either
the bindings or hand-written code that invokes
NativeValueTraits<T>::nativeValue(), the build will fail. For example, if I
comment out the specialization for IDLBoolean I get errors such as this one:

../../third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h:85:3:
error: inline function 'blink::NativeValueTraits<blink::IDLBoolean,
void>::nativeValue' is not defined [-Werror,-Wundefined-inline]
  nativeValue(v8::Isolate*, v8::Local<v8::Value>, ExceptionState&);
  ^
gen/blink/bindings/core/v8/V8ProgressEventInit.cpp:48:60: note: used here
    bool lengthComputable = NativeValueTraits<IDLBoolean>::nativeValue(isolate,
lengthComputableValue, exceptionState);
                                                           ^

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:197: return
nativeValue<V8StringResourceMode::DefaultMode>(isolate, value,
On 2017/03/06 12:56:33, haraken wrote:
> 
> Just to confirm: Using the DefaultMode doesn't change the existing behavior,
> right?

Right, that's what the specialization for WTF::String in V8Binding.h also does.

However, note that DOMStrings (aka IDLString) have not been converted to
NativeValueTraits in v8_types.py for now because the Python code first needs to
be adapted to handle these different modes (it currently does so with
V8StringResource directly).

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:205:
V8StringResource<Mode> s(value);
On 2017/03/06 12:56:33, haraken wrote:
> 
> s => string

Done.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h:269: return
nativeValue(isolate, value);
On 2017/03/06 12:56:33, haraken wrote:
> 
> Is this used? If it's unused, we can add NOT_REACHED().
> 

This one is supposed to be used by callers which have no information about the
type they need to use NativeValueTraits with: in this case, they just know they
need to pass those 3 parameters. If callers know they are handling a Promise,
they can invoke the second version below directly and avoid having to create an
ExceptionState that's not used (which was Yuki's original concern with
converting everything to NativeValueTraits).

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:907: struct
NativeValueTraits<String> {
On 2017/03/06 12:56:33, haraken wrote:
> On 2017/03/06 11:45:56, Yuki(slow_til_mar08) wrote:
> > On 2017/03/06 11:43:49, Raphael Kubo da Costa (rakuco) wrote:
> > > On 2017/03/06 11:39:00, Yuki(slow_til_mar08) wrote:
> > > > Just a question, do you have any plan to move these specializations out
to
> > > > NativeValueTraitsImpl.h?
> > > 
> > > That could be done in the future, yes, but if possible I'd rather get rid
of
> > all
> > > the specializations in this file (i.e. all callers would use
> NativeValueTraits
> > > with IDL{Byte,DOM,USV}String). It's pretty doable once sequences are
> converted
> > > to IDLSequence, but doing this requires some more work and goes back to
the
> > > question of getting rid of toImplArray and toMemberNativeArray in favor of
> > > toImplSequence while not degrading performance.
> > 
> > Sounds a nice plan.  I see.
> 
> Shall we add a TODO comment?

Done.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (left):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:477:
V8_VALUE_TO_CPP_VALUE = {
On 2017/03/06 12:56:33, haraken wrote:
> 
> Are you planning to remove the remaining things in V8_VALUE_TO_CPP_VALUE?

That'd be the final plan, though I still need to figure out how to avoid
performance regressions.

> Add a TODO comment?

OK.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_types.py:574: trait_type =
idl_type.name
On 2017/03/06 12:56:33, haraken wrote:
> 
> What types hit this branch?

Interfaces, unions, dictionaries etc. Basically anything that's not a basic
WebIDL type.

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:78:
return {{cpp_class}}::create(ScriptState::current(isolate), value);
On 2017/03/06 12:56:33, haraken wrote:
> 
> If this nativeValue() is expected to be not used, we can add NOT_REACHED().

I don't think there's anything preventing callback functions from being used in
records, which is why this was added in the first place. If that's not the case,
it makes more sense to remove this NativeValueTraits specialization altogether.

[Raphael Kubo da Costa (rakuco), 2017-03-06 14:37:20]

haraken's question about when to use NativeValueTraits.h vs.
NativeValueTraitsImpl.h was a very good one, and it got me thinking.

The situation was a bit confusing up to patch v3. In general:
* NativeValueTraits.h: when specializing NativeValueTraits for a new type.
* NativeValueTraitsImpl.h: when having to use one of the specializations defined
there.

However, if one needed to use e.g. NativeValueTraits<Element>::nativeValue() in
their code, they need to include the generated V8Element.h as well as
NativeValueTraitsImpl.h for the NativeValueTraits overload for IDL interfaces to
be used. Additionally, NativeValueTraitsImpl.h also had specializations for
types unrelated to IDLBase (SerializedScriptValue).

I've changed some things in patch v4 in addition to adding some comments and
fixing some easy comments. Notably:
* SFINAEHelper is gone from the primary NativeValueTraits template, which
simplifies the code quite a bit. Each IDL interface now generates its
NativeValueTraits specialization. Going back to the example above, one would
only need to include V8Element.h in order to use
NativeValueTraits<Element>::nativeValue().
* The SerializedScriptValue specialization was moved to V8Binding.h itself, it
does not derive from IDLBase. I could also move it to SerializedScriptValue.h if
you think it's better.
* IDLTypesHelper was removed altogether in favor of just doing what it did in
IDLSequence. I also added some comments. The downside is that the code will
pretty much have to be duplicated for IDLRecord, so I'm still on the fence about
this.

Please take another look.

[haraken, 2017-03-06 19:10:29]

LGTM!

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

https://codereview.chromium.org/2730183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:908: struct
NativeValueTraits<SerializedScriptValue>

Yeah, I'd prefer moving this to SerializedScriptValue.h. All type specialization
in this file should be gone.

[Raphael Kubo da Costa (rakuco), 2017-03-06 19:40:48]

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

https://codereview.chromium.org/2730183003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Binding.h:908: struct
NativeValueTraits<SerializedScriptValue>
On 2017/03/06 19:10:29, haraken wrote:
> 
> Yeah, I'd prefer moving this to SerializedScriptValue.h. All type
specialization
> in this file should be gone.

Done in patch v6 \o/

[Raphael Kubo da Costa (rakuco), 2017-03-06 19:41:05]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 19:41:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-06 22:00:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:00:03]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-03-07 06:27:47]

LGTM.

[Raphael Kubo da Costa (rakuco), 2017-03-07 08:06:26]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-03-07 08:06:26]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2730183003/#ps100001
(title: "Move specialization for SerializedScriptValue to the appropriate
header")

[commit-bot: I haz the power, 2017-03-07 08:07:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 09:27:06]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488873986013920,
"parent_rev": "5aa9ade8c31edfb02e26319d8546378a5860f76f", "commit_rev":
"504550f038ae920fbc2bee9797f298189262e4fc"}

[commit-bot: I haz the power, 2017-03-07 09:28:12]

Description was changed from

==========
bindings: Add C++ versions of WebIDL types and generalize NativeValueTraits.

This being introduced primarily in order to support the record<K,V> WebIDL
type, as it needs to be able to convert arbitrary JS objects into their C++
counterparts without all the information available to the IDL code generator
at build-time.

The changes are two-fold:

1. IDLTypes.h contains C++ classes corresponding to the basic WebIDL
   types. At the moment they are limited to booleans, numbers, strings,
   promises and sequences. All classes inherit from IDLBase, and provide
   information about their corresponding C++ types (e.g. an IDLOctet
   corresponds to an uint8_t).

2. NativeValueTraits was expanded to cope with both the existing template
   specializations which essentially have the type one expects nativeValue()
   to return (e.g. AtomicString, double etc) as well as the new variations
   introduced for the IDL types above: when NativeValueTraits is specialized
   for an IDLBase-based type, all type information is derived from it.

The idea behind 1) and 2) is that NativeValueTraits<T>::nativeValue() should
be a standard interface for converting any JS value to C++ by interpreting
the JS value as a T.

Wherever possible, v8_types.py now generates invocations to
NativeValueTraits<T>::nativeValue() instead of direct calls to type-specific
conversion functions. It was not done when the conversion functions
themselves take arguments that do not match nativeValue()'s signature. For
example, converting to a Promise or an IDL interface does not require an
ExceptionState, so using nativeValue() would introduce unnecessary overhead
by requiring an ExceptionState to be created and passed.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Add C++ versions of WebIDL types and generalize NativeValueTraits.

This being introduced primarily in order to support the record<K,V> WebIDL
type, as it needs to be able to convert arbitrary JS objects into their C++
counterparts without all the information available to the IDL code generator
at build-time.

The changes are two-fold:

1. IDLTypes.h contains C++ classes corresponding to the basic WebIDL
   types. At the moment they are limited to booleans, numbers, strings,
   promises and sequences. All classes inherit from IDLBase, and provide
   information about their corresponding C++ types (e.g. an IDLOctet
   corresponds to an uint8_t).

2. NativeValueTraits was expanded to cope with both the existing template
   specializations which essentially have the type one expects nativeValue()
   to return (e.g. AtomicString, double etc) as well as the new variations
   introduced for the IDL types above: when NativeValueTraits is specialized
   for an IDLBase-based type, all type information is derived from it.

The idea behind 1) and 2) is that NativeValueTraits<T>::nativeValue() should
be a standard interface for converting any JS value to C++ by interpreting
the JS value as a T.

Wherever possible, v8_types.py now generates invocations to
NativeValueTraits<T>::nativeValue() instead of direct calls to type-specific
conversion functions. It was not done when the conversion functions
themselves take arguments that do not match nativeValue()'s signature. For
example, converting to a Promise or an IDL interface does not require an
ExceptionState, so using nativeValue() would introduce unnecessary overhead
by requiring an ExceptionState to be created and passed.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2730183003
Cr-Commit-Position: refs/heads/master@{#455045}
Committed:
https://chromium.googlesource.com/chromium/src/+/504550f038ae920fbc2bee9797f2...
==========

[commit-bot: I haz the power, 2017-03-07 09:28:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/504550f038ae920fbc2bee9797f2...

[Yuki, 2017-03-08 10:14:22]

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

https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:60: struct MaybeWrapped {
On 2017/03/06 12:56:32, haraken wrote:
> 
> MaybeWrapped => NeedsMember ?

rakuco, you seems overlooking this comment.

This comment somewhat makes sense because we often use the term "wrap" for V8
wrapper object (for a Blink DOM object).  It's a bit confusing.

NeedsMember looks like that it returns a boolean value, so we may like another
name.

What about MaybeMember for example?

I'm okay to change the name in https://crrev.com/2732093003

[Raphael Kubo da Costa (rakuco), 2017-03-08 10:18:56]

On 2017/03/08 10:14:22, Yuki(slow_til_mar08) wrote:
>
https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/IDLTypes.h (right):
> 
>
https://codereview.chromium.org/2730183003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/IDLTypes.h:60: struct MaybeWrapped
{
> On 2017/03/06 12:56:32, haraken wrote:
> > 
> > MaybeWrapped => NeedsMember ?
> 
> rakuco, you seems overlooking this comment.
> 
> This comment somewhat makes sense because we often use the term "wrap" for V8
> wrapper object (for a Blink DOM object).  It's a bit confusing.
> 
> NeedsMember looks like that it returns a boolean value, so we may like another
> name.
> 
> What about MaybeMember for example?
> 
> I'm okay to change the name in https://crrev.com/2732093003

Oops, indeed I forgot about this one. I'll update the patch in
https://crrev.com/2732093003 once I manage to find out what's breaking the
Windows component builds in that CL.