Implement V8ValueTraits<T>::toV8Value conversion functions

Implement V8ValueTraits<T>::toV8Value conversion functions

Add toV8Value functions to V8ValueTraits<T>. It converts many types to v8::Handle<V8::Value>.
Make ScriptPromiseResolver and ScriptPromiseResolverWithContext use them.

BUG=352552
R=haraken

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

[yhirano, 2014-04-17 03:32:09]

Can you take a look at this CL?
This change was originally included in
https://codereview.chromium.org/236673003/ . But it may take some more time to
land the CL, and another CL may use these functions. So I would like to decouple
the change.

[yhirano, 2014-04-17 03:36:43]

On 2014/04/17 03:32:09, yhirano wrote:
> Can you take a look at this CL?
> This change was originally included in
> https://codereview.chromium.org/236673003/ . But it may take some more time to
> land the CL, and another CL may use these functions. So I would like to
decouple
> the change.

"another CL" is https://codereview.chromium.org/228783007/ . I will ask the
author to use ScriptPromiseResolverWithContext, but it resolves a Promise with
undefined, so this change is needed.

[haraken, 2014-04-17 03:46:23]

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:54:
m_value.set(m_scriptState->isolate(), toV8(value));

I'm not sure if it's a good idea to introduce toV8() just for Promise.

As a bottom line, we can write:

  resolve(v8::Integer::New(value));

in the call sites. Then you just need to define resolve(v8::Handle<v8::Value>).

If you feel that writing v8::Integer::New(value) in all call sites of
resolve/reject, you can define helper methods in V8Binding.h and use them. The
helper methods are not only for Promise but also for all other bindings.

What do you think? (I'm OK with introducing the helper methods in a follow-up CL
if you need them.)

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:128: v8::Handle<v8::Value>
toV8(uint8_t value) { return v8::Integer::New(m_scriptState->isolate(), value);
}

Don't you want to use v8::Integer::NewFromUnsigned?

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:130: v8::Handle<v8::Value>
toV8(uint16_t value) { return v8::Integer::New(m_scriptState->isolate(), value);
}

Ditto.

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:132: v8::Handle<v8::Value>
toV8(uint32_t value) { return v8::Number::New(m_scriptState->isolate(), value);
}

Ditto.

[haraken, 2014-04-17 03:47:37]

> If you feel that writing v8::Integer::New(value) in all call sites of
> resolve/reject,

Correction: If you feel that writing v8::Integer::New(value) in all call sites
of resolve/reject is verbose,

[yhirano, 2014-04-17 03:52:24]

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):

https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:54:
m_value.set(m_scriptState->isolate(), toV8(value));
On 2014/04/17 03:46:23, haraken wrote:
> 
> I'm not sure if it's a good idea to introduce toV8() just for Promise.
> 
> As a bottom line, we can write:
> 
>   resolve(v8::Integer::New(value));
> 
> in the call sites. Then you just need to define
resolve(v8::Handle<v8::Value>).
> 
> If you feel that writing v8::Integer::New(value) in all call sites of
> resolve/reject, you can define helper methods in V8Binding.h and use them. The
> helper methods are not only for Promise but also for all other bindings.
> 
> What do you think? (I'm OK with introducing the helper methods in a follow-up
CL
> if you need them.)
I think HandleScope is needed to use v8::Undefined(), v8::...::New, etc, because
they create handles (whereas entering a v8 Context is not needed). Is that
correct?

[haraken, 2014-04-17 04:14:58]

On 2014/04/17 03:52:24, yhirano wrote:
>
https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
> File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):
> 
>
https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
> Source/bindings/v8/ScriptPromiseResolverWithContext.h:54:
> m_value.set(m_scriptState->isolate(), toV8(value));
> On 2014/04/17 03:46:23, haraken wrote:
> > 
> > I'm not sure if it's a good idea to introduce toV8() just for Promise.
> > 
> > As a bottom line, we can write:
> > 
> >   resolve(v8::Integer::New(value));
> > 
> > in the call sites. Then you just need to define
> resolve(v8::Handle<v8::Value>).
> > 
> > If you feel that writing v8::Integer::New(value) in all call sites of
> > resolve/reject, you can define helper methods in V8Binding.h and use them.
The
> > helper methods are not only for Promise but also for all other bindings.
> > 
> > What do you think? (I'm OK with introducing the helper methods in a
follow-up
> CL
> > if you need them.)
> I think HandleScope is needed to use v8::Undefined(), v8::...::New, etc,
because
> they create handles (whereas entering a v8 Context is not needed). Is that
> correct?

Yes, HandleScope is needed. So you want to put toV8() into resolve/reject
because you don't want to write a HandleScope in the call site. That sounds
reasonable.

Then shall we just move the toV8() functions to V8Binding.h so that other
binding code can use them as well? (Probably we want to rename it to toV8Value()
since toV8() is used for wrappers.)

[haraken, 2014-04-17 06:10:49]

