ServiceWorker: Add support for .skipWaiting and controllerchange event(2/3)

ServiceWorker: Add support for .skipWaiting and controllerchange event(2/3)

The ServiceWorkerScriptContext will handle .skipWaiting request by
sending an IPC message to browser process. The message will be handled
by ServiceWorkerVersion.

Currently we will only affect controllees using the same registration of
the worker.

Spec:
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-global-scope-skipwaiting
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container-controllerchange-event

(1/3): https://codereview.chromium.org/723923002/
(2/3): This
(3/3): https://codereview.chromium.org/765323002/

BUG=370742
Committed: https://crrev.com/979efbd4faa3adc0454cb7b27c16665af4e0204c
Cr-Commit-Position: refs/heads/master@{#307862}

[xiang, 2014-11-13 07:30:18]

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

[xiang, 2014-11-13 07:35:10]

PTAL, thanks!

Currently there's a little difference between the impl and the spec.
The spec says: "If service worker's state is installed, then:
Run Activate algorithm, or its equivalent, passing service worker's registration
as the argument."
The impl does: if the  worker state is installed, before we run Activate
algorithm, we also do the steps described in Install algorigthm 9.1. Which means
pages controlled by different registrations will also be handled in this case.

[michaeln, 2014-11-14 02:05:11]

i'll take a closer look, but wanted to get this comment back first...

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:271:
exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
I don't believe this is the desired behavior. What you're calling
"exiting_version" could very well be a version from a completely different
registration. Other registrations and pages controlled by them should not be
affected.

Nothing in the steps for 4.1.7 suggests the behavior you're implementing. And
the non-normlative language sayz its purpose is to promote to "active even while
clients are using the registration".

[falken, 2014-11-14 04:00:15]

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:262:
host->active_version();
You can add "1. Let exitingWorker be the active worker that controls client." to
help the reader find this place in the spec.

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:271:
exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
On 2014/11/14 02:05:11, michaeln wrote:
> I don't believe this is the desired behavior. What you're calling
> "exiting_version" could very well be a version from a completely different
> registration. Other registrations and pages controlled by them should not be
> affected.
> 
> Nothing in the steps for 4.1.7 suggests the behavior you're implementing. And
> the non-normlative language sayz its purpose is to promote to "active even
while
> clients are using the registration".

Right I think this can happen if this registration's pattern matches the
document, but another registration has a longer pattern match. So you have to
skip if existing_version's registration is not us.

I think this is a bug in the spec, which just says: "For each service worker
client client whose client url matches registration's scope url". Could you open
a spec bug?

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:105: // Activate
version immediately. Used when skip waiting flag is set.
The second sentence is a little misleading: it's also called by
ActivateWaitingVersionWhenReady when skip waiting is not set.

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:761: if
(running_status() == STARTING || running_status() == RUNNING)
I'm curious in what circumstances can this occur? Does it make sense to use
ServiceWorker::SendMessage instead, which will start the worker if it's not
running?

[xiang, 2014-11-14 13:17:28]

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:271:
exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
On 2014/11/14 04:00:15, falken wrote:
> On 2014/11/14 02:05:11, michaeln wrote:
> > I don't believe this is the desired behavior. What you're calling
> > "exiting_version" could very well be a version from a completely different
> > registration. Other registrations and pages controlled by them should not be
> > affected.
> > 
> > Nothing in the steps for 4.1.7 suggests the behavior you're implementing.
And
> > the non-normlative language sayz its purpose is to promote to "active even
> while
> > clients are using the registration".
> 
> Right I think this can happen if this registration's pattern matches the
> document, but another registration has a longer pattern match. So you have to
> skip if existing_version's registration is not us.
> 
> I think this is a bug in the spec, which just says: "For each service worker
> client client whose client url matches registration's scope url". Could you
open
> a spec bug?

Yes, a longer matched registration should not be affected.

However there are still cases that the new "skip-waiting" registration is a
subset of existing one. We should replace these clients' controllers even they
are controlled by different registrations. 

I have open a spec issue and made a proposal for this problem:
https://github.com/slightlyoff/ServiceWorker/issues/561

Please take a look, thanks.

[michaeln, 2014-11-14 23:06:39]

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:271:
exiting_version->SetStatus(ServiceWorkerVersion::REDUNDANT);
> Yes, a longer matched registration should not be affected.
> 
> However there are still cases that the new "skip-waiting" registration is a
> subset of existing one. We should replace these clients' controllers even they
> are controlled by different registrations. 
> 
> I have open a spec issue and made a proposal for this problem:
> https://github.com/slightlyoff/ServiceWorker/issues/561
> 
> Please take a look, thanks.

Hmmm, section 7.9 for the [[Install]] algo is obviously incorrect and i think
overly complicated.

> We should replace these clients' controllers even they are controlled by
different registrations

I don't think there's a need for that complexity. The primary purpose is to
force updates into use w/o having to reload (afaik). Here are 2 alternative
options that satisfy that primary purpose and avoid the complexity of mutating
other registrations and their clients.

1. SkipWating only affets pages that are controlled by an older version of the
same registration. So it's constrained to only apply to updates and not initial
installs.

2. SkipWaiting affects pages that are either controlled by an older version of
the same registration (as above) OR are not controlled by any serviceworker.
This variant allows 'skipwaiting' to jump into use upon initial installations
too.

Of these two options, I think #2 is the better choice.
 
In our impl, only those hosts already associated with the registration at the
time skipWaiting() is called would possibly be affected. This method doesn't
need to enumerate hosts. It can NotifyObervers of OnSkippedWaiting() having
happened.

Wdyt?

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:43:
ServiceWorkerRegistration* registration) {}
virtual void OnSkippedWaiting(reg) ?

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:135: for (size_t i = 0;
i < pending_skip_waiting_requests_.size(); ++i)
good place for (int request_id : pending_skip_waiting_requests_)

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:757:
registration->ActivateWaitingVersion();
I don't think ActivateWaitingVersion expects to be re-entered. If
!pending_skip_waiting_requests.emtpy(), just push this id on the back.

