[ServiceWorker, PushMessaging] Use ScriptPromiseResolverWithContext.

[ServiceWorker, PushMessaging] Use ScriptPromiseResolverWithContext.

Use ScriptPromiseResolverWithContext instead of a pair of
ScriptPromiseResolver and NewScriptState.
This CL doen't change the behavior in many cases: The only exception
is when the resolution / rejection happens on the suspended or stopped
ExecutionContext. In such a case, ScriptPromiseResolver suspends or stops
resolution or rejection.

R=dominicc@chromium.org, mvanouwerkerk@chromium.org
TBR=mvanouwerkerk@chromium.org
BUG=352552
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172011

[haraken, 2014-04-14 12:13:16]

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
File Source/bindings/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
Source/bindings/v8/CallbackPromiseAdapter.h:75: return;

This will change existing behavior in a case where onSuccess() is called after
an ExecutionContext is stopped. In the old code, resolve() was called. In the
new code, nothing is called. Is this change OK?

In addition, even if you want to return immediately when the ExecutionContext is
gone, you should check it by the existence of
ScriptPromiseResolverWithContext::m_resolver (instead of the existence of
ScriptPromiseResolverWithContext::m_scriptState). As I commented in
ScriptPromiseResolverWithContext.h, m_scriptState shouldn't be cleared out until
the holder object dies.

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
Source/bindings/v8/CallbackPromiseAdapter.h:81: return;

Ditto.

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

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:65: // The returned
pointer can be null.

(I should have pointed this out in the previous code review but) you don't need
to clear m_scriptState when an ExecutionContext is stopped. NewScriptState
should be valid as long as the holder object is alive.

[yhirano, 2014-04-14 12:37:23]

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
File Source/bindings/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
Source/bindings/v8/CallbackPromiseAdapter.h:75: return;
On 2014/04/14 12:13:17, haraken wrote:
> 
> This will change existing behavior in a case where onSuccess() is called after
> an ExecutionContext is stopped. In the old code, resolve() was called. In the
> new code, nothing is called. Is this change OK?
> 
> In addition, even if you want to return immediately when the ExecutionContext
is
> gone, you should check it by the existence of
> ScriptPromiseResolverWithContext::m_resolver (instead of the existence of
> ScriptPromiseResolverWithContext::m_scriptState). As I commented in
> ScriptPromiseResolverWithContext.h, m_scriptState shouldn't be cleared out
until
> the holder object dies.
Yes, this CL changes the behavior as written in the description, though the
blink Promises can't work with a stopped execution context (see [A]).
In addition to that, current ScriptPromiseResolverWithContext ignores resolve or
reject callls when the execution context is stopped.

What should a resolver do when resolve or reject called on a stopped execution
context?
I think of three options:
1. Do nothing (the current behavior)
2. Work normally
3. The resolver rejects the associated promise with a certain exception when the
executionContext is stopped.

A:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[dominicc (has gone to gerrit), 2014-04-15 00:22:15]

I *think* ServiceWorker can live with this, but there's one detail I'm not sure
about.

Currently you could pair CallbackPromiseAdapter with one ExecutionContext and
ScriptPromiseResolverWithContext with another ExecutionContext and create the
WebCore object associated with one context (from the CallbackPromiseAdapter) and
call the promise in another context (from the ScriptPromiseResolverWithContext.)

Wouldn't you need something like that when you're accessing something
cross-frame?

