ServiceWorker: Make skipWaiting only works on documents using current registration

ServiceWorker: Make skipWaiting only works on documents using current registration

A document will associate a new matched registration if it is not controlled,
but it doesn't equal to "using" the registration. We should omit documents
that are not using the registration when skipWaiting happens.

BUG=370742
TEST=http/tests/serviceworker/skip-waiting.html

Committed: https://crrev.com/837728521e7cfa757b9e5871423606b2529d08b1
Cr-Commit-Position: refs/heads/master@{#308327}

[xiang, 2014-12-12 04:32:04]

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

[xiang, 2014-12-12 04:33:29]

PTAL, thanks!

[falken, 2014-12-15 02:29:25]

lgtm with nits

Please also indicate in the change description how this is tested.

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:87: if
(!controlling_version_)
I think some explanation here using comments or function names would be good.
The key points are:
1. A client is using a registration if it is controlled by the active worker of
the registration (definition).
2. skippedWaiting alters a registration but does not cause a client to start
using the registration.

Maybe a helper function like ServiceWorkerProviderHost::IsUsingRegistration()
along with a comment?

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:270: //
controller.
nit: This comment probably works better in OnSkippedWaiting itself. It also has
some typos: "Only provider *hosts* *that* are currently using this
*registration* will change the controller".

[xiang, 2014-12-15 06:00:53]

Thanks for the review, CL updated.

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:87: if
(!controlling_version_)
On 2014/12/15 02:29:25, falken wrote:
> I think some explanation here using comments or function names would be good.
> The key points are:
> 1. A client is using a registration if it is controlled by the active worker
of
> the registration (definition).
> 2. skippedWaiting alters a registration but does not cause a client to start
> using the registration.
> 
> Maybe a helper function like ServiceWorkerProviderHost::IsUsingRegistration()
> along with a comment?

I added these comments. I think a IsUsingRegistration() method maybe not proper
here as it is in a transition state, the controlling_version_ is not equal to
registration->active_version().

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_registration.cc:270: //
controller.
On 2014/12/15 02:29:25, falken wrote:
> nit: This comment probably works better in OnSkippedWaiting itself. It also
has
> some typos: "Only provider *hosts* *that* are currently using this
> *registration* will change the controller".

Done.

[falken, 2014-12-15 06:18:01]

Thanks for adding the comments. I still have some feedback.

Also, please don't land until the test mentioned in patch 3/3 codereview is
ready (sorry, I should have caught that before giving l-g-t-m).

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:87: if
(!controlling_version_)
On 2014/12/15 06:00:52, xiang wrote:
> On 2014/12/15 02:29:25, falken wrote:
> > I think some explanation here using comments or function names would be
good.
> > The key points are:
> > 1. A client is using a registration if it is controlled by the active worker
> of
> > the registration (definition).
> > 2. skippedWaiting alters a registration but does not cause a client to start
> > using the registration.
> > 
> > Maybe a helper function like
ServiceWorkerProviderHost::IsUsingRegistration()
> > along with a comment?
> 
> I added these comments. I think a IsUsingRegistration() method maybe not
proper
> here as it is in a transition state, the controlling_version_ is not equal to
> registration->active_version().

My imagination was something like { return
associated_registration_->active_version() == controlling_version_; }

https://codereview.chromium.org/798833002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/798833002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:89: // the
registration.
Since "using" is a normal English word, it would make it more clear that this is
sentence is a definition by saying:
A client is said to be "using" a registration...

[falken, 2014-12-15 06:20:20]

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/798833002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:87: if
(!controlling_version_)
On 2014/12/15 06:18:01, falken wrote:
> On 2014/12/15 06:00:52, xiang wrote:
> > On 2014/12/15 02:29:25, falken wrote:
> > > I think some explanation here using comments or function names would be
> good.
> > > The key points are:
> > > 1. A client is using a registration if it is controlled by the active
worker
> > of
> > > the registration (definition).
> > > 2. skippedWaiting alters a registration but does not cause a client to
start
> > > using the registration.
> > > 
> > > Maybe a helper function like
> ServiceWorkerProviderHost::IsUsingRegistration()
> > > along with a comment?
> > 
> > I added these comments. I think a IsUsingRegistration() method maybe not
> proper
> > here as it is in a transition state, the controlling_version_ is not equal
to
> > registration->active_version().
> 
> My imagination was something like { return
> associated_registration_->active_version() == controlling_version_; }

Ah I totally missed the point of your comment. OK, just comment is fine.

[xiang, 2014-12-15 06:30:04]

> 
>
https://codereview.chromium.org/798833002/diff/20001/content/browser/service_...
> File content/browser/service_worker/service_worker_provider_host.cc (right):
> 
>
https://codereview.chromium.org/798833002/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_provider_host.cc:89: // the
> registration.
> Since "using" is a normal English word, it would make it more clear that this
is
> sentence is a definition by saying:
> A client is said to be "using" a registration...

Done, thanks!
I will land this after that test CL review done.

[xiang, 2014-12-15 08:21:20]

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

[commit-bot: I haz the power, 2014-12-15 08:21:45]

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

[commit-bot: I haz the power, 2014-12-15 09:10:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-15 09:11:47]

Patchset 3 (id:??) landed as
https://crrev.com/837728521e7cfa757b9e5871423606b2529d08b1
Cr-Commit-Position: refs/heads/master@{#308327}

[michaeln, 2014-12-15 23:49:41]

This seems like a step backwards?

Now, even with skip waiting, a page reload (or new nav) is always required in
order to get a newly registered (for the first time) service worker into use.

That seems contrary to the intent of skipWaiting. What do the spec miesters
think about this behavior?

[xiang, 2014-12-16 01:11:59]

On 2014/12/15 23:49:41, michaeln wrote:
> This seems like a step backwards?
> 
> Now, even with skip waiting, a page reload (or new nav) is always required in
> order to get a newly registered (for the first time) service worker into use.
> 
> That seems contrary to the intent of skipWaiting. What do the spec miesters
> think about this behavior?

This CL intends to address https://codereview.chromium.org/765323002/#msg17
comment.
I think it's a decision from spec guys, maybe falken@ knows more rational about
it.

[falken, 2014-12-16 01:27:14]

On 2014/12/16 01:11:59, xiang wrote:
> On 2014/12/15 23:49:41, michaeln wrote:
> > This seems like a step backwards?
> > 
> > Now, even with skip waiting, a page reload (or new nav) is always required
in
> > order to get a newly registered (for the first time) service worker into
use.
> > 
> > That seems contrary to the intent of skipWaiting. What do the spec miesters
> > think about this behavior?
> 
> This CL intends to address https://codereview.chromium.org/765323002/#msg17
> comment.
> I think it's a decision from spec guys, maybe falken@ knows more rational
about
> it.

Yep, this is in agreement with the spec. replace() has been split into a "skip
waiting" part and "take control" part, The take control part is to be spec'd:
https://github.com/slightlyoff/ServiceWorker/issues/586