Make vector conversion more general in V8Bindings

Make vector conversion more general in V8Bindings

toV8Value() could not convert vectors of basic types; by removing toV8ArrayNoInline() and always calling v8Array(), we instead pass back through
V8ValueTraits<> for each element, and as a result can convert arrays of basic types through the explicit specializations of that class.

R=kentaro@chromium.org,yhirano@chromium.org
BUG=None

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

[gavinp, 2014-07-20 04:15:18]

Hara-san, Hirano-san, can you each please take a look at this patch? This is my
first work in bindings, so it would be helpful if you can be especially
detailed.

This issue came up for me while implementing ServiceWorker APIs with promises;
the ScriptPromiseResolver::resolve() method eventually calls
V8ValueTraits<>::toV8Value(), and I could not convert an array of strings.

While I was here, I added all the types that were reasonable and easy, for
instance the numeric types. It's arguably extra bloat, but I also do not want
another future engineer to suffer the confusion that this limitation caused me.

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

https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:241: v8::Handle<v8::Value>
v8ArrayNoInline(const Vector<T, inlineCapacity>& iterator,
v8::Handle<v8::Object> creationContext, v8::Isolate*);
These two forward declarations are required to make type resolution work for the
basic types like WTFString; I think because they are not in an associated
namespace, but I'm not certain.

I note that these two calls are the only uses of v8ArrayNoInline, so another
option is to move the definition of the two v8ArrayNoInline<>() up above their
corresponding V8ValueTraits<>::toV8Value(). I didn't do that initially and
instead added these forward declarations because... this file looks very
carefully ordered and I didn't want to upset the boat.

[haraken, 2014-07-20 04:50:46]

+tasak

Looks good but have one question.

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

https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:263: // should also have an explicit
specialization of toV8NoInline<>() in V8Binding.cpp, so that vectors of the same
type can also be converted.

Just help me understand: Why do we need to have explicit specialization of
toV8NoInline<>() whereas we don't need to have explicit specialization of
toV8<>()?

[gavinp, 2014-07-20 05:11:16]

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

https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:263: // should also have an explicit
specialization of toV8NoInline<>() in V8Binding.cpp, so that vectors of the same
type can also be converted.
On 2014/07/20 04:50:46, haraken wrote:
> 
> Just help me understand: Why do we need to have explicit specialization of
> toV8NoInline<>() whereas we don't need to have explicit specialization of
> toV8<>()?

I do not know; I am very new in this coding area.

It's very interesting that IDL can bind function return values of Vector<String>
correctly (this occurs a number of times), but that they didn't work through
V8ValueTraits<>::toV8Value(). I do not know why or understand at present. Do you
have suggestions on where we can look to answer this?

[haraken, 2014-07-20 05:16:36]

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

https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:263: // should also have an explicit
specialization of toV8NoInline<>() in V8Binding.cpp, so that vectors of the same
type can also be converted.
On 2014/07/20 05:11:16, gavinp wrote:
> On 2014/07/20 04:50:46, haraken wrote:
> > 
> > Just help me understand: Why do we need to have explicit specialization of
> > toV8NoInline<>() whereas we don't need to have explicit specialization of
> > toV8<>()?
> 
> I do not know; I am very new in this coding area.
> 
> It's very interesting that IDL can bind function return values of
Vector<String>
> correctly (this occurs a number of times), but that they didn't work through
> V8ValueTraits<>::toV8Value(). I do not know why or understand at present. Do
you
> have suggestions on where we can look to answer this?

I think we can ask yhirano@ or tasak@ :) The toV8 family is insanely complicated
to understand for us...

[yhirano, 2014-07-22 01:46:33]

On 2014/07/20 05:16:36, haraken wrote:
>
https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/400493005/diff/70001/Source/bindings/core/v8/...
> Source/bindings/core/v8/V8Binding.h:263: // should also have an explicit
> specialization of toV8NoInline<>() in V8Binding.cpp, so that vectors of the
same
> type can also be converted.
> On 2014/07/20 05:11:16, gavinp wrote:
> > On 2014/07/20 04:50:46, haraken wrote:
> > > 
> > > Just help me understand: Why do we need to have explicit specialization of
> > > toV8NoInline<>() whereas we don't need to have explicit specialization of
> > > toV8<>()?
> > 
> > I do not know; I am very new in this coding area.
> > 
> > It's very interesting that IDL can bind function return values of
> Vector<String>
> > correctly (this occurs a number of times), but that they didn't work through
> > V8ValueTraits<>::toV8Value(). I do not know why or understand at present. Do
> you
> > have suggestions on where we can look to answer this?
> 
> I think we can ask yhirano@ or tasak@ :) The toV8 family is insanely
complicated
> to understand for us...

