ServiceWorker: Update the install sequence as per the latest spec

ServiceWorker: Update the install sequence as per the latest spec

This change...

(a) updates the install sequence based on the latest spec. Before this
patch, the installing version is set after resolving the register promise.
After this patch, the installing version is set before resolving the promise.

(b) fills registration's version attributes before resolving the register
promise. This should fix the problem mentioned c#5 in the issue and should
absorb the change of the timing to set the installing version due to (a).

(c) adds a separate IPC for updatefound event to adjust the timing to fire
the event. Before this patch, the event shares SetVersionAttributes message.

[1] Blink: https://codereview.chromium.org/524193003/
[2] Chromium: THIS PATCH
[3] Blink: https://codereview.chromium.org/517223002/

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#installation-algorithm


BUG=406240
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=run_webkit_tests.py http/tests/serviceworker/

Committed: https://crrev.com/8148543da0da6062980504f2851afb69e4bdaa4f
Cr-Commit-Position: refs/heads/master@{#292838}

[nhiroki, 2014-08-28 11:55:57]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-08-28 16:20:42]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2014-08-28 16:20:52]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2014-08-28 16:20:59]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2014-08-28 16:25:29]

Patchset #1 (id:80001) has been deleted

[nhiroki, 2014-08-28 16:37:55]

nhiroki@chromium.org changed reviewers:
+ horo@chromium.org, michaeln@chromium.org

[nhiroki, 2014-08-28 16:40:43]

On 2014/08/28 16:37:55, nhiroki wrote:
> mailto:nhiroki@chromium.org changed reviewers:
> + mailto:horo@chromium.org, mailto:michaeln@chromium.org

Hi, would do you give me your early feedback on this CL? I'll create/change
tests tomorrow. Thanks!

[nhiroki, 2014-08-28 16:43:14]

On 2014/08/28 16:40:43, nhiroki wrote:
> On 2014/08/28 16:37:55, nhiroki wrote:
> > mailto:nhiroki@chromium.org changed reviewers:
> > + mailto:horo@chromium.org, mailto:michaeln@chromium.org
> 
> Hi, would do you give me your early feedback on this CL? I'll create/change
> tests tomorrow. Thanks!

would do you -> would you :p

[michaeln, 2014-08-29 00:57:57]

looks pretty good to me

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

https://codereview.chromium.org/517493002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:383:
ChangedVersionAttributesMask mask;
the 'change mask' seems out of place here? what's changed? these structs are not
reflecting a change in state, just the state as it is at this point in time.

https://codereview.chromium.org/517493002/diff/100001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/517493002/diff/100001/content/common/service_...
content/common/service_worker/service_worker_types.h:98: int changed_mask;
the changed_mask seems out of place in this struct? in some of the existing
cases it doesn't apply, i'd vote to keep the mask out of this struct and to pass
it around as a separate value while propagating mutation events.

would it make sense for ServiceWorkerRegistrationObjectInfo to have as a data
member a ServiceWorkerVersionAttributes struct?

[nhiroki, 2014-08-29 08:23:10]

Thanks! Updated and removed WIP mark.
Can you take another look?

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

https://codereview.chromium.org/517493002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:383:
ChangedVersionAttributesMask mask;
On 2014/08/29 00:57:57, michaeln wrote:
> the 'change mask' seems out of place here? what's changed? these structs are
not
> reflecting a change in state, just the state as it is at this point in time.

That makes sense. Removed the mask and made the dispatcher send only attributes.

https://codereview.chromium.org/517493002/diff/100001/content/common/service_...
File content/common/service_worker/service_worker_types.h (right):

https://codereview.chromium.org/517493002/diff/100001/content/common/service_...
content/common/service_worker/service_worker_types.h:98: int changed_mask;
On 2014/08/29 00:57:57, michaeln wrote:
> the changed_mask seems out of place in this struct? in some of the existing
> cases it doesn't apply, i'd vote to keep the mask out of this struct and to
pass
> it around as a separate value while propagating mutation events.

Agree. Reverted the change.

> would it make sense for ServiceWorkerRegistrationObjectInfo to have as a data
> member a ServiceWorkerVersionAttributes struct?

I'm not really sure whether this is useful or not for now because only two
message have SWRegObjectInfo as a member and one of them (ie.
ServiceWorkerMsg_UpdateFound) does not need such version attributes.

I guess getRegistration() may need a new message that has both SWRegObjectInfo
and attributes, so let me revisit when we implement it.

[nhiroki, 2014-08-29 08:25:26]

nhiroki@chromium.org changed reviewers:
+ nasko@chromium.org

[nhiroki, 2014-08-29 08:25:26]

+nasko@: Can you review service_worker_messages.h? Thanks.

[nhiroki, 2014-08-29 09:22:49]

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

https://codereview.chromium.org/517493002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:356:
new_version()->SetStatus(ServiceWorkerVersion::INSTALLING);
fyi: I'm wondering if we should set the worker to 'installing' before calling
ResolvePromise so that we can hide the 'parsed' state in JS. Filled a github
issue:
https://github.com/slightlyoff/ServiceWorker/issues/432

[michaeln, 2014-08-29 20:21:28]

lgtm

[nasko, 2014-08-29 21:17:38]

LGTM with a couple of nits.

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

https://codereview.chromium.org/517493002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:253:
registration->SetInstalling(GetServiceWorker(attrs.installing, true));
nit: Can you point to the CL that changes those methods in the CL description?
The one already mentioned only has changes to layout tests files.

https://codereview.chromium.org/517493002/diff/120001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/517493002/diff/120001/content/common/service_...
content/common/service_worker/service_worker_messages.h:222: // process.
nit: s/process/phase/? process is already used for the "child process" so is the
"installation process" another OS level process or is it a phase of the lifetime
of a service worker?

Also, UpdateFound doesn't sound like installation, rather an update.

[horo, 2014-09-01 02:17:54]

lgtm

[nhiroki, 2014-09-01 03:46:42]

Thank you for reviewing!

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

https://codereview.chromium.org/517493002/diff/120001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher.cc:253:
registration->SetInstalling(GetServiceWorker(attrs.installing, true));
On 2014/08/29 21:17:37, nasko wrote:
> nit: Can you point to the CL that changes those methods in the CL description?
> The one already mentioned only has changes to layout tests files.

Those are existing methods introduced by the different issue from this, so the
blink-side only needs to update layout tests and I think we don't have to
mention it in the description.

https://codereview.chromium.org/517493002/diff/120001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/517493002/diff/120001/content/common/service_...
content/common/service_worker/service_worker_messages.h:222: // process.
On 2014/08/29 21:17:37, nasko wrote:
> nit: s/process/phase/? process is already used for the "child process" so is
the
> "installation process" another OS level process or is it a phase of the
lifetime
> of a service worker?

Good point. I meant the latter. Replaced 'process' with 'phase'.

> Also, UpdateFound doesn't sound like installation, rather an update.

This is a jargon in the ServiceWorker, so I'd like to keep it as is.
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#se...

[nhiroki, 2014-09-01 05:16:59]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-09-01 05:17:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/517493002/140001

[commit-bot: I haz the power, 2014-09-01 06:12:40]

Committed patchset #3 (id:140001) as 68b394f77a844b17a613722926dc56374827c8c1

[commit-bot: I haz the power, 2014-09-10 03:15:44]

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