[bindings] Introduce ScriptValue.to<T> to convert to native values.

[bindings] Introduce ScriptValue.to<T> to convert to native values.

The IndexedDB utilizes couple of methods to translate ScriptValue to their respective native counterparts.
This warrants addition of ScriptValue.to<T> template method to do the job using the NativeValueTraits<T>.

R=haraken@chromium.org, jsbell@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191901

[vivekg, 2015-03-13 16:04:21]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-03-13 16:04:21]

PTAL, thanks.

[jsbell, 2015-03-13 16:33:26]

lgtm

script_value_cast<>() has isolate as a later parameter, in contrast to
https://code.google.com/p/chromium/issues/detail?id=424446 - but I'll let the
bindings team argue about consistency. :)

[vivekg, 2015-03-13 17:58:20]

On 2015/03/13 16:33:26, jsbell wrote:
> lgtm
> 
> script_value_cast<>() has isolate as a later parameter, in contrast to
> https://code.google.com/p/chromium/issues/detail?id=424446 - but I'll let the
> bindings team argue about consistency. :)

Thanks jsbell@, in order to maintain the same arguments ordering as of
NativeValueTraits<T>::nativeValue(), I have made the script_value_cast accept
the isolate as later arg.

[vivekg, 2015-03-13 18:18:21]

vivekg@chromium.org changed reviewers:
+ jl@opera.com

[vivekg, 2015-03-13 18:19:21]

On 2015/03/13 18:18:21, vivekg_ wrote:
> mailto:vivekg@chromium.org changed reviewers:
> + mailto:jl@opera.com

haraken@, Jens: any suggestions?

[haraken, 2015-03-14 01:27:19]

https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValue.h:61: return script_value_cast<T>(v8Value(),
isolate(), exceptionState);

I'm wondering why we cannot simply call:

  NativeValueTraits<T>::nativeValue(v8Value(), isolate, exceptionState);

https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/V8B...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/V8B...
Source/bindings/core/v8/V8Binding.h:808: T script_value_cast(const
v8::Local<v8::Value>& value, v8::Isolate* isolate, ExceptionState&
exceptionState)

Why does this need to be defined in V8Binding.h? Can we put this in
ScriptValue.h?

[vivekg, 2015-03-14 01:49:27]

On 2015/03/14 01:27:19, haraken wrote:
>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
> File Source/bindings/core/v8/ScriptValue.h (right):
> 
>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
> Source/bindings/core/v8/ScriptValue.h:61: return
script_value_cast<T>(v8Value(),
> isolate(), exceptionState);
> 
> I'm wondering why we cannot simply call:
> 
>   NativeValueTraits<T>::nativeValue(v8Value(), isolate, exceptionState);
>

The ScriptValue is a leaf include file and being included from v8binding.h

Also the NativeValueTraits <> needs v8binding.h to be included to work.
Including v8binding.h in scriptvalue.h firstly violates the inclusion rules and
secondly  throws tons of XXX not a type error. Hence I created this little
redirection hee.

>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/V8B...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/V8B...
> Source/bindings/core/v8/V8Binding.h:808: T script_value_cast(const
> v8::Local<v8::Value>& value, v8::Isolate* isolate, ExceptionState&
> exceptionState)
> 
> Why does this need to be defined in V8Binding.h? Can we put this in
> ScriptValue.h?

[haraken, 2015-03-14 10:37:01]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, yukishiino@chromium.org

[haraken, 2015-03-14 10:37:01]

On 2015/03/14 01:49:27, vivekg_ wrote:
> On 2015/03/14 01:27:19, haraken wrote:
> >
>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
> > File Source/bindings/core/v8/ScriptValue.h (right):
> > 
> >
>
https://codereview.chromium.org/1009503003/diff/1/Source/bindings/core/v8/Scr...
> > Source/bindings/core/v8/ScriptValue.h:61: return
> script_value_cast<T>(v8Value(),
> > isolate(), exceptionState);
> > 
> > I'm wondering why we cannot simply call:
> > 
> >   NativeValueTraits<T>::nativeValue(v8Value(), isolate, exceptionState);
> >
> 
> The ScriptValue is a leaf include file and being included from v8binding.h
> 
> Also the NativeValueTraits <> needs v8binding.h to be included to work.
> Including v8binding.h in scriptvalue.h firstly violates the inclusion rules
and
> secondly  throws tons of XXX not a type error. Hence I created this little
> redirection hee.

Thanks, understood.

bashi-san or shiino-san: Do you have any better idea on this?

[vivekg, 2015-03-16 02:31:03]

bashi@, any suggestions?

