bindings: Use Maybe APIs in ScriptPromise

bindings: Use Maybe APIs in ScriptPromise

Replacing following APIs.
- Promise::Resolver::New
- Promise::Resolver::Resolve
- Promise::Resolver::Reject
- Promise::Catch
- Promise::Then

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

[bashi, 2015-03-25 03:52:28]

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

[bashi, 2015-03-25 03:52:29]

Hirano-san, PTAL?

Here is an announcement of V8 API change.
https://groups.google.com/d/msg/v8-users/gQVpp1HmbqM/W_ZIMIZP4SgJ

We are going to add empty checks everywhere we call V8 APIs which can throw
exceptions.

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromise.cpp (left):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromise.cpp:117: // v8::Promise::Then may return
an empty value, for example when
Removed this comment as this can be said on all other Maybe APIs.

[haraken, 2015-03-25 04:29:59]

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromise.cpp:170: if
(!v8::Promise::Resolver::New(isolate->GetCurrentContext()).ToLocal(&resolver))

It seems better to pass ScriptState* to rejectWithDOMException. We want to avoid
calling GetCurrentContext in places where it's not clear we're in a correct
context or not. It's OK to call GetCurrentContext in binding callbacks (that are
directly called back by JS) but we want to avoid GetCurrentContext in other
places as much as possible.

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112:
v8::Local<v8::Context> context = m_isolate->GetCurrentContext();

Not directly related to your CL, ScriptPromisePropertyBase should record the
ScriptState from which the ScriptPromisePropertyBase is created, and use the
recorded ScriptState here.

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

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValue.h:97: return m_scriptState ?
isolate()->GetCurrentContext() : v8::Local<v8::Context>();

This seems wrong. We need to use m_scriptState->context().

Also I don't think it is valid to return an empty context from this method. If
an empty context is returned, the caller side will crash when the empty context
is passed in to a next V8 API.

[bashi, 2015-03-25 04:56:00]

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromise.cpp:170: if
(!v8::Promise::Resolver::New(isolate->GetCurrentContext()).ToLocal(&resolver))
On 2015/03/25 04:29:59, haraken wrote:
> 
> It seems better to pass ScriptState* to rejectWithDOMException. We want to
avoid
> calling GetCurrentContext in places where it's not clear we're in a correct
> context or not. It's OK to call GetCurrentContext in binding callbacks (that
are
> directly called back by JS) but we want to avoid GetCurrentContext in other
> places as much as possible.
> 

Done.

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112:
v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
On 2015/03/25 04:29:59, haraken wrote:
> 
> Not directly related to your CL, ScriptPromisePropertyBase should record the
> ScriptState from which the ScriptPromisePropertyBase is created, and use the
> recorded ScriptState here.

Added a todo comment.

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

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValue.h:97: return m_scriptState ?
isolate()->GetCurrentContext() : v8::Local<v8::Context>();
On 2015/03/25 04:29:59, haraken wrote:
> 
> This seems wrong. We need to use m_scriptState->context().
> 
> Also I don't think it is valid to return an empty context from this method. If
> an empty context is returned, the caller side will crash when the empty
context
> is passed in to a next V8 API.
> 

Done.

[haraken, 2015-03-25 05:08:29]

LGTM

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptPromisePropertyBase.h:74: // v8::Isolate*.

Yeah, let's fix this in a follow-up. It looks wrong to use a current context in
resolveOrRejectInternal. There is no guarantee that the current context is the
context on which the promise wants to be resolved. (The promise needs to be
resolved on the context from which the promise was created.)

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:67: ASSERT(isEmpty() || m_scriptState);

I'm just wondering but is it valid that m_scriptState is 0 here? (You can
address this in a follow-up.)

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:75: ASSERT(isEmpty() || m_scriptState);

Ditto.

[bashi, 2015-03-25 07:25:32]

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptPromisePropertyBase.h:74: // v8::Isolate*.
On 2015/03/25 05:08:29, haraken wrote:
> 
> Yeah, let's fix this in a follow-up. It looks wrong to use a current context
in
> resolveOrRejectInternal. There is no guarantee that the current context is the
> context on which the promise wants to be resolved. (The promise needs to be
> resolved on the context from which the promise was created.)

Will do.

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:67: ASSERT(isEmpty() || m_scriptState);
On 2015/03/25 05:08:29, haraken wrote:
> 
> I'm just wondering but is it valid that m_scriptState is 0 here? (You can
> address this in a follow-up.)

