ScriptPromise should understand the ScriptState from which the ScriptPromise is generated

ScriptPromise should understand the ScriptState from which the ScriptPromise is generated

I'm making ScriptValue understand the ScriptState from which the ScriptValue is created. For that goal, this CL makes ScriptPromise understand the ScriptState from which the ScriptPromise is created. For that goal, this CL passes around ScriptState in a lot of places.

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

[haraken, 2014-05-08 14:08:41]

yhirano@: This CL is making non-trivial changes to Promise implementation. Would
you mind taking a look at them first?

[yhirano, 2014-05-09 06:24:56]

One question: ScriptPromise::m_scriptState should be removed when ScriptValue
has its own ScriptState, is that right?

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.cpp:48:
ASSERT(!m_scriptState->contextIsEmpty());
Please keep the InContext assertion.

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.cpp:67:
ASSERT(!m_scriptState->contextIsEmpty());
ditto

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

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:149: return
ToV8Value<ScriptPromiseResolver, v8::Isolate*>::toV8Value(value,
m_scriptState->isolate(), m_scriptState->isolate());
This can be ToV8Value<ScriptPromiseResolver, ScriptState*> .

https://codereview.chromium.org/273683006/diff/20001/Source/core/css/FontFace...
File Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/273683006/diff/20001/Source/core/css/FontFace...
Source/core/css/FontFaceSet.idl:2: * Copyright (C) 2013 Google Inc. All rights
reerved.
??

[haraken, 2014-05-09 07:37:24]

Thanks for review! PTAL.

> One question: ScriptPromise::m_scriptState should be removed when ScriptValue
> has its own ScriptState, is that right?

Right. I added a FIXME to remove ScriptPromiseResolver::m_scriptState once
ScriptValue::scriptState becomes available.

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.cpp:48:
ASSERT(!m_scriptState->contextIsEmpty());
On 2014/05/09 06:24:57, yhirano wrote:
> Please keep the InContext assertion.

Done.

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.cpp:67:
ASSERT(!m_scriptState->contextIsEmpty());
On 2014/05/09 06:24:57, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.cpp:67:
ASSERT(!m_scriptState->contextIsEmpty());
On 2014/05/09 06:24:57, yhirano wrote:
> ditto

Done.

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

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:149: return
ToV8Value<ScriptPromiseResolver, v8::Isolate*>::toV8Value(value,
m_scriptState->isolate(), m_scriptState->isolate());
On 2014/05/09 06:24:57, yhirano wrote:
> This can be ToV8Value<ScriptPromiseResolver, ScriptState*> .

Actually I don't fully understand this template. If I change it to
ToV8Value<ScriptPromiseResolver, ScriptState*>, I hit the compile error.

In the first place, why are we passing m_isolate to both second and the third
parameter?

https://codereview.chromium.org/273683006/diff/20001/Source/core/css/FontFace...
File Source/core/css/FontFaceSet.idl (right):

https://codereview.chromium.org/273683006/diff/20001/Source/core/css/FontFace...
Source/core/css/FontFaceSet.idl:2: * Copyright (C) 2013 Google Inc. All rights
reerved.
On 2014/05/09 06:24:57, yhirano wrote:
> ??

Done.

[yhirano, 2014-05-09 07:58:36]

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

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:149: return
ToV8Value<ScriptPromiseResolver, v8::Isolate*>::toV8Value(value,
m_scriptState->isolate(), m_scriptState->isolate());
On 2014/05/09 07:37:25, haraken wrote:
> On 2014/05/09 06:24:57, yhirano wrote:
> > This can be ToV8Value<ScriptPromiseResolver, ScriptState*> .
> 
> Actually I don't fully understand this template. If I change it to
> ToV8Value<ScriptPromiseResolver, ScriptState*>, I hit the compile error.
> 
> In the first place, why are we passing m_isolate to both second and the third
> parameter?

The second parameter is used for getting the creation context and its type is
the second template parameter of ToV8Value. The third parameter type is always
v8::Isolate*. Please compare with the above toV8Value.

The second parameter is given to T::getCreationContext where T is the first
template parameter of ToV8Value. The current code gets the isolate from the
scriptState in the second toV8Value, gets the scriptState from the given isolate
in the second getCreationContext and get the V8 context from it.
By changing both the second toV8Value and the second getCreationContext, you can
save unnecessary ScriptState -> isolate -> Scriptstate conversion.

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

https://codereview.chromium.org/273683006/diff/60001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:134: static v8::Handle<v8::Object>
getCreationContext(v8::Isolate* isolate)
So you have to change this function to take ScriptState* instead of
v8::Isolate*.

[haraken, 2014-05-09 08:42:00]

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

https://codereview.chromium.org/273683006/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:149: return
ToV8Value<ScriptPromiseResolver, v8::Isolate*>::toV8Value(value,
m_scriptState->isolate(), m_scriptState->isolate());
On 2014/05/09 07:58:37, yhirano wrote:
> On 2014/05/09 07:37:25, haraken wrote:
> > On 2014/05/09 06:24:57, yhirano wrote:
> > > This can be ToV8Value<ScriptPromiseResolver, ScriptState*> .
> > 
> > Actually I don't fully understand this template. If I change it to
> > ToV8Value<ScriptPromiseResolver, ScriptState*>, I hit the compile error.
> > 
> > In the first place, why are we passing m_isolate to both second and the
third
> > parameter?
> 
> The second parameter is used for getting the creation context and its type is
> the second template parameter of ToV8Value. The third parameter type is always
> v8::Isolate*. Please compare with the above toV8Value.
> 
> The second parameter is given to T::getCreationContext where T is the first
> template parameter of ToV8Value. The current code gets the isolate from the
> scriptState in the second toV8Value, gets the scriptState from the given
isolate
> in the second getCreationContext and get the V8 context from it.
> By changing both the second toV8Value and the second getCreationContext, you
can
> save unnecessary ScriptState -> isolate -> Scriptstate conversion.

Thanks for the details, done.

[yhirano, 2014-05-09 08:58:01]

lgtm

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

https://codereview.chromium.org/273683006/diff/80001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:133: // Used by
ToV8Value<ScriptPromiseResolver, v8::Isolate*>.
Can you fix this comment, too?

[haraken, 2014-05-09 08:59:29]

Thanks for review!

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

https://codereview.chromium.org/273683006/diff/80001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolver.h:133: // Used by
ToV8Value<ScriptPromiseResolver, v8::Isolate*>.
On 2014/05/09 08:58:02, yhirano wrote:
> Can you fix this comment, too?

Done.

[haraken, 2014-05-09 08:59:36]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-09 09:01:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/273683006/100001

[commit-bot: I haz the power, 2014-05-09 10:17:33]

Change committed as 173743