[bashi, 2015-03-16 03:19:57]

On 2015/03/16 02:31:03, vivekg_ wrote:
> bashi@, any suggestions?

Could putting un-specialized NativeValueTraits in a separate file (e.g.
NativeValueTraits.h) work? DictionaryHelperTraits uses similar pattern.

[Yuki, 2015-03-16 04:38:00]

On 2015/03/16 03:19:57, bashi1 wrote:
> On 2015/03/16 02:31:03, vivekg_ wrote:
> > bashi@, any suggestions?
> 
> Could putting un-specialized NativeValueTraits in a separate file (e.g.
> NativeValueTraits.h) work? DictionaryHelperTraits uses similar pattern.

I'm thinking a following design/implementation, which looks like toV8() in
ToV8.h.

Re-implement NativeValueTraits<T>::nativeValue() as toNative<T>.  We don't need
a trait class for this purpose.  Simple global template function toNative<T>
does the trick, as same as global overloaded toV8() does.  toNative is reverse
version of toV8.

V8Binding.h:
template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate* isolate,
ExceptionState& exceptionState);
// declaration only, no implementation

ToNative.h:
template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate* isolate,
ExceptionState& exceptionState);
// actural implementation of primitive types
// toNative<String>, toNative<int>, etc.

V8Xxx.h:
template<V8Xxx> toNative(...);
// Provides specialized version of type Xxx

ScriptValue.h:
template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate* isolate,
ExceptionState& exceptionState);
// declaration only as a forward declaration

If we want, we can put the declaration in Foo.h, and both of V8Binding.h and
ScriptValue.h can include it, as bashi@ suggested.

[bashi, 2015-03-16 04:51:57]

On 2015/03/16 04:38:00, Yuki wrote:
> On 2015/03/16 03:19:57, bashi1 wrote:
> > On 2015/03/16 02:31:03, vivekg_ wrote:
> > > bashi@, any suggestions?
> > 
> > Could putting un-specialized NativeValueTraits in a separate file (e.g.
> > NativeValueTraits.h) work? DictionaryHelperTraits uses similar pattern.
> 
> I'm thinking a following design/implementation, which looks like toV8() in
> ToV8.h.
> 
> Re-implement NativeValueTraits<T>::nativeValue() as toNative<T>.  We don't
need
> a trait class for this purpose.  Simple global template function toNative<T>
> does the trick, as same as global overloaded toV8() does.  toNative is reverse
> version of toV8.
> 
> V8Binding.h:
> template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate*
isolate,
> ExceptionState& exceptionState);
> // declaration only, no implementation
> 
> ToNative.h:
> template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate*
isolate,
> ExceptionState& exceptionState);
> // actural implementation of primitive types
> // toNative<String>, toNative<int>, etc.
> 
> V8Xxx.h:
> template<V8Xxx> toNative(...);
> // Provides specialized version of type Xxx
> 
> ScriptValue.h:
> template<typename T> toNative(v8::Local<v8::Value> value, v8::Isolate*
isolate,
> ExceptionState& exceptionState);
> // declaration only as a forward declaration
> 
> If we want, we can put the declaration in Foo.h, and both of V8Binding.h and
> ScriptValue.h can include it, as bashi@ suggested.

I generally support this idea. One thing we should keep in mind is that some JS
-> C++ conversions don't have 1:1 relationship. For example, we need to take
into account of [EnforceRange] and [Clamp] when we convert long.
http://heycam.github.io/webidl/#es-long

[vivekg, 2015-03-16 05:06:53]

Thank you yuki@ and bashi@. I have also one idea listed as below. Please let me
know your thoughts.

NativeValueTraits.h
===================
template <typename T> struct NativeValueTraits;

template <typename T>
struct NativeValueTraitsBase {
    virtual T operator()(const v8::Local<v8::Value>&, v8::Isolate*,
ExceptionState&) = 0;
};


ScriptValue.h
=============
class CORE_EXPORT ScriptValue final {
    ...
    template <typename T>
    T to(ExceptionState& exceptionState) const
    {
        // We use the functor object here.
        return NativeValueTraits<T>()(v8Value(), isolate(), exceptionState);
    }
    ...
}


V8BindingForModules.h
=====================
#include "bindings/core/v8/NativeValueTraits.h"

template <>
struct NativeValueTraits<IDBKey*> : public NativeValueTraitsBase<IDBKey*> {
    IDBKey* operator()(const v8::Local<v8::Value>&, v8::Isolate*,
ExceptionState&) override 
    {
        return ...;
    }
};