Currently toV8 and toV8NoInline are generated for each ScriptWrappable class to
convert its object to a V8 value.
V8ValueTraits<T>::toV8Value is implemented for more general types including
primitives and containers.
And V8ValueTraits<T*>::toV8Value uses toV8NoInline, not toV8, to be available
without including the corresponding generated header.

Either of the following look reasonable to me:
 - Remove toV8NoInline and generate V8ValueTraits<T*> for each ScriptWrappable
class.
 - Make v8ArrayNoInline use V8ValueTraits::toV8Value instead of toV8NoInline.
  - Optionally you can remove v8ArrayNoInline and implement it as
V8ValueTraits::toV8Value directly.

[gavinp, 2014-07-22 02:16:47]

On 2014/07/22 01:46:33, yhirano wrote:
> 
> Either of the following look reasonable to me:
>  - Remove toV8NoInline and generate V8ValueTraits<T*> for each ScriptWrappable
> class.
>  - Make v8ArrayNoInline use V8ValueTraits::toV8Value instead of toV8NoInline.
>   - Optionally you can remove v8ArrayNoInline and implement it as
> V8ValueTraits::toV8Value directly.

I think this is a really good suggestion; it fixes the instant bug, and also
simplifies this complicated code by removing a layer of indirection that is not
needed. Thanks for taking a look Hirano-san, I'm going to proceed and upload the
version you suggest.

[gavinp, 2014-07-22 08:40:15]

Hirano-san, Hara-san,

Here's the latest version of this patch. I kept toV8NoInline() in the end,
because the problem was really just v8ArrayNoInline, which isn't a good idea to
use in the general case. The rules of explicit specialization of classes would
have required a declaration at each call site, whereas the function template
doesn't require that, so it fits much better in to our code generation
framework.

What do you think?

[haraken, 2014-07-22 08:57:07]

On 2014/07/22 08:40:15, gavinp wrote:
> Hirano-san, Hara-san,
> 
> Here's the latest version of this patch. I kept toV8NoInline() in the end,
> because the problem was really just v8ArrayNoInline, which isn't a good idea
to
> use in the general case. The rules of explicit specialization of classes would
> have required a declaration at each call site, whereas the function template
> doesn't require that, so it fits much better in to our code generation
> framework.
> 
> What do you think?

If we can remove toV8ArrayNoInline, that would be the best; i.e., if this CL
works, I'm very happy :)

hirano-san: would you take a look?

[yhirano, 2014-07-23 01:51:52]

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

https://codereview.chromium.org/400493005/diff/110001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:406: struct V8ValueTraits<WTF::Vector<T,
inlineCapacity, Allocator> > {
Can you tell me why you move toV8Value functions?

[gavinp, 2014-07-23 01:57:24]

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

https://codereview.chromium.org/400493005/diff/110001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:406: struct V8ValueTraits<WTF::Vector<T,
inlineCapacity, Allocator> > {
On 2014/07/23 01:51:52, yhirano wrote:
> Can you tell me why you move toV8Value functions?

Type deduction across namespaces for function template resolution requires
definition in the correct order. This comes up with Vector<WTF::String> going to
the partial specialization of toV8Value<>(). So we can either declare the
toV8Value functions after the v8Array function that they use, or we can forward
declare toV8Value, like the previous CL did for toV8ValueNoInline.

Since it's the same translation unit, ordering to avoid a forward declaration
seemed like the way to go.

What do you think?

[yhirano, 2014-07-23 02:24:32]

Thanks, lgtm

[haraken, 2014-07-23 02:25:00]

LGTM2

[gavinp, 2014-07-23 02:45:15]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-07-23 02:46:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/400493005/110001

[commit-bot: I haz the power, 2014-07-23 03:32:18]

Change committed as 178724