[xiang, 2014-11-17 08:42:10]

Thanks for the review, before
https://github.com/slightlyoff/ServiceWorker/issues/561 has a conclusion I
changed the CL to follow #2 solution.

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.h (right):

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:43:
ServiceWorkerRegistration* registration) {}
On 2014/11/14 23:06:39, michaeln wrote:
> virtual void OnSkippedWaiting(reg) ?

Done.

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_registration.h:105: // Activate
version immediately. Used when skip waiting flag is set.
On 2014/11/14 04:00:15, falken wrote:
> The second sentence is a little misleading: it's also called by
> ActivateWaitingVersionWhenReady when skip waiting is not set.

Done.

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

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:135: for (size_t i = 0;
i < pending_skip_waiting_requests_.size(); ++i)
On 2014/11/14 23:06:39, michaeln wrote:
> good place for (int request_id : pending_skip_waiting_requests_)

Done.

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:757:
registration->ActivateWaitingVersion();
On 2014/11/14 23:06:39, michaeln wrote:
> I don't think ActivateWaitingVersion expects to be re-entered. If
> !pending_skip_waiting_requests.emtpy(), just push this id on the back.

Done. Thanks!

https://codereview.chromium.org/717353004/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:761: if
(running_status() == STARTING || running_status() == RUNNING)
On 2014/11/14 04:00:15, falken wrote:
> I'm curious in what circumstances can this occur? Does it make sense to use
> ServiceWorker::SendMessage instead, which will start the worker if it's not
> running?

When .skipWaiting called in worker script parsing stage, the status will be
STARTING, we don't want to re-start the worker in this case.

And I feel this acknowledge message is not like other event messages and should
only send back when work is still alive.

[falken, 2014-11-19 04:19:56]

looking pretty good

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

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:60:
CHECK_EQ(associated_registration_.get(), registration);
seems DCHECK_EQ is OK here

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:66: AsWeakPtr(),
waiting_version));
It's subtle that OnSkippedWaiting is called just before the skip to ACTIVE, so
it waits for any status change then sets controller. We won't detect an error if
the status change is to REDUNDANT for example.

Can ServiceWorkerRegistration instead call OnSkippepWaiting after setting the
version to ACTIVATING?

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:214:
CHECK(should_activate_when_ready_);
why not DCHECK?

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:246: // "8. Fire a
simple event named controllerchange..."
these line numbers are now out of the sync with the spec

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:757:
registration->ActivateWaitingVersionWhenReady();
this seems to depend on the activation to happen asynchronously, so the
request_id is pushed on before the skip happens. it may be more straightforward
to push on first, then call ActivateWaitingVersionWhenReady

https://codereview.chromium.org/717353004/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/717353004/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:200: // Asks the browser
to force this worker become activated.
nit: "to become activated"

