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

third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h

Index: third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h
index 3bc77e548afbe3d997b21bdb675e06fdf1f05fac..f627788a2cfcea83c2492f221cd7be1aba00ff31 100644
--- a/third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h
+++ b/third_party/WebKit/Source/bindings/core/v8/NativeValueTraits.h
@@ -5,20 +5,65 @@
 #ifndef NativeValueTraits_h
 #define NativeValueTraits_h
 
-#include "v8/include/v8.h"
+#include <v8.h>

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

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".

+#include <type_traits>
+#include "bindings/core/v8/IDLTypesBase.h"
 #include "wtf/Allocator.h"
 
 namespace blink {
 
 class ExceptionState;
 
-template <typename T, typename... Arguments>
-struct NativeValueTraits {
-  STATIC_ONLY(NativeValueTraits);
-  static T nativeValue(v8::Isolate*,
-                       v8::Local<v8::Value>,
-                       ExceptionState&,
-                       Arguments... args);
+// NativeValueTraitsBase is supposed to be inherited by NativeValueTraits
+// classes. They serve as a way to hold the ImplType typedef without requiring
+// all NativeValueTraits specializations to declare it.
+//
+// The primary template below is used by NativeValueTraits specializations with
+// types that do not inherit from IDLBase, in which case it is assumed the type
+// of the specialization is also |ImplType|. The NativeValueTraitsBase
+// specialization is used for IDLBase-based types, which are supposed to have
+// their own |ImplType| typedefs.
+template <typename T, typename U = void>

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

Why do we need |U| here?

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

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 {
       ^

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

I see.

+struct NativeValueTraitsBase {
+  using ImplType = T;
+  STATIC_ONLY(NativeValueTraitsBase);
+};
+
+template <typename T>
+struct NativeValueTraitsBase<
+    T,
+    typename std::enable_if<std::is_base_of<IDLBase, T>::value>::type> {
+  using ImplType = typename T::ImplType;
+  STATIC_ONLY(NativeValueTraitsBase);
+};
+
+// Primary template for NativeValueTraits. It is not supposed to be used
+// directly: there needs to be a specialization for each type which represents
+// a JavaScript type that will be converted to a C++ representation.
+// Its main goal is to provide a standard interface for converting JS types
+// into C++ ones.
+//
+// Example:
+// template <>
+// struct NativeValueTraits<IDLLong> : public NativeValueTraitsBase<IDLLong> {
+//   static inline int32_t nativeValue(v8::Isolate* isolate,
+//                                     v8::Local<v8::Value> value,
+//                                     ExceptionState& exceptionState) {
+//     return toInt32(isolate, value, exceptionState, NormalConversion);
+//   }
+// }
+//
+// Note that there exist some specializations (particularly in V8Binding.h) for
+// which T actually represents the final C++ type that a JavaScript value
+// should be converted to. Introducing new specializations of this kind is
+// discouraged.
+template <typename T, typename... Args>

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

I think that we don't need |Args| here.
We can add IntegerConversionConfiguration without Args here.

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

|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]

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]

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]

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]

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]

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]

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]

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

+struct NativeValueTraits : public NativeValueTraitsBase<T> {
+  // This declaration serves only as a blueprint for specializations: the
+  // return type can change, but all specializations are expected to provide a
+  // nativeValue() method that takes the 3 arguments below.
+  static inline typename NativeValueTraitsBase<T>::ImplType
+  nativeValue(v8::Isolate*, v8::Local<v8::Value>, ExceptionState&);
 };
 
 }  // namespace blink