Pass script state to Web API object->script wrappable converter.

Pass script state to Web API object->script wrappable converter.

This is required to create script wrappable objects which depend on
their execution context, such as EventTargets and ActiveDOMObjects.

BUG=349919
TBR=mvanouwerkerk@chromium.org

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

[dominicc (has gone to gerrit), 2014-04-11 23:23:09]

PTAL

abarth: bindings
mvanouwerkerk: modules/push_messaging
michaeln: modules/serviceworkers

There are lots of Blink objects which rely on an execution context
(EventTargets, ActiveDOMObjects, etc.) Requiring ::from to conjure one up seems
bad.

Since CallbackPromiseAdapter enters a context, ::from could use
v8::Isolate::Current() to fetch it back, but that seems against the purpose of
NewScriptState which wants to abstract V8.

Alternatively, the Web* interface could provide an accessor to the context. But
that should be Blink's domestic affair.

WDYT about simply passing the script state to ::from?

[abarth-chromium, 2014-04-11 23:27:34]

bindings LGTM

[michaeln1, 2014-04-11 23:46:28]

I don't really understand the abstractions in the bindings layer. Maybe you can
help explain the voodoo?

In the CallbackPromiseAdapter ctor...

m_scriptState(NewScriptState::current(toIsolate(context)))


Ok, which 'context' is that?
a) The 'context' of the script that invoked the method that returns a promise?
b) The native 'context' of the object whose promise returning method was called?

https://codereview.chromium.org/236133002/diff/1/Source/bindings/v8/CallbackP...
File Source/bindings/v8/CallbackPromiseAdapter.h (right):

https://codereview.chromium.org/236133002/diff/1/Source/bindings/v8/CallbackP...
Source/bindings/v8/CallbackPromiseAdapter.h:82:
m_resolver->reject(T::from(error));
Since this is a generic template, should it also anticipate that the errorT may
need this treatment too? If its not needed, the ::from function can just ignore
it.

https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.h (right):

https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.h:61: static
PassRefPtr<ServiceWorker> from(NewScriptState*, WebType* worker)
Why is this not a PassRefPtr<NewScriptState>?

[dominicc (has gone to gerrit), 2014-04-12 00:53:31]

On 2014/04/11 23:46:28, michaeln1 wrote:
> I don't really understand the abstractions in the bindings layer. Maybe you
can
> help explain the voodoo?

Sure. To level set: This is just refactoring, it doesn't change behavior. (Hence
no test.)

> In the CallbackPromiseAdapter ctor...
> 
> m_scriptState(NewScriptState::current(toIsolate(context)))
> 
> 
> Ok, which 'context' is that?
> a) The 'context' of the script that invoked the method that returns a promise?
> b) The native 'context' of the object whose promise returning method was
called?

It's whichever context the caller passed. In WebCore::ServiceWorkerContainer's
case it's passing (a). This is the right context for calling back into the
"resolve" callback, but the wrong context for associating the ServiceWorker
argument to that callback in the case the script is reaching across
iframes/windows.

>
https://codereview.chromium.org/236133002/diff/1/Source/bindings/v8/CallbackP...
> File Source/bindings/v8/CallbackPromiseAdapter.h (right):
> 
>
https://codereview.chromium.org/236133002/diff/1/Source/bindings/v8/CallbackP...
> Source/bindings/v8/CallbackPromiseAdapter.h:82:
> m_resolver->reject(T::from(error));
> Since this is a generic template, should it also anticipate that the errorT
may
> need this treatment too? If its not needed, the ::from function can just
ignore
> it.

Good point, I suppose that could happen. REWS took my machine offline :'( so
I'll update this patch on Monday.

>
https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
> File Source/modules/serviceworkers/ServiceWorker.h (right):
> 
>
https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
> Source/modules/serviceworkers/ServiceWorker.h:61: static
> PassRefPtr<ServiceWorker> from(NewScriptState*, WebType* worker)
> Why is this not a PassRefPtr<NewScriptState>?

Because the CallbackPromiseAdapter has a RefPtr to the NewScriptState. You
shouldn't be deleting CallbackPromiseAdapter in ::from because you'd crash when
it tries to invoke the callback in the very next step. It's possible that
CallbackPromiseAdapter should weaken that contract and change its lifetime, but
that's a separate issue.

[haraken, 2014-04-12 02:12:28]

https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.h (right):

https://codereview.chromium.org/236133002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.h:61: static
PassRefPtr<ServiceWorker> from(NewScriptState*, WebType* worker)
On 2014/04/11 23:46:29, michaeln1 wrote:
> Why is this not a PassRefPtr<NewScriptState>?

Because we're not intending to pass an ownership of NewScriptState here. A
general programming rule of NewScriptState is to use NewScriptState* on stack
and store RefPtr<NewScriptState> in an object that wants to keep the
NewScriptState alive while the object is alive.

[haraken, 2014-04-12 02:15:02]

LGTM for bindings/

[dominicc (has gone to gerrit), 2014-04-14 00:20:56]

kinuko: PTAL, since it is Sunday for michaeln.

No feedback from mvanouwerkerk so I may TBR him.

[kinuko, 2014-04-14 02:17:48]

lgtm

[dominicc (has gone to gerrit), 2014-04-14 02:19:17]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-04-14 02:19:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominicc@chromium.org/236133002/20001

[commit-bot: I haz the power, 2014-04-14 02:48:40]

Change committed as 171413

[Michael van Ouwerkerk, 2014-04-28 16:50:42]

On 2014/04/14 02:48:40, I haz the power (commit-bot) wrote:
> Change committed as 171413

lgtm Sorry for the delay, I was away.