[yhirano, 2014-04-15 01:02:17]

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
File Source/bindings/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Callb...
Source/bindings/v8/CallbackPromiseAdapter.h:75: return;
On 2014/04/14 12:37:23, yhirano wrote:
> On 2014/04/14 12:13:17, haraken wrote:
> > 
> > This will change existing behavior in a case where onSuccess() is called
after
> > an ExecutionContext is stopped. In the old code, resolve() was called. In
the
> > new code, nothing is called. Is this change OK?
> > 
> > In addition, even if you want to return immediately when the
ExecutionContext
> is
> > gone, you should check it by the existence of
> > ScriptPromiseResolverWithContext::m_resolver (instead of the existence of
> > ScriptPromiseResolverWithContext::m_scriptState). As I commented in
> > ScriptPromiseResolverWithContext.h, m_scriptState shouldn't be cleared out
> until
> > the holder object dies.
> Yes, this CL changes the behavior as written in the description, though the
> blink Promises can't work with a stopped execution context (see [A]).
> In addition to that, current ScriptPromiseResolverWithContext ignores resolve
or
> reject callls when the execution context is stopped.
> 
> What should a resolver do when resolve or reject called on a stopped execution
> context?
> I think of three options:
> 1. Do nothing (the current behavior)
> 2. Work normally
> 3. The resolver rejects the associated promise with a certain exception when
the
> executionContext is stopped.
> 
> A:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 

I made ScriptPromiseResolver::m_scriptState const so that we can use after the
resolver is cleared.

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

https://codereview.chromium.org/236713003/diff/20001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:65: // The returned
pointer can be null.
On 2014/04/14 12:13:17, haraken wrote:
> 
> (I should have pointed this out in the previous code review but) you don't
need
> to clear m_scriptState when an ExecutionContext is stopped. NewScriptState
> should be valid as long as the holder object is alive.

Done.

[yhirano, 2014-04-15 01:11:22]

On 2014/04/15 00:22:15, dominicc wrote:
> I *think* ServiceWorker can live with this, but there's one detail I'm not
sure
> about.
> 
> Currently you could pair CallbackPromiseAdapter with one ExecutionContext and
> ScriptPromiseResolverWithContext with another ExecutionContext and create the
> WebCore object associated with one context (from the CallbackPromiseAdapter)
and
> call the promise in another context (from the
ScriptPromiseResolverWithContext.)
> 
> Wouldn't you need something like that when you're accessing something
> cross-frame?

You are talking about wrappers, right?
A Promise doesn't have its wrapper, so the only wrapper we need to care about is
the wrapper of a resolution / rejection target.
We could add more resolve / reject functions to ScriptPromiseResolveWithContext
if you need, like:

template<typename T>
void resolve(T value, ExecutionContext*);

But I'm not sure if you need.

[haraken, 2014-04-15 01:27:46]

LGTM in terms of implementation.

[dominicc (has gone to gerrit), 2014-04-15 02:22:41]

On 2014/04/15 01:27:46, haraken wrote:
> LGTM in terms of implementation.

LGTM too, I'm not in modules/serviceworkers OWNERS yet tho', sorry. You'll need
kinuko.

[yhirano, 2014-04-15 02:30:22]

+kinuko for OWNER review

[kinuko, 2014-04-15 02:31:12]

lgtm

[yhirano, 2014-04-16 13:26:00]

mvanouwerkerk: ping

[yhirano, 2014-04-18 15:44:17]

mvanouwerkerk: ping

[dominicc (has gone to gerrit), 2014-04-21 03:38:32]

I think it is safe to TBR mvanouwerkerk since you wrote this bit of
infrastructure and you've given him a reasonable time to reply. If there's
a problem with this we can deal with it post-hoc.

Dominic


On Sat, Apr 19, 2014 at 12:44 AM, <yhirano@chromium.org> wrote:

> mvanouwerkerk: ping
>
> https://codereview.chromium.org/236713003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yhirano, 2014-04-21 04:55:29]

On 2014/04/21 03:38:32, dominicc wrote:
> I think it is safe to TBR mvanouwerkerk since you wrote this bit of
> infrastructure and you've given him a reasonable time to reply. If there's
> a problem with this we can deal with it post-hoc.
> 
> Dominic
> 
Thanks, Dominic!

[yhirano, 2014-04-21 04:55:35]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-04-21 04:55:43]

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

[commit-bot: I haz the power, 2014-04-21 05:04:45]

Change committed as 172011

[Michael van Ouwerkerk, 2014-04-29 16:46:15]

On 2014/04/21 05:04:45, I haz the power (commit-bot) wrote:
> Change committed as 172011

Apologies for the non-response, I was OOO. lgtm