[xiang, 2014-11-24 07:04:01]

Thanks for the review, patch updated.
Btw, what's your idea about the use case:
https://github.com/slightlyoff/ServiceWorker/issues/561/#issuecomment-63409739?

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

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:60:
CHECK_EQ(associated_registration_.get(), registration);
On 2014/11/19 04:19:56, falken wrote:
> seems DCHECK_EQ is OK here

Done.

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:66: AsWeakPtr(),
waiting_version));
On 2014/11/19 04:19:56, falken wrote:
> It's subtle that OnSkippedWaiting is called just before the skip to ACTIVE, so
> it waits for any status change then sets controller. We won't detect an error
if
> the status change is to REDUNDANT for example.
> 
> Can ServiceWorkerRegistration instead call OnSkippepWaiting after setting the
> version to ACTIVATING?

Yes, you are right, we can move it after ACTIVATING if only support current
registration replacing. Thanks!

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:214:
CHECK(should_activate_when_ready_);
On 2014/11/19 04:19:56, falken wrote:
> why not DCHECK?

Done.

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:246: // "8. Fire a
simple event named controllerchange..."
On 2014/11/19 04:19:56, falken wrote:
> these line numbers are now out of the sync with the spec

Done.

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/717353004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:757:
registration->ActivateWaitingVersionWhenReady();
On 2014/11/19 04:19:56, falken wrote:
> this seems to depend on the activation to happen asynchronously, so the
> request_id is pushed on before the skip happens. it may be more
straightforward
> to push on first, then call ActivateWaitingVersionWhenReady

Done.

https://codereview.chromium.org/717353004/diff/40001/content/common/service_w...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/717353004/diff/40001/content/common/service_w...
content/common/service_worker/service_worker_messages.h:200: // Asks the browser
to force this worker become activated.
On 2014/11/19 04:19:56, falken wrote:
> nit: "to become activated"

Done.

[falken, 2014-11-25 09:25:02]

I think this looks good, but please address michaeln's comment on the Blink-side
CL.

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:243: // "9. Fire a
simple event named controllerchange..."
It's not too clear how Step 9 is implemented, can you add a small comment about
that?

[falken, 2014-11-25 15:50:46]

> Btw, what's your idea about the use case:
>
https://github.com/slightlyoff/ServiceWorker/issues/561/#issuecomment-63409739?

Forgot to respond to this. I think the use case makes sense, but let's just see
how the spec discussion evolves and track it.

[michaeln, 2014-11-25 23:50:22]

i like how this is shaping up too

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

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:90:
dispatcher_host_->Send(new ServiceWorkerMsg_SetControllerServiceWorker(
we could add a param to the msg and method whether the 'controllerchanged' event
should be raised

[falken, 2014-11-27 03:40:40]

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:226:
DCHECK(!exiting_version->HasControllee());
Can't this DCHECK fail in the skip waiting scenario?

[xiang, 2014-11-28 08:01:40]

Patchset #5 (id:80001) has been deleted

[xiang, 2014-11-28 08:04:26]

Thanks for the review! CL updated, PTAL.

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

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.cc:90:
dispatcher_host_->Send(new ServiceWorkerMsg_SetControllerServiceWorker(
On 2014/11/25 23:50:22, michaeln wrote:
> we could add a param to the msg and method whether the 'controllerchanged'
event
> should be raised

Done.

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:226:
DCHECK(!exiting_version->HasControllee());
On 2014/11/27 03:40:40, falken wrote:
> Can't this DCHECK fail in the skip waiting scenario?

Done.

https://codereview.chromium.org/717353004/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_registration.cc:243: // "9. Fire a
simple event named controllerchange..."
On 2014/11/25 09:25:02, falken wrote:
> It's not too clear how Step 9 is implemented, can you add a small comment
about
> that?

Done.

[falken, 2014-12-01 02:29:59]

looks good, I think it needs to be a three-sided patch though, something like:
(1) blink Web interface changes, (2) chrome impl, (3) blink tests

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:92:
should_notify_controllerchange = true;
nit: bool should_notify_controllerchange = previous_version.get() && version &&
version->skip_waiting(); is less verbose

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (left):

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:201:
DCHECK_EQ(active_version(), version);
I guess this DCHECK can fail now if OnNoControllees gets called by the worker
that was replaced by a skip waiting worker? It would seem in that case we
shouldn't proceed with the rest of the function. Should we instead early-return
if version != active_version()?

https://codereview.chromium.org/717353004/diff/100001/content/child/service_w...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/717353004/diff/100001/content/child/service_w...
content/child/service_worker/web_service_worker_provider_impl.cc:78:
context_->controller()->info(), false), false);
nit: add /* shouldNotifyControllerChange */ next to the false or else use a bool
variable

[xiang, 2014-12-01 07:20:07]

CL updated, PTAL, thanks!

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:92:
should_notify_controllerchange = true;
On 2014/12/01 02:29:59, falken wrote:
> nit: bool should_notify_controllerchange = previous_version.get() && version
&&
> version->skip_waiting(); is less verbose

Done.

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (left):

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:201:
DCHECK_EQ(active_version(), version);
On 2014/12/01 02:29:59, falken wrote:
> I guess this DCHECK can fail now if OnNoControllees gets called by the worker
> that was replaced by a skip waiting worker? It would seem in that case we
> shouldn't proceed with the rest of the function. Should we instead
early-return
> if version != active_version()?

Thanks for point this out, I think we can remove the listener in
SetVersionInternal(), how do you think?

https://codereview.chromium.org/717353004/diff/100001/content/child/service_w...
File content/child/service_worker/web_service_worker_provider_impl.cc (right):

https://codereview.chromium.org/717353004/diff/100001/content/child/service_w...
content/child/service_worker/web_service_worker_provider_impl.cc:78:
context_->controller()->info(), false), false);
On 2014/12/01 02:29:59, falken wrote:
> nit: add /* shouldNotifyControllerChange */ next to the false or else use a
bool
> variable