You mean we should just ASSERT(m_scriptState)? I think it makes sense.

[bashi, 2015-03-25 07:35:01]

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:67: ASSERT(isEmpty() || m_scriptState);
On 2015/03/25 07:25:32, bashi1 wrote:
> On 2015/03/25 05:08:29, haraken wrote:
> > 
> > I'm just wondering but is it valid that m_scriptState is 0 here? (You can
> > address this in a follow-up.)
> 
> You mean we should just ASSERT(m_scriptState)? I think it makes sense.

It seems that some tests fail when dropping isEmpty(), so I'll keep the current
condition.

[yhirano, 2015-03-25 07:39:08]

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112:
v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
On 2015/03/25 04:56:00, bashi1 wrote:
> On 2015/03/25 04:29:59, haraken wrote:
> > 
> > Not directly related to your CL, ScriptPromisePropertyBase should record the
> > ScriptState from which the ScriptPromisePropertyBase is created, and use the
> > recorded ScriptState here.
> 
> Added a todo comment.

No, I don't think so. ScriptPromiseProperty is designed to be world-neutral and
having a one special world (or ScriptState) is not right.
resolver->CreationContext() is the right context (see
ScriptPromisePropertyBase::resolveOrReject.)

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:118:
resolver->Resolve(context, resolvedValue(m_isolate,
resolver->CreationContext()->Global()));
resolver->CreationContext() can be replaced with |context|.

[bashi, 2015-03-25 08:01:46]

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112:
v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
On 2015/03/25 07:39:08, yhirano wrote:
> On 2015/03/25 04:56:00, bashi1 wrote:
> > On 2015/03/25 04:29:59, haraken wrote:
> > > 
> > > Not directly related to your CL, ScriptPromisePropertyBase should record
the
> > > ScriptState from which the ScriptPromisePropertyBase is created, and use
the
> > > recorded ScriptState here.
> > 
> > Added a todo comment.
> 
> No, I don't think so. ScriptPromiseProperty is designed to be world-neutral
and
> having a one special world (or ScriptState) is not right.
> resolver->CreationContext() is the right context (see
> ScriptPromisePropertyBase::resolveOrReject.)

Ah, I should have read the code carefully. Thanks. Done.

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:118:
resolver->Resolve(context, resolvedValue(m_isolate,
resolver->CreationContext()->Global()));
On 2015/03/25 07:39:08, yhirano wrote:
> resolver->CreationContext() can be replaced with |context|.

Done.

[yhirano, 2015-03-25 08:09:13]

lgtm

[bashi, 2015-03-25 08:09:38]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-03-25 08:09:39]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1035623002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-03-25 08:09:52]

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

[haraken, 2015-03-25 09:04:23]

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptPromisePropertyBase.cpp (right):

https://codereview.chromium.org/1035623002/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:112:
v8::Local<v8::Context> context = m_isolate->GetCurrentContext();
On 2015/03/25 07:39:08, yhirano wrote:
> On 2015/03/25 04:56:00, bashi1 wrote:
> > On 2015/03/25 04:29:59, haraken wrote:
> > > 
> > > Not directly related to your CL, ScriptPromisePropertyBase should record
the
> > > ScriptState from which the ScriptPromisePropertyBase is created, and use
the
> > > recorded ScriptState here.
> > 
> > Added a todo comment.
> 
> No, I don't think so. ScriptPromiseProperty is designed to be world-neutral
and
> having a one special world (or ScriptState) is not right.
> resolver->CreationContext() is the right context (see
> ScriptPromisePropertyBase::resolveOrReject.)

Thanks for the catch, you're right :)

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

https://codereview.chromium.org/1035623002/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:67: ASSERT(isEmpty() || m_scriptState);
On 2015/03/25 07:35:01, bashi1 wrote:
> On 2015/03/25 07:25:32, bashi1 wrote:
> > On 2015/03/25 05:08:29, haraken wrote:
> > > 
> > > I'm just wondering but is it valid that m_scriptState is 0 here? (You can
> > > address this in a follow-up.)
> > 
> > You mean we should just ASSERT(m_scriptState)? I think it makes sense.
> 
> It seems that some tests fail when dropping isEmpty(), so I'll keep the
current
> condition.

Thanks for the investigation. Yeah, if the value is empty, the value doesn't
need to be associated with any ScriptState, so allowing no ScriptState when the
value is empty would make sense. Sorry about the noise.

[commit-bot: I haz the power, 2015-03-25 09:19:42]

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