ServiceWorker: Wait for registration promise to resolve before changing states.

ServiceWorker: Wait for registration promise to resolve before changing states.

V8 Promise resolving is done as an async task. So resolving the
ServiceWorker registration promise followed by installing and activating
the worker on the browser side could result in an activated service
worker visible in the JavaScript "then" method, although the worker is
expected to have not yet begun installation at that point.

This patch teaches ServiceWorker to wait for the registration promise to
resolve before processing state changes from the browser.

This patch won't have an effect until the Chromium side change also lands.

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

[falken, 2014-04-23 09:44:38]

kinuko, yhirano: can you please review?

Chromium-side change: https://codereview.chromium.org/249143002/

[kinuko, 2014-04-23 14:20:00]

ServiceWorker related part lgtm

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:104: {
Can  you also add 'FIXME: deprecate' comment here? (As this one looks confusing)

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:150: {
nit: maybe ASSERT(m_isPromisePending) here

https://codereview.chromium.org/247263010/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerProxy.h (right):

https://codereview.chromium.org/247263010/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProxy.h:21: // FIXME(falken): Remove this after
Chrome change rolls in.
nit: no ldap needed for FIXME in blink

[haraken, 2014-04-23 14:35:29]

bindings/ LGTM.

[falken, 2014-04-24 00:58:49]

Thanks! +tkent: Can you review public/ please?

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:104: {
On 2014/04/23 14:20:00, kinuko wrote:
> Can  you also add 'FIXME: deprecate' comment here? (As this one looks
confusing)

Done.

https://codereview.chromium.org/247263010/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:150: {
On 2014/04/23 14:20:00, kinuko wrote:
> nit: maybe ASSERT(m_isPromisePending) here

Done.

https://codereview.chromium.org/247263010/diff/20001/public/platform/WebServi...
File public/platform/WebServiceWorkerProxy.h (right):

https://codereview.chromium.org/247263010/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerProxy.h:21: // FIXME(falken): Remove this after
Chrome change rolls in.
On 2014/04/23 14:20:00, kinuko wrote:
> nit: no ldap needed for FIXME in blink

Done.

[tkent, 2014-04-24 01:24:02]

lgtm

[yhirano, 2014-04-24 01:53:49]

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:156: }
Shouldn't we clear the queue?

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:163: ThenFunction::create(this,
ThenFunction::Rejected));
You don't have to pass the second parameter. It doesn't do anything, does it?

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:102: static
v8::Handle<v8::Value> from(ScriptPromiseResolverWithContext* resolver,
WebServiceWorker* worker)
If it's used in PromiseCallbackAdapter, I think we need HandleScope there.
Instead, you can return a V8UndefinedType instance.

[falken, 2014-04-24 02:38:41]

Thanks for the review! Can you take another look?

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:156: }
On 2014/04/24 01:53:49, yhirano wrote:
> Shouldn't we clear the queue?

Oh course! Thanks

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:163: ThenFunction::create(this,
ThenFunction::Rejected));
On 2014/04/24 01:53:49, yhirano wrote:
> You don't have to pass the second parameter. It doesn't do anything, does it?

I didn't realize we could do that. Thanks

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/247263010/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:102: static
v8::Handle<v8::Value> from(ScriptPromiseResolverWithContext* resolver,
WebServiceWorker* worker)
On 2014/04/24 01:53:49, yhirano wrote:
> If it's used in PromiseCallbackAdapter, I think we need HandleScope there.
> Instead, you can return a V8UndefinedType instance.

Done. Much simpler.

[yhirano, 2014-04-24 03:19:13]

lgtm

[falken, 2014-04-24 05:48:56]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-04-24 05:49:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/247263010/80001

[commit-bot: I haz the power, 2014-04-24 06:21:48]

Change committed as 172459

[michaeln1, 2014-04-24 21:15:07]

https://codereview.chromium.org/247263010/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/247263010/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorker.h:57: virtual void
setState(blink::WebServiceWorkerState) { }
This webkit api is kinda convoluted? The proxy has an onStateChanged(st) method
to inform blink, this class has both a setter for blink to inform chromium, and
a getter for blink to query chromium. Hmmm...

If all you're looking at is this api, you'd be left wondering wth is up with
this, who/what exactly is the purveyor of truth regarding this 'state' junk?

What if this was rearranged a little? Here's a mental model. The blink-side can
be ready for state change events, but initially it might not be ready. When its
not ready, the chrome-side will defer making state changes (and instead queue
them up), so WebServiceWorker::state() will not mutate and
WebServiceWorkerProxy::dispatchStateChangeEvent() will not be called. When the
blink-side becomes ready, the chrome-side flushes the queued up changes by
updating its internal state and calling dispatchStateChangeEvent() for each
queue change.

So a webkit api that looks closer to...

// State changes are queued up until the blink-side is ready for them.
bool WebServiceWorkerProxy::isReady()

// Lets the chromium-side know that the blink-side has become ready or
// perhaps toggled back to not ready.
bool WebServiceWorker::proxyReadyChanged();

[falken, 2014-04-25 01:13:10]

https://codereview.chromium.org/247263010/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/247263010/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorker.h:57: virtual void
setState(blink::WebServiceWorkerState) { }
On 2014/04/24 21:15:08, michaeln1 wrote:
> This webkit api is kinda convoluted? The proxy has an onStateChanged(st)
method
> to inform blink, this class has both a setter for blink to inform chromium,
and
> a getter for blink to query chromium. Hmmm...
> 
> If all you're looking at is this api, you'd be left wondering wth is up with
> this, who/what exactly is the purveyor of truth regarding this 'state' junk?
> 
> What if this was rearranged a little? Here's a mental model. The blink-side
can
> be ready for state change events, but initially it might not be ready. When
its
> not ready, the chrome-side will defer making state changes (and instead queue
> them up), so WebServiceWorker::state() will not mutate and
> WebServiceWorkerProxy::dispatchStateChangeEvent() will not be called. When the
> blink-side becomes ready, the chrome-side flushes the queued up changes by
> updating its internal state and calling dispatchStateChangeEvent() for each
> queue change.
> 
> So a webkit api that looks closer to...
> 
> // State changes are queued up until the blink-side is ready for them.
> bool WebServiceWorkerProxy::isReady()
> 
> // Lets the chromium-side know that the blink-side has become ready or
> // perhaps toggled back to not ready.
> bool WebServiceWorker::proxyReadyChanged();

Good idea, things would be clearer this way and I think it should work. I'll
take a stab at it and see how it turns out.

[michaeln, 2014-04-25 01:32:20]

> Good idea, things would be clearer this way and I think it should work. I'll
> take a stab at it and see how it turns out.

Wunderbar :)

[kinuko, 2014-04-25 04:24:45]

Yeah that sounds nicer.

On Fri, Apr 25, 2014 at 10:32 AM, <michaeln@chromium.org> wrote:

> Good idea, things would be clearer this way and I think it should work.
>> I'll
>> take a stab at it and see how it turns out.
>>
>
> Wunderbar :)
>
> https://codereview.chromium.org/247263010/
>

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