Done.

[falken, 2014-12-01 07:27:52]

Please remove "When there are controllees using different registrations, we
should also
make these workers redundant before assign the new controller." from the CL
description, as that's no longer the case, right?

lgtm

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (left):

https://codereview.chromium.org/717353004/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:201:
DCHECK_EQ(active_version(), version);
On 2014/12/01 07:20:07, xiang wrote:
> On 2014/12/01 02:29:59, falken wrote:
> > I guess this DCHECK can fail now if OnNoControllees gets called by the
worker
> > that was replaced by a skip waiting worker? It would seem in that case we
> > shouldn't proceed with the rest of the function. Should we instead
> early-return
> > if version != active_version()?
> 
> Thanks for point this out, I think we can remove the listener in
> SetVersionInternal(), how do you think?

Looks right, nice idea.

[michaeln, 2014-12-01 23:43:15]

fwiw, lgtm 2

[xiang, 2014-12-03 23:52:00]

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

[commit-bot: I haz the power, 2014-12-03 23:53:22]

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

[commit-bot: I haz the power, 2014-12-03 23:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-03 23:58:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiang, 2014-12-04 00:07:59]

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

[xiang, 2014-12-04 00:08:35]

dcheng@ would you please review the IPC change? thanks.

[dcheng, 2014-12-04 00:17:09]

I need to take a more in-depth look, but here are some immediate thoughts.

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:91:
previous_version.get() && version && version->skip_waiting();
previous_version && ...

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:120:
scoped_refptr<ServiceWorkerVersion>* data_member,
o.O

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:128: if
(data_member->get() && data_member->get() == active_version_.get())
if (data_member && *data_member == active_version_)