On 2014/04/17 04:14:58, haraken wrote:
> On 2014/04/17 03:52:24, yhirano wrote:
> >
>
https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
> > File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):
> > 
> >
>
https://codereview.chromium.org/238723009/diff/1/Source/bindings/v8/ScriptPro...
> > Source/bindings/v8/ScriptPromiseResolverWithContext.h:54:
> > m_value.set(m_scriptState->isolate(), toV8(value));
> > On 2014/04/17 03:46:23, haraken wrote:
> > > 
> > > I'm not sure if it's a good idea to introduce toV8() just for Promise.
> > > 
> > > As a bottom line, we can write:
> > > 
> > >   resolve(v8::Integer::New(value));
> > > 
> > > in the call sites. Then you just need to define
> > resolve(v8::Handle<v8::Value>).
> > > 
> > > If you feel that writing v8::Integer::New(value) in all call sites of
> > > resolve/reject, you can define helper methods in V8Binding.h and use them.
> The
> > > helper methods are not only for Promise but also for all other bindings.
> > > 
> > > What do you think? (I'm OK with introducing the helper methods in a
> follow-up
> > CL
> > > if you need them.)
> > I think HandleScope is needed to use v8::Undefined(), v8::...::New, etc,
> because
> > they create handles (whereas entering a v8 Context is not needed). Is that
> > correct?
> 
> Yes, HandleScope is needed. So you want to put toV8() into resolve/reject
> because you don't want to write a HandleScope in the call site. That sounds
> reasonable.
> 
> Then shall we just move the toV8() functions to V8Binding.h so that other
> binding code can use them as well? (Probably we want to rename it to
toV8Value()
> since toV8() is used for wrappers.)

V8Binding.h already has V8ValueTraits, so you can probably use it.

[yhirano, 2014-04-17 11:23:01]

Done.

V8ValueTraits<T>::arrayValue and V8ValueTraits<T>::toV8Value are very similar,
so we may be able to unify them, in the future.

[haraken, 2014-04-17 12:39:05]

This looks much better.

> V8ValueTraits<T>::arrayValue and V8ValueTraits<T>::toV8Value are very similar,
> so we may be able to unify them, in the future.

Sorry for asking you too much, but wouldn't it be possible to avoid the
duplication between V8ValueTraits<T>::arrayValue and
V8ValueTraits<T>::toV8Value? (If you're planning to do the clean-up in a
follow-up CL shortly, I'm OK with landing this CL first.)

[haraken, 2014-04-17 12:42:27]

https://codereview.chromium.org/238723009/diff/50001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/238723009/diff/50001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:272: //                                   set
false otherwise.

I'm not sure if we need the toV8ValueNeedsCreationContext flag. For objects that
need a creation context should use toV8(). For objects that don't need a
creation context should use toV8Value(). Then we don't need the flag to
distinguish these, do we?

It would be confusing to have both toV8() and toV8Value() to wrap one object.

[yhirano, 2014-04-18 01:09:42]

https://codereview.chromium.org/238723009/diff/50001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/238723009/diff/50001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:272: //                                   set
false otherwise.
On 2014/04/17 12:42:27, haraken wrote:
> 
> I'm not sure if we need the toV8ValueNeedsCreationContext flag. For objects
that
> need a creation context should use toV8(). For objects that don't need a
> creation context should use toV8Value(). Then we don't need the flag to
> distinguish these, do we?
> 
> It would be confusing to have both toV8() and toV8Value() to wrap one object.

Do you mean that V8ValueTraits are for the types that don't need creationContext
for the conversion and There should not be V8ValueTraits<*Ptr<T> > ?

[yhirano, 2014-04-18 03:11:40]

At PS6 I reduced idential definitions by introducing
V8ValueTraitsToV8ValueIsIdentialToArrayJsValue.
I also introduced ToV8ValueHelper in V8Binding.h to make it easy to define a
custom toV8Value (like in ScriptPromiseResolver).

[haraken, 2014-04-18 03:59:58]

Hmm, actually I don't quite understand why this CL becomes such complicated. Now
you have toV8Value, toV8ValueHelper, arrayV8Value, toV8ValueNeedsCreationContext
and V8ValueTraitsToV8ValueIsIdentialToArrayJsValue.

We don't want to introduce complicated templates into the code base. Let me chat
with you offline.

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:92: resolve(value,
v8::Object::New(m_isolate));

Do you want to create a new object just for getting a creation context? Can't we
use a global object of the current context?

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:98: reject(value,
v8::Object::New(m_isolate));

Ditto.

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:137: v8::Handle<v8::Context>
v8Context = toV8Context(context, DOMWrapperWorld::current(isolate));

Not related to this CL, I think it's wrong to use the current world here. It's
not guaranteed that the current world at this point is the world in which we
want to create a new V8 object. (I'll fix the issue in the near future.)

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/V8Bin...
File Source/bindings/v8/V8Binding.h (right):

https://codereview.chromium.org/238723009/diff/90001/Source/bindings/v8/V8Bin...
Source/bindings/v8/V8Binding.h:465: template<typename T, size_t inlinCapacity>

inlineCapacity

[yhirano, 2014-04-18 06:31:25]

Discussed with haraken, we agreed the following.
 - Use V8ValueTraits for types that don't need the creation context for the
conversion.
 - Write the conversion manually for types that need the creation context for
the conversion in ScriptPromiseResolver[WithContext].

Done.

[haraken, 2014-04-18 07:07:23]

Discussed offline. It's a shame that we have to write a lot of specialization of
toV8Value() in all files that want to use toV8Value(), but we couldn't come up
with any better idea. So LGTM.

Recently toV8 family is getting very dirty (toV8, toV8NoInline, toV8Value,
arrayV8Value, toV8ValueTraits etc). We should seriously think about the
refactoring.

One renaming suggestion: arrayV8Value => toV8Value ?

[yhirano, 2014-04-18 07:17:37]

Thanks. The renaming is done.

[yhirano, 2014-04-18 07:17:47]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-04-18 07:17:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/238723009/190001

[commit-bot: I haz the power, 2014-04-18 08:19:09]

Change committed as 171932