WIP bindings: Expand usage of NativeValueTraits.

WIP bindings: Expand usage of NativeValueTraits.

Make it a uniform wrapper that can call all sorts of different functions.

BUG=685754

[Raphael Kubo da Costa (rakuco), 2017-02-28 19:37:01]

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

[Raphael Kubo da Costa (rakuco), 2017-02-28 19:37:02]

Bashi-san et al: please take a look. This is the experiment I've been doing to
expand the usage of NativeValueTraits<> to convert V8 types to C++.

The idea is to standardize on nativeValue(isolate, v8Value, exceptionState) as
the method call, even if the conversion function being called does not need an
ExceptionState.

And the big shift is that we move from NativeValueTraits<final C++ type I want>
to NativeValueTraits<IDL or C++ type I want to convert>, and we decide the best
output type.

I haven't updated the bindings expectations and the code still looks very messy,
but the layout tests in external/wpt, fast/js and bindings were all working.

Let me know if this is indeed the direction we should we move to.

[haraken, 2017-03-01 01:34:04]

I think this is a right way forward.

On 2017/02/28 19:37:02, Raphael Kubo da Costa (rakuco) wrote:
> Bashi-san et al: please take a look. This is the experiment I've been doing to
> expand the usage of NativeValueTraits<> to convert V8 types to C++.
> 
> The idea is to standardize on nativeValue(isolate, v8Value, exceptionState) as
> the method call, even if the conversion function being called does not need an
> ExceptionState.
> 
> And the big shift is that we move from NativeValueTraits<final C++ type I
want>
> to NativeValueTraits<IDL or C++ type I want to convert>, and we decide the
best
> output type.

I'm just curious but why do we need NativeValueTraits<C++ type I want to
convert>? Wouldn't it be enough to support NativeValueTraits<IDL type I want to
convert>?

> 
> I haven't updated the bindings expectations and the code still looks very
messy,
> but the layout tests in external/wpt, fast/js and bindings were all working.
> 
> Let me know if this is indeed the direction we should we move to.

[Yuki, 2017-03-01 06:25:43]

I have a concern that this way would regress the binding performance.
I like the code cleanness best, but the binding layer is always
under the pressure to improve the performance.