The advantages of using the functor object is that it enforces the derived
classes to use the same signature (here the operator()(...)) apart from being
more c++ish :)

[Yuki, 2015-03-16 05:21:32]

On 2015/03/16 05:06:53, vivekg_ wrote:
> Thank you yuki@ and bashi@. I have also one idea listed as below. Please let
me
> know your thoughts.
> 
> NativeValueTraits.h
> ===================
> template <typename T> struct NativeValueTraits;
> 
> template <typename T>
> struct NativeValueTraitsBase {
>     virtual T operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> ExceptionState&) = 0;
> };
> 
> 
> ScriptValue.h
> =============
> class CORE_EXPORT ScriptValue final {
>     ...
>     template <typename T>
>     T to(ExceptionState& exceptionState) const
>     {
>         // We use the functor object here.
>         return NativeValueTraits<T>()(v8Value(), isolate(), exceptionState);
>     }
>     ...
> }
> 
> 
> V8BindingForModules.h
> =====================
> #include "bindings/core/v8/NativeValueTraits.h"
> 
> template <>
> struct NativeValueTraits<IDBKey*> : public NativeValueTraitsBase<IDBKey*> {
>     IDBKey* operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> ExceptionState&) override 
>     {
>         return ...;
>     }
> };
> 
> The advantages of using the functor object is that it enforces the derived
> classes to use the same signature (here the operator()(...)) apart from being
> more c++ish :)

Looks beautiful, though, ...
We generally avoid using "virtual" because of its overhead.  If it's not
performance-sensitive, we'd be okay to use virtual, otherwise, it'd be better
not to use virtual.

[vivekg, 2015-03-16 05:27:43]

On 2015/03/16 at 05:21:32, yukishiino wrote:
> On 2015/03/16 05:06:53, vivekg_ wrote:
> > Thank you yuki@ and bashi@. I have also one idea listed as below. Please let
me
> > know your thoughts.
> > 
> > NativeValueTraits.h
> > ===================
> > template <typename T> struct NativeValueTraits;
> > 
> > template <typename T>
> > struct NativeValueTraitsBase {
> >     virtual T operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> > ExceptionState&) = 0;
> > };
> > 
> > 
> > ScriptValue.h
> > =============
> > class CORE_EXPORT ScriptValue final {
> >     ...
> >     template <typename T>
> >     T to(ExceptionState& exceptionState) const
> >     {
> >         // We use the functor object here.
> >         return NativeValueTraits<T>()(v8Value(), isolate(), exceptionState);
> >     }
> >     ...
> > }
> > 
> > 
> > V8BindingForModules.h
> > =====================
> > #include "bindings/core/v8/NativeValueTraits.h"
> > 
> > template <>
> > struct NativeValueTraits<IDBKey*> : public NativeValueTraitsBase<IDBKey*> {
> >     IDBKey* operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> > ExceptionState&) override 
> >     {
> >         return ...;
> >     }
> > };
> > 
> > The advantages of using the functor object is that it enforces the derived
> > classes to use the same signature (here the operator()(...)) apart from
being
> > more c++ish :)
> 
> Looks beautiful, though, ...
> We generally avoid using "virtual" because of its overhead.  If it's not
performance-sensitive, we'd be okay to use virtual, otherwise, it'd be better
not to use virtual.

Sure, makes perfect sense not to use 'virtual' here as this is performance
sensitive area of binding. I will change the code accordingly and submit for
your reviews.
Thank you.

[vivekg, 2015-03-16 05:27:43]

On 2015/03/16 at 05:21:32, yukishiino wrote:
> On 2015/03/16 05:06:53, vivekg_ wrote:
> > Thank you yuki@ and bashi@. I have also one idea listed as below. Please let
me
> > know your thoughts.
> > 
> > NativeValueTraits.h
> > ===================
> > template <typename T> struct NativeValueTraits;
> > 
> > template <typename T>
> > struct NativeValueTraitsBase {
> >     virtual T operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> > ExceptionState&) = 0;
> > };
> > 
> > 
> > ScriptValue.h
> > =============
> > class CORE_EXPORT ScriptValue final {
> >     ...
> >     template <typename T>
> >     T to(ExceptionState& exceptionState) const
> >     {
> >         // We use the functor object here.
> >         return NativeValueTraits<T>()(v8Value(), isolate(), exceptionState);
> >     }
> >     ...
> > }
> > 
> > 
> > V8BindingForModules.h
> > =====================
> > #include "bindings/core/v8/NativeValueTraits.h"
> > 
> > template <>
> > struct NativeValueTraits<IDBKey*> : public NativeValueTraitsBase<IDBKey*> {
> >     IDBKey* operator()(const v8::Local<v8::Value>&, v8::Isolate*,
> > ExceptionState&) override 
> >     {
> >         return ...;
> >     }
> > };
> > 
> > The advantages of using the functor object is that it enforces the derived
> > classes to use the same signature (here the operator()(...)) apart from
being
> > more c++ish :)
> 
> Looks beautiful, though, ...
> We generally avoid using "virtual" because of its overhead.  If it's not
performance-sensitive, we'd be okay to use virtual, otherwise, it'd be better
not to use virtual.

