ServiceWorker: add ServiceWorkerClients.claim() support (1/3).

ServiceWorker: add ServiceWorkerClients.claim() support (1/3).

This's the Blink side CL for initial ServiceWorkerClients.claim() support.
ServiceWorkerClients passes promise resolver callback to worker client
which will resolve/reject it after Chrome handles it.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#clients-claim

(1/3): This
(2/3): https://codereview.chromium.org/871853002/
(3/3): https://codereview.chromium.org/872593002/

BUG=450106
TEST=https://codereview.chromium.org/872593002/

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

[xiang, 2015-01-23 04:52:42]

xiang.long@intel.com changed reviewers:
+ falken@chromium.org, jsbell@chromium.org, michaeln@chromium.org

[xiang, 2015-01-23 04:55:56]

xiang.long@intel.com changed reviewers:
+ mkwst@chromium.org

[xiang, 2015-01-23 04:57:19]

xiang.long@intel.com changed reviewers:
- mkwst@chromium.org

[xiang, 2015-01-23 04:57:20]

Hi, PTAL, thanks.

[mlamouri (slow - plz ping), 2015-01-23 10:08:06]

mlamouri@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-01-23 10:08:08]

Is there support for the feature from other vendors?

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Clients.idl:14: [CallWith=ScriptState] Promise
claim();
Shouldn't that be being a feature flag?

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:82: return
ScriptPromise();
There are other places where such a check isn't done like just above in
::getAll(). Are those buggy?

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:69: return
createException(InvalidStateError, "The Service Worker operation failed by
invalid state.", webError->message);
nit: the error message doesn't sound correct but I'm no native speaker

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

https://codereview.chromium.org/868463004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerClientsClaimCallbacks.h:14: using
WebServiceWorkerClientsClaimCallbacks = WebCallbacks<void,
WebServiceWorkerError>;
Would that make sense to have that in WebServiceWorkerContextClient.h instead of
creating a new file?

[jsbell, 2015-01-24 00:42:16]

this looks fine to me w/ mounir's comments, but falken should comment on how we
want to expose this

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Clients.idl:14: [CallWith=ScriptState] Promise
claim();
On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> Shouldn't that be being a feature flag?

If it's a non-controversial addition to the core feature set we don't have to go
through the intent/flag process. falken@ should probably make the call (I
haven't been tracking SW spec development as closely)

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:82: return
ScriptPromise();
On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> There are other places where such a check isn't done like just above in
> ::getAll(). Are those buggy?

Per that CL... yes. Any method called via bindings that runs in a worker that
calls back into v8 (e.g. ScriptPromiseResolver::create) could fail. That CL also
explicitly mentions getAll() but didn't put in a fix for it.

It's a hack, but we probably want this here (and in getAll); it will affect many
other APIs available to Workers, but we may notice it more frequently with SWs
since they're killed off aggressively.

xiang: what prompted you to add it here specifically?

I would suggest keeping the early exit, adding it to getAll() as well, but
changing the comment to:

// FIXME: May be null due to worker termination. http://crbug.com/388017

(I've pinged that bug and pointed at this CL too)

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:69: return
createException(InvalidStateError, "The Service Worker operation failed by
invalid state.", webError->message);
On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> nit: the error message doesn't sound correct but I'm no native speaker

+1

Given the text in the specification leading to this error, the message could be
one of:

"The Service Worker is not an active worker."

or:

"The operation failed because the Service Worker is not active."

[jsbell, 2015-01-24 01:15:02]

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:69: return
createException(InvalidStateError, "The Service Worker operation failed by
invalid state.", webError->message);
Ah, since this is just the default message, and the back end will provide the
real message in this failure case, this can be generic, e.g.:

"The Service Worker state was not valid."

... which isn't helpful, but that's fine since no-one should ever see it.

[falken, 2015-01-26 04:55:12]

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/Clients.idl (right):

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/Clients.idl:14: [CallWith=ScriptState] Promise
claim();
On 2015/01/24 00:42:16, jsbell wrote:
> On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> > Shouldn't that be being a feature flag?
> 
> If it's a non-controversial addition to the core feature set we don't have to
go
> through the intent/flag process. falken@ should probably make the call (I
> haven't been tracking SW spec development as closely)

We'll go through the "Intent to Implement and Ship" process. As it's a
functionality that has been in the spec since the beginning (though in a
different form as "replace()") I don't really expect controversy but we'll get
that intent out ASAP. If it fails to gain approval we'll revert before the
branch point.

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:82: return
ScriptPromise();
On 2015/01/24 00:42:16, jsbell wrote:
> On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> > There are other places where such a check isn't done like just above in
> > ::getAll(). Are those buggy?
> 
> Per that CL... yes. Any method called via bindings that runs in a worker that
> calls back into v8 (e.g. ScriptPromiseResolver::create) could fail. That CL
also
> explicitly mentions getAll() but didn't put in a fix for it.
> 
> It's a hack, but we probably want this here (and in getAll); it will affect
many
> other APIs available to Workers, but we may notice it more frequently with SWs
> since they're killed off aggressively.
> 
> xiang: what prompted you to add it here specifically?
> 
> I would suggest keeping the early exit, adding it to getAll() as well, but
> changing the comment to:
> 
> // FIXME: May be null due to worker termination. http://crbug.com/388017
> 
> (I've pinged that bug and pointed at this CL too)

http://crbug.com/413518 has more details, I've just duped 380017 to that. This
is a general problem with worker thread termination. All these checks are just
band-aids to stop from crashing too frequently.

I'm guessing Xiang added it because he was bit by it in skipWaiting:
https://codereview.chromium.org/823583006/

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:69: return
createException(InvalidStateError, "The Service Worker operation failed by
invalid state.", webError->message);
On 2015/01/24 01:15:01, jsbell wrote:
> Ah, since this is just the default message, and the back end will provide the
> real message in this failure case, this can be generic, e.g.:
> 
> "The Service Worker state was not valid."
> 
> ... which isn't helpful, but that's fine since no-one should ever see it.

Right, jsbell's message is good. We've actually historically showed these
generic messages but I'm working on providing more specific messages when
possible.

[xiang, 2015-01-26 06:47:54]

Thanks all for the review & comments, CL updated.

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:82: return
ScriptPromise();
On 2015/01/26 04:55:11, falken wrote:
> On 2015/01/24 00:42:16, jsbell wrote:
> > On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> > > There are other places where such a check isn't done like just above in
> > > ::getAll(). Are those buggy?
> > 
> > Per that CL... yes. Any method called via bindings that runs in a worker
that
> > calls back into v8 (e.g. ScriptPromiseResolver::create) could fail. That CL
> also
> > explicitly mentions getAll() but didn't put in a fix for it.
> > 
> > It's a hack, but we probably want this here (and in getAll); it will affect
> many
> > other APIs available to Workers, but we may notice it more frequently with
SWs
> > since they're killed off aggressively.
> > 
> > xiang: what prompted you to add it here specifically?
> > 
> > I would suggest keeping the early exit, adding it to getAll() as well, but
> > changing the comment to:
> > 
> > // FIXME: May be null due to worker termination. http://crbug.com/388017
> > 
> > (I've pinged that bug and pointed at this CL too)
> 
> http://crbug.com/413518 has more details, I've just duped 380017 to that. This
> is a general problem with worker thread termination. All these checks are just
> band-aids to stop from crashing too frequently.
> 
> I'm guessing Xiang added it because he was bit by it in skipWaiting:
> https://codereview.chromium.org/823583006/

updated the comment.

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

https://codereview.chromium.org/868463004/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerError.cpp:69: return
createException(InvalidStateError, "The Service Worker operation failed by
invalid state.", webError->message);
On 2015/01/26 04:55:11, falken wrote:
> On 2015/01/24 01:15:01, jsbell wrote:
> > Ah, since this is just the default message, and the back end will provide
the
> > real message in this failure case, this can be generic, e.g.:
> > 
> > "The Service Worker state was not valid."
> > 
> > ... which isn't helpful, but that's fine since no-one should ever see it.
> 
> Right, jsbell's message is good. We've actually historically showed these
> generic messages but I'm working on providing more specific messages when
> possible.

Done.

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

https://codereview.chromium.org/868463004/diff/20001/public/platform/WebServi...
public/platform/WebServiceWorkerClientsClaimCallbacks.h:14: using
WebServiceWorkerClientsClaimCallbacks = WebCallbacks<void,
WebServiceWorkerError>;
On 2015/01/23 10:08:08, Mounir Lamouri wrote:
> Would that make sense to have that in WebServiceWorkerContextClient.h instead
of
> creating a new file?

I thought this header will also be used by modules/serviceworker/ which should
not depends on public/web headers, so created a new one and put it to
public/platform.

[mlamouri (slow - plz ping), 2015-01-26 09:10:58]

lgtm

[falken, 2015-01-27 03:25:19]

lgtm 2

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

https://codereview.chromium.org/868463004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:80: // FIXME: May be null
due to worker termination. http://crbug.coma/413518
typo: .coma

[xiang, 2015-01-28 05:50:47]

xiang.long@intel.com changed reviewers:
+ haraken@chromium.org

[xiang, 2015-01-28 05:53:22]

xiang.long@intel.com changed reviewers:
+ mkwst@chromium.org
- haraken@chromium.org

[xiang, 2015-01-28 05:57:49]

Thanks for the review.

mkwst@ would you help to review: Source/web & public/ changes? Thanks.

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

https://codereview.chromium.org/868463004/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerClients.cpp:80: // FIXME: May be null
due to worker termination. http://crbug.coma/413518
On 2015/01/27 03:25:18, falken wrote:
> typo: .coma

Done.

[xiang, 2015-01-29 09:30:58]

ping mkwst@, do you have time to take a look? Thanks.

[Mike West, 2015-01-29 11:50:23]

LGTM, sorry for the delay.

[xiang, 2015-02-02 01:21:06]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2015-02-02 01:21:24]

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

[commit-bot: I haz the power, 2015-02-02 02:34:47]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189313