This way, we always instantiate an ExceptionState even when we
don't need it.  Then, we need to check ExceptionState.hadException()
(although this CL doesn't check it).  We need to pay extra cost.

Note that we're currently assuming that
  Type* impl = V8Type::toImpl(info.Holder());
never fail, and we're not checking if impl == nullptr.

This CL is going to make the above code be like below.
  // extra string resource: May increase the binary size, too.
  ExceptionState exceptionState("interface name", "attr/op name", ...);
  Type* impl = nativeValue(isolate, info.Holder(), exceptionState);
  if (exceptionState.hadException()) {
    return;
  }

I like the basic idea and the effort to make the codebase cleaner,
but we may not want to introduce extra cost to create an
ExceptionState, or checking the result, etc.

Since the code generator knew whether the type is an interface type
or primitive type, etc., we may want to take its advantage rather
than unifying/abstracting types.

IMHO, the generated code should be optimized with exact/actual
types, but the code generator, etc. can be much smarter/cleaner.

[Raphael Kubo da Costa (rakuco), 2017-03-01 07:32:38]

On 2017/03/01 01:34:04, haraken wrote:
> I'm just curious but why do we need NativeValueTraits<C++ type I want to
> convert>? Wouldn't it be enough to support NativeValueTraits<IDL type I want
to
> convert>?

Sorry, I had interfaces and dictionaries in mind when I wrote "C++ type", so
you're right, this is about IDL types.

On 2017/03/01 06:25:43, Yuki(ooo_til_feb28) wrote:
> I like the basic idea and the effort to make the codebase cleaner,
> but we may not want to introduce extra cost to create an
> ExceptionState, or checking the result, etc.
> 
> Since the code generator knew whether the type is an interface type
> or primitive type, etc., we may want to take its advantage rather
> than unifying/abstracting types.
> 
> IMHO, the generated code should be optimized with exact/actual
> types, but the code generator, etc. can be much smarter/cleaner.

We need some standardization in the API, otherwise converting a V8 object into a
Vector<std::pair<K, V>> (which is what prompted this change in the first place)
won't work. The V8->Record code we started discussing in
https://codereview.chromium.org/2709983004/ is supposed to be entirely static in
V8Binding.h or something, yet it needs to convert V8 objects into arbitrary C++
types without knowing anything about them. How should this be dealt if we decide
to leave the code generator "smarter" than regular C++ code?

[Yuki, 2017-03-01 07:56:10]

On 2017/03/01 07:32:38, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/01 01:34:04, haraken wrote:
> > I'm just curious but why do we need NativeValueTraits<C++ type I want to
> > convert>? Wouldn't it be enough to support NativeValueTraits<IDL type I want
> to
> > convert>?
> 
> Sorry, I had interfaces and dictionaries in mind when I wrote "C++ type", so
> you're right, this is about IDL types.
> 
> On 2017/03/01 06:25:43, Yuki(ooo_til_feb28) wrote:
> > I like the basic idea and the effort to make the codebase cleaner,
> > but we may not want to introduce extra cost to create an
> > ExceptionState, or checking the result, etc.
> > 
> > Since the code generator knew whether the type is an interface type
> > or primitive type, etc., we may want to take its advantage rather
> > than unifying/abstracting types.
> > 
> > IMHO, the generated code should be optimized with exact/actual
> > types, but the code generator, etc. can be much smarter/cleaner.
> 
> We need some standardization in the API, otherwise converting a V8 object into
a
> Vector<std::pair<K, V>> (which is what prompted this change in the first
place)
> won't work. The V8->Record code we started discussing in
> https://codereview.chromium.org/2709983004/ is supposed to be entirely static
in
> V8Binding.h or something, yet it needs to convert V8 objects into arbitrary
C++
> types without knowing anything about them. How should this be dealt if we
decide
> to leave the code generator "smarter" than regular C++ code?

Ah, I see.  Then, we may want to generalize it only for specific
use cases such as Vector<std::pair<K, V>>.  We do not need to use
the general version everywhere we can optimize.

The code generator should know it's a sequence, maplike, or an
usual interface type.  So, we can use the general version only
for specific cases.

I have something like below in my mind.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
In cases that we don't know an exact type, we'd like to use
toNativeValue<V>(holder).

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

On 2017/03/01 07:56:10, Yuki(ooo_til_feb28) wrote:
> Ah, I see.  Then, we may want to generalize it only for specific
> use cases such as Vector<std::pair<K, V>>.  We do not need to use
> the general version everywhere we can optimize.
> 
> The code generator should know it's a sequence, maplike, or an
> usual interface type.  So, we can use the general version only
> for specific cases.
> 
> I have something like below in my mind.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
> In cases that we don't know an exact type, we'd like to use
> toNativeValue<V>(holder).

Let me see if I understood you correctly: instead of trimming down v8_types.py,
your idea is to leave it as unchanged as possible, keep the generic
NativeValueTraits overloads added in this patch and only use them for the
upcoming toImplRecord() code then?

[Yuki, 2017-03-01 08:15:00]

On 2017/03/01 08:02:49, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/03/01 07:56:10, Yuki(ooo_til_feb28) wrote:
> > Ah, I see.  Then, we may want to generalize it only for specific
> > use cases such as Vector<std::pair<K, V>>.  We do not need to use
> > the general version everywhere we can optimize.
> > 
> > The code generator should know it's a sequence, maplike, or an
> > usual interface type.  So, we can use the general version only
> > for specific cases.
> > 
> > I have something like below in my mind.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/scrip...
> > In cases that we don't know an exact type, we'd like to use
> > toNativeValue<V>(holder).
> 
> Let me see if I understood you correctly: instead of trimming down
v8_types.py,
> your idea is to leave it as unchanged as possible, keep the generic
> NativeValueTraits overloads added in this patch and only use them for the
> upcoming toImplRecord() code then?

Yes.  I'd propose to add your generic support on NativeValueTraits, and
use it for toImplRecord() first.  We might want to use it more widely
later.  But let's use it for limited places first.  That's my opinion.

What do you guys think?

[Raphael Kubo da Costa (rakuco), 2017-03-01 09:48:42]

On 2017/03/01 08:15:00, Yuki(ooo_til_feb28) wrote:
> Yes.  I'd propose to add your generic support on NativeValueTraits, and
> use it for toImplRecord() first.  We might want to use it more widely
> later.  But let's use it for limited places first.  That's my opinion.
> 
> What do you guys think?

No objections from my side, though I need to think a bit of how to choose e.g.
idl::Octet instead of unsigned short in a conversion.

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

Since following your proposal involves reducing usage of these new overloads to
records, I've uploaded a new patch to
https://codereview.chromium.org/2709983004/ so we can see it in action.