https://codereview.chromium.org/717353004/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_message_filter.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_message_filter.cc:115: bool
should_notify_controllerchange) {
Why is this bool unused?

[xiang, 2014-12-04 07:51:37]

Would you please take a look again? Thanks!

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:91:
previous_version.get() && version && version->skip_waiting();
On 2014/12/04 00:17:09, dcheng wrote:
> previous_version && ...

Done.

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:128: if
(data_member->get() && data_member->get() == active_version_.get())
On 2014/12/04 00:17:09, dcheng wrote:
> if (data_member && *data_member == active_version_)

seems it should be "if (*data_member && *data_member == active_version_)"

https://codereview.chromium.org/717353004/diff/120001/content/child/service_w...
File content/child/service_worker/service_worker_message_filter.cc (right):

https://codereview.chromium.org/717353004/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_message_filter.cc:115: bool
should_notify_controllerchange) {
On 2014/12/04 00:17:09, dcheng wrote:
> Why is this bool unused?

This method is to handle stale message from browser process, so the bool
argument is not used.

[xiang, 2014-12-09 05:43:07]

dcheng@ ping, PTAL, thanks!

[xiang, 2014-12-10 06:03:14]

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

[xiang, 2014-12-10 06:03:58]

On 2014/12/09 05:43:07, xiang wrote:
> dcheng@ ping, PTAL, thanks!

mkwst@ would you help to review the IPC part? thanks.

[dcheng, 2014-12-10 21:20:54]

ipc changes lgtm. sorry for the delay.

[xiang, 2014-12-11 02:11:18]

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

[commit-bot: I haz the power, 2014-12-11 02:13:06]

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

[xiang, 2014-12-11 02:26:18]

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

[xiang, 2014-12-11 03:05:05]

Patchset #8 (id:160001) has been deleted

[xiang, 2014-12-11 03:11:14]

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

[commit-bot: I haz the power, 2014-12-11 03:12:29]

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

[commit-bot: I haz the power, 2014-12-11 04:17:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-11 04:17:19]

Failed to apply patch for
content/browser/service_worker/service_worker_version.cc:
While running git apply --index -3 -p1;
  error: patch failed:
content/browser/service_worker/service_worker_version.cc:648
  Falling back to three-way merge...
  Applied patch to 'content/browser/service_worker/service_worker_version.cc'
with conflicts.
  U content/browser/service_worker/service_worker_version.cc

Patch:       content/browser/service_worker/service_worker_version.cc
Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc
b/content/browser/service_worker/service_worker_version.cc
index
5ad3c4ebac6480802c534e30ae416c07e893bb38..7bbda82943b0aa01fbbc58a153aff0ecd6a71531
100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -113,6 +113,7 @@ ServiceWorkerVersion::ServiceWorkerVersion(
       context_(context),
       script_cache_map_(this, context),
       is_doomed_(false),
+      skip_waiting_(false),
       weak_factory_(this) {
   DCHECK(context_);
   DCHECK(registration);
@@ -142,6 +143,12 @@ void ServiceWorkerVersion::SetStatus(Status status) {
 
   status_ = status;
 
+  if (skip_waiting_ && status_ == ACTIVATED) {
+    for (int request_id : pending_skip_waiting_requests_)
+      DidSkipWaiting(request_id);
+    pending_skip_waiting_requests_.clear();
+  }
+
   std::vector<base::Closure> callbacks;
   callbacks.swap(status_change_callbacks_);
   for (const auto& callback : callbacks)
@@ -648,6 +655,8 @@ bool ServiceWorkerVersion::OnMessageReceived(const
IPC::Message& message) {
                         OnPostMessageToDocument)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_FocusClient,
                         OnFocusClient)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_SkipWaiting,
+                        OnSkipWaiting)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -879,6 +888,27 @@ void ServiceWorkerVersion::OnFocusClientFinished(int
request_id, bool result) {
       request_id, result));
 }
 
+void ServiceWorkerVersion::OnSkipWaiting(int request_id) {
+  skip_waiting_ = true;
+  if (status_ != INSTALLED)
+    return DidSkipWaiting(request_id);
+
+  if (!context_)
+    return;
+  ServiceWorkerRegistration* registration =
+      context_->GetLiveRegistration(registration_id_);
+  if (!registration)
+    return;
+  pending_skip_waiting_requests_.push_back(request_id);
+  if (pending_skip_waiting_requests_.size() == 1)
+    registration->ActivateWaitingVersionWhenReady();
+}
+
+void ServiceWorkerVersion::DidSkipWaiting(int request_id) {
+  if (running_status() == STARTING || running_status() == RUNNING)
+    embedded_worker_->SendMessage(ServiceWorkerMsg_DidSkipWaiting(request_id));
+}
+
 void ServiceWorkerVersion::ScheduleStopWorker() {
   if (running_status() != RUNNING)
     return;

[commit-bot: I haz the power, 2014-12-11 04:17:40]

The CQ bit was unchecked by commit-bot@chromium.org

[xiang, 2014-12-11 04:21:10]

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

[commit-bot: I haz the power, 2014-12-11 04:22:31]

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

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

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

[commit-bot: I haz the power, 2014-12-11 04:23:06]

The CQ bit was unchecked by commit-bot@chromium.org

[xiang, 2014-12-11 04:28:23]

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

[commit-bot: I haz the power, 2014-12-11 04:29:26]

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

[commit-bot: I haz the power, 2014-12-11 05:24:30]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2014-12-11 05:25:58]

Patchset 9 (id:??) landed as
https://crrev.com/979efbd4faa3adc0454cb7b27c16665af4e0204c
Cr-Commit-Position: refs/heads/master@{#307862}