Sure, makes perfect sense not to use 'virtual' here as this is performance
sensitive area of binding. I will change the code accordingly and submit for
your reviews.
Thank you.

[vivekg, 2015-03-16 06:10:40]

yuki@, there is one problem using the toNative<T> template version with using
Vector<T> i.e.

template <typename T>
Vector<T> toNative<Vector<T>>(const v8::Local<v8::Value>& value, v8::Isolate*
isolate, ExceptionState& exceptionState)
{
    return toImplArray<T>(value, 0, isolate, exceptionState);
}

Throws the below error:

error: function template partial specialization
‘toNative<WTF::Vector<CharacterType> >’ is not allowed

Got some literature explaining why its not allowed [1]

[1] http://www.gotw.ca/publications/mill17.htm

How do we resolve or get around this error?

[Yuki, 2015-03-16 06:31:38]

On 2015/03/16 06:10:40, vivekg_ wrote:
> yuki@, there is one problem using the toNative<T> template version with using
> Vector<T> i.e.
> 
> template <typename T>
> Vector<T> toNative<Vector<T>>(const v8::Local<v8::Value>& value, v8::Isolate*
> isolate, ExceptionState& exceptionState)
> {
>     return toImplArray<T>(value, 0, isolate, exceptionState);
> }
> 
> Throws the below error:
> 
> error: function template partial specialization
> ‘toNative<WTF::Vector<CharacterType> >’ is not allowed
> 
> Got some literature explaining why its not allowed [1]
> 
> [1] http://www.gotw.ca/publications/mill17.htm
> 
> How do we resolve or get around this error?

Oops.  Then, it seems better to keep using a trait class.

[vivekg, 2015-03-16 07:40:59]

Done, PTAL.

[Yuki, 2015-03-16 08:03:07]

lgtm

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NativeValueTraits.h:8: namespace v8 {
You'd better include <v8.h>, which is a third party library from our point of
view.

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NativeValueTraits.h:21: static T nativeValue(const
v8::Local<v8::Value>&, v8::Isolate*, ExceptionState&);
I'm not quite sure why this has been const v8::Local&.

I think v8::Local is defined as being able to pass it as a value, and we pass it
as a value everywhere else.

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:817: struct NativeValueTraits<String> {
Could you add a FIXME comment so that we can move these specializations out to
NativeValueTraitsImpl.h or something in future?  (or, +1 if you can do it in a
follow-up CL.)

Since we have NativeValueTraits.h, it's strange that V8Binding.h provides its
implementations.

[vivekg, 2015-03-16 08:32:17]

Thank you. Done the v8.h inclusion changes. For the other comments, I will do
the follow up patches.

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/NativeValueTraits.h (right):

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NativeValueTraits.h:8: namespace v8 {
On 2015/03/16 at 08:03:07, Yuki wrote:
> You'd better include <v8.h>, which is a third party library from our point of
view.

Done.

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/NativeValueTraits.h:21: static T nativeValue(const
v8::Local<v8::Value>&, v8::Isolate*, ExceptionState&);
On 2015/03/16 at 08:03:07, Yuki wrote:
> I'm not quite sure why this has been const v8::Local&.
> 
> I think v8::Local is defined as being able to pass it as a value, and we pass
it as a value everywhere else.

I will take this up as follow up patch.

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/1009503003/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:817: struct NativeValueTraits<String> {
On 2015/03/16 at 08:03:07, Yuki wrote:
> Could you add a FIXME comment so that we can move these specializations out to
NativeValueTraitsImpl.h or something in future?  (or, +1 if you can do it in a
follow-up CL.)
> 
> Since we have NativeValueTraits.h, it's strange that V8Binding.h provides its
implementations.

Yes, I will put up the followup patch.

[vivekg, 2015-03-16 08:47:33]

The CQ bit was checked by vivekg@chromium.org

[vivekg, 2015-03-16 08:47:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/1009503003/#ps40001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-03-16 08:47:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1009503003/40001

[commit-bot: I haz the power, 2015-03-16 10:12:58]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191901