ServiceWorker: Enable ServiceWorkerRegistration and update layout tests

ServiceWorker: Enable ServiceWorkerRegistration and update layout tests

This enables ServiceWorkerRegistration interface and updates layout tests
to work with it.

Most of layout tests are contributed by horo@

NOTE: ServiceWorker feature is still behind a flag, so this doesn't break
any real web applications.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#service-worker-registration-obj


BUG=396400
TEST=run_webkit_tests.py --debug http/tests/serviceworker

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

[falken, 2014-08-14 04:46:12]

thanks for working on these so quickly

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/registration-stress.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/registration-stress.html:38: gc();
Can you add a comment explaining why we're calling gc()?

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:31:
registrationObservation = internals.observeGC(registration);
observeGC just returns an observer you can use to check if something was gc'd.
But registrationObservation is not used.

When the ActiveDOMObject change for SWRegistration goes in, we'll want a test
for registration gc. Something like:

register().then(function(r) {
  observation = internals.observeGC(r);
  setTimeout(check, 0);
});

function check() {
  gc();
  shouldBeFalse('observation.wasCollected');
}

If you just want to add to this test, you can check
shouldBeFalse('registrationObservation.wasCollected') after the gc() calls
below.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:34: var
obj = {};
This indentation is wrong. Here the function is not an argument, so the body
should be indented just 2 spaces.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:81:
worker.onstatechange = t.step_func(function() {
indentation

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:92: var
obj1 = {};
indentation

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:112:
worker.onstatechange = t.step_func(function() {
indentation

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/interfaces.html (right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/interfaces.html:22: verifyInterface(
as this function is an argument, this should be indented an additional 2 spaces

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:22:
'registration.waiting shuold be null');
typo: "shuold" (and below)

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:27:
registration.addEventListener('updatefound', t.step_func(function() {
this should use wait_for_update()

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:28:
assert_equals(registration.waiting , null,
nit: extra space before ,  (and below)

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:29:
'registration.waiting shuld be null');
typo: "shuld"

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:41:
message = e.data;
nit: var

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:57: //
[BUG] Same scope test doesn't work. updatefound doesn't called.
nit: FIXME, not [BUG] (but please link to a bug if it exists)

[nhiroki, 2014-08-14 04:54:53]

horo@: Can you take a look at falken@'s comments?

[horo, 2014-08-14 07:11:47]

I uploaded in 
https://codereview.chromium.org/468753003/#ps540001

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/registration-stress.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/registration-stress.html:38: gc();
On 2014/08/14 04:46:11, falken wrote:
> Can you add a comment explaining why we're calling gc()?
removed

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:31:
registrationObservation = internals.observeGC(registration);
On 2014/08/14 04:46:11, falken wrote:
> observeGC just returns an observer you can use to check if something was gc'd.
> But registrationObservation is not used.
> 
> When the ActiveDOMObject change for SWRegistration goes in, we'll want a test
> for registration gc. Something like:
> 
> register().then(function(r) {
>   observation = internals.observeGC(r);
>   setTimeout(check, 0);
> });
> 
> function check() {
>   gc();
>   shouldBeFalse('observation.wasCollected');
> }
> 
> If you just want to add to this test, you can check
> shouldBeFalse('registrationObservation.wasCollected') after the gc() calls
> below.

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:34: var
obj = {};
On 2014/08/14 04:46:11, falken wrote:
> This indentation is wrong. Here the function is not an argument, so the body
> should be indented just 2 spaces.

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:81:
worker.onstatechange = t.step_func(function() {
On 2014/08/14 04:46:11, falken wrote:
> indentation

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:92: var
obj1 = {};
On 2014/08/14 04:46:11, falken wrote:
> indentation

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:112:
worker.onstatechange = t.step_func(function() {
On 2014/08/14 04:46:11, falken wrote:
> indentation

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/interfaces.html (right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/interfaces.html:22: verifyInterface(
On 2014/08/14 04:46:11, falken wrote:
> as this function is an argument, this should be indented an additional 2
spaces

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:22:
'registration.waiting shuold be null');
On 2014/08/14 04:46:12, falken wrote:
> typo: "shuold" (and below)

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:27:
registration.addEventListener('updatefound', t.step_func(function() {
On 2014/08/14 04:46:12, falken wrote:
> this should use wait_for_update()

We can't test (registration.waiting/active == null) with wait_for_update().

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:28:
assert_equals(registration.waiting , null,
On 2014/08/14 04:46:12, falken wrote:
> nit: extra space before ,  (and below)

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:29:
'registration.waiting shuld be null');
On 2014/08/14 04:46:12, falken wrote:
> typo: "shuld"

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:41:
message = e.data;
On 2014/08/14 04:46:12, falken wrote:
> nit: var

Done.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:57: //
[BUG] Same scope test doesn't work. updatefound doesn't called.
On 2014/08/14 04:46:12, falken wrote:
> nit: FIXME, not [BUG] (but please link to a bug if it exists)

removed.

[nhiroki, 2014-08-14 07:29:38]

Applied horo@'s patchset 28!

[nhiroki, 2014-08-14 08:04:05]

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:58: //
[BUG] Same scope test doesn't work. updatefound doesn't called.
This would be intended behavior. unregister() could be cancelled by following
register() and a registration could be reused. In the case, updatefound was
already fired.

As horo-san did, I'd like to disable this for now and rewrite it later.

[falken, 2014-08-14 10:16:37]

This basically looks good.

I don't like how the gc test is now asserting nothing gets deleted. Let me split
it in two tests which are more discriminating.

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:27:
registration.addEventListener('updatefound', t.step_func(function() {
On 2014/08/14 07:11:47, horo wrote:
> On 2014/08/14 04:46:12, falken wrote:
> > this should use wait_for_update()
> 
> We can't test (registration.waiting/active == null) with wait_for_update().

You can save registration in a global:

var registration;
register().then(function(r) {
    registration = r; 
    assert_equals(registration.waiting, null);
    ...
    return wait_for_update();
  })
.then(function(worker) {
    assert_equals(registration.waiting, null);
    ...
  })

Our tests prefer that pattern over indentation marching.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:71:
shouldBeFalse('swObservation.wasCollected');
This test has gotten pretty silly :(  It really makes no sense for someone
reading it who doesn't know the history.

Actually our code resets registration properties, but does so just after
dispatching the statechange event for redundant. Unfortunately, I don't see a
good way to wait for a change to the registration properties.

I think we should have two test files. One that shows that registration is not
gc()'d ever and one that shows the service worker is gc()'d once registration
properties are reset (manually). I'll create these tests.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html:17:
resolve(registration.installing);
this promise is just wait_for_update()?

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/test-helpers.js:63: return new
Promise(test.step_func(function(resolve, reject) {
you can omit reject as it's not used

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:33:
assert_true(registration.installing != null,
assert_not_equals

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:58: //
[BUG] Same scope test doesn't work. updatefound doesn't called.
On 2014/08/14 08:04:04, nhiroki wrote:
> This would be intended behavior. unregister() could be cancelled by following
> register() and a registration could be reused. In the case, updatefound was
> already fired.
> 
> As horo-san did, I'd like to disable this for now and rewrite it later.

I'm a bit confused. What should this FIXME say?

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-controller.html (left):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-controller.html:72:
async_test(function(t) {
Why is this removed?

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister.html:16: }, 'Unregistering out
of origin');
The indents in this file are all off, I think it predates our style guide. I'd
either fix them all or none.

[nhiroki, 2014-08-14 12:17:34]

jfyi: I just uploaded multiple-register.html and unregister-then-register.html,
and now there are all tests to be fixed on this CL.

[falken, 2014-08-14 13:13:37]

I think we're stuck with service-worker-gc.html in its current form. So let's
just improve the comments for now.

My idea to clear the registration manually doesn't work because it's readonly,
and my idea to register a new worker doesn't work because of crbug.com/398355

For reference, the latter idea is at: https://codereview.chromium.org/476743002/

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:5:
description('Test that a registered Service Worker with an event handler is not
garbage collected prematurely');
replace with 'Test that ServiceWorker and ServiceWorkerRegistration are not
garbage collected prematurely'

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:68: {
add comment:
    // The worker is 'redundant' but the registration holds a reference to it,
    // so it shouldn't be collected yet.
    // FIXME: When crbug.com/398355 is fixed, register a new script URL and
    // once the new worker is activated, check that the old worker is gc'd.

[nhiroki, 2014-08-14 13:22:08]

Thanks! Updated tests and removed WIP mark!!

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/120001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:27:
registration.addEventListener('updatefound', t.step_func(function() {
On 2014/08/14 10:16:36, falken wrote:
> On 2014/08/14 07:11:47, horo wrote:
> > On 2014/08/14 04:46:12, falken wrote:
> > > this should use wait_for_update()
> > 
> > We can't test (registration.waiting/active == null) with wait_for_update().
> 
> You can save registration in a global:
> 
> var registration;
> register().then(function(r) {
>     registration = r; 
>     assert_equals(registration.waiting, null);
>     ...
>     return wait_for_update();
>   })
> .then(function(worker) {
>     assert_equals(registration.waiting, null);
>     ...
>   })
> 
> Our tests prefer that pattern over indentation marching.
>   

Done.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:71:
shouldBeFalse('swObservation.wasCollected');
On 2014/08/14 10:16:36, falken wrote:
> This test has gotten pretty silly :(  It really makes no sense for someone
> reading it who doesn't know the history.
> 
> Actually our code resets registration properties, but does so just after
> dispatching the statechange event for redundant. Unfortunately, I don't see a
> good way to wait for a change to the registration properties.
> 
> I think we should have two test files. One that shows that registration is not
> gc()'d ever and one that shows the service worker is gc()'d once registration
> properties are reset (manually). I'll create these tests.

Acknowledged.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html:17:
resolve(registration.installing);
On 2014/08/14 10:16:36, falken wrote:
> this promise is just wait_for_update()?

I think we cannot call wait_for_update() here since there is no 'test' object.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/test-helpers.js (right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/test-helpers.js:63: return new
Promise(test.step_func(function(resolve, reject) {
On 2014/08/14 10:16:36, falken wrote:
> you can omit reject as it's not used

Done.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:33:
assert_true(registration.installing != null,
On 2014/08/14 10:16:36, falken wrote:
> assert_not_equals

Comparing to the resolved worker would work. Fixed.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:58: //
[BUG] Same scope test doesn't work. updatefound doesn't called.
On 2014/08/14 10:16:37, falken wrote:
> On 2014/08/14 08:04:04, nhiroki wrote:
> > This would be intended behavior. unregister() could be cancelled by
following
> > register() and a registration could be reused. In the case, updatefound was
> > already fired.
> > 
> > As horo-san did, I'd like to disable this for now and rewrite it later.
> 
> I'm a bit confused. What should this FIXME say?

Hmmm... apparently this test is now working. Okay, enabled this.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-controller.html (left):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-controller.html:72:
async_test(function(t) {
On 2014/08/14 10:16:37, falken wrote:
> Why is this removed?

Maybe wrongly removed. Recalled.

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister.html (right):

https://codereview.chromium.org/466723002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister.html:16: }, 'Unregistering out
of origin');
On 2014/08/14 10:16:37, falken wrote:
> The indents in this file are all off, I think it predates our style guide. I'd
> either fix them all or none.

Done.

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html
(right):

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:5:
description('Test that a registered Service Worker with an event handler is not
garbage collected prematurely');
On 2014/08/14 13:13:36, falken wrote:
> replace with 'Test that ServiceWorker and ServiceWorkerRegistration are not
> garbage collected prematurely'

Done.

https://codereview.chromium.org/466723002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/service-worker-gc.html:68: {
On 2014/08/14 13:13:36, falken wrote:
> add comment:
>     // The worker is 'redundant' but the registration holds a reference to it,
>     // so it shouldn't be collected yet.
>     // FIXME: When crbug.com/398355 is fixed, register a new script URL and
>     // once the new worker is activated, check that the old worker is gc'd.

Done.

[falken, 2014-08-14 14:23:15]

lgtm with style nits

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/registration-stress.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/registration-stress.html:14: var
number_of_workers = 50;
nit: this probably makes more sense as number_of_registrations now

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-body-stream.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-body-stream.html:10: t,
'resources/fetch-body-stream-worker.js', scope)
i think you indent an additional 2 space here

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch.html:10: t,
'resources/fetch-worker.js', scope)
i think this is wrapped, so it should be indented an additional 2

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:11:
.then(function(registration) {
the .then chain should be indented an additional 2 spaces

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:40:
obj.synced,
additional 2 spaces (and throughout this file)

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/multiple-register.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/multiple-register.html:15:
previous_registration = registration;
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/multiple-register.html:18:
.then(function(registered_worker) {
nit: this is archaic naming now. installing_worker is better

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:10: t,
'resources/postmessage-msgport-to-client-worker.js', scope)
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:42:
result, expected,
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage-to-client.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-to-client.html:10: t,
'resources/postmessage-to-client-worker.js', scope)
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage.html:10: t,
'resources/postmessage-worker.js', scope)
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/registration-end-to-end.html:26:
.then(t.step_func(function(registration) {
t.step_func can be removed from a promise chain

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html:15:
return new Promise(function(resolve, reject) {
reject can be omitted

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:8: var
registration;
can you move this down to be the same scope as expectedScope, options;

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:12: var
expectedScope, options;
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:40:
messageChannel.port1.onmessage = t.step_func(function(e) {
Would it be better to promisify this?

  return new Promise(function(resolve) {
      var messageChannel = new MessageChannel();
      messageChannel.port1.onmessage = resolve;
      worker.postMessage();
    });
.then(function(e) {
  var message = e.data;

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-controller.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-controller.html:53: return
wait_for_update(t, registration);
indent

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-then-register.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-then-register.html:88: var
sw_container = iframe.contentWindow.navigator.serviceWorker;
now this var isn't so useful

either move this variable just above where it's used (and probably just make it
"var controller" and set to the controller), or remove it entirely

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/waiting.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/waiting.html:31: var container =
frame.contentWindow.navigator.serviceWorker;
probably better to remove or make it "var controller = ...controller"

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

Thank you for reviewing!

Although there are some remaining mismatches with the style guide, let me keep
them as is for now. I'll fix them later.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/chromium/registration-stress.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/chromium/registration-stress.html:14: var
number_of_workers = 50;
On 2014/08/14 14:23:14, falken wrote:
> nit: this probably makes more sense as number_of_registrations now

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch-body-stream.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch-body-stream.html:10: t,
'resources/fetch-body-stream-worker.js', scope)
On 2014/08/14 14:23:14, falken wrote:
> i think you indent an additional 2 space here

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/fetch.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/fetch.html:10: t,
'resources/fetch-worker.js', scope)
On 2014/08/14 14:23:14, falken wrote:
> i think this is wrapped, so it should be indented an additional 2

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:11:
.then(function(registration) {
On 2014/08/14 14:23:14, falken wrote:
> the .then chain should be indented an additional 2 spaces

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/install-phase-event-waituntil.html:40:
obj.synced,
On 2014/08/14 14:23:14, falken wrote:
> additional 2 spaces (and throughout this file)

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/multiple-register.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/multiple-register.html:15:
previous_registration = registration;
On 2014/08/14 14:23:14, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/multiple-register.html:18:
.then(function(registered_worker) {
On 2014/08/14 14:23:14, falken wrote:
> nit: this is archaic naming now. installing_worker is better

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:10: t,
'resources/postmessage-msgport-to-client-worker.js', scope)
On 2014/08/14 14:23:14, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:42:
result, expected,
On 2014/08/14 14:23:14, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage-to-client.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage-to-client.html:10: t,
'resources/postmessage-to-client-worker.js', scope)
On 2014/08/14 14:23:15, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/postmessage.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/postmessage.html:10: t,
'resources/postmessage-worker.js', scope)
On 2014/08/14 14:23:15, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/registration-end-to-end.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/registration-end-to-end.html:26:
.then(t.step_func(function(registration) {
On 2014/08/14 14:23:15, falken wrote:
> t.step_func can be removed from a promise chain

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/fetch-mixed-content-iframe.html:15:
return new Promise(function(resolve, reject) {
On 2014/08/14 14:23:15, falken wrote:
> reject can be omitted

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html
(right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:8: var
registration;
On 2014/08/14 14:23:15, falken wrote:
> can you move this down to be the same scope as expectedScope, options;

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:12: var
expectedScope, options;
On 2014/08/14 14:23:15, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/serviceworkerglobalscope-scope.html:40:
messageChannel.port1.onmessage = t.step_func(function(e) {
On 2014/08/14 14:23:15, falken wrote:
> Would it be better to promisify this?
> 
>   return new Promise(function(resolve) {
>       var messageChannel = new MessageChannel();
>       messageChannel.port1.onmessage = resolve;
>       worker.postMessage();
>     });
> .then(function(e) {
>   var message = e.data;
> 
> 

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-controller.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-controller.html:53: return
wait_for_update(t, registration);
On 2014/08/14 14:23:15, falken wrote:
> indent

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/unregister-then-register.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/unregister-then-register.html:88: var
sw_container = iframe.contentWindow.navigator.serviceWorker;
On 2014/08/14 14:23:15, falken wrote:
> now this var isn't so useful
> 
> either move this variable just above where it's used (and probably just make
it
> "var controller" and set to the controller), or remove it entirely

Done.

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/waiting.html (right):

https://codereview.chromium.org/466723002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/waiting.html:31: var container =
frame.contentWindow.navigator.serviceWorker;
On 2014/08/14 14:23:15, falken wrote:
> probably better to remove or make it "var controller = ...controller"

Done.

[nhiroki, 2014-08-14 23:04:13]

Hi tkent@, can you review public/platform/WebServiceWorkerProvider.h as an
OWNER? Thanks.

[tkent, 2014-08-14 23:14:54]

https://codereview.chromium.org/466723002/diff/260001/public/platform/WebServ...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/466723002/diff/260001/public/platform/WebServ...
public/platform/WebServiceWorkerProvider.h:56: #ifdef
DISABLE_SERVICE_WORKER_REGISTRATION
Don't you remove this block?

[nhiroki, 2014-08-14 23:28:21]

Thank you. Updated.

https://codereview.chromium.org/466723002/diff/260001/public/platform/WebServ...
File public/platform/WebServiceWorkerProvider.h (right):

https://codereview.chromium.org/466723002/diff/260001/public/platform/WebServ...
public/platform/WebServiceWorkerProvider.h:56: #ifdef
DISABLE_SERVICE_WORKER_REGISTRATION
On 2014/08/14 23:14:54, tkent wrote:
> Don't you remove this block?

Removed such a block from here and elsewhere.

[tkent, 2014-08-14 23:31:08]

lgtm

[nhiroki, 2014-08-14 23:37:58]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-14 23:39:37]

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

[commit-bot: I haz the power, 2014-08-14 23:39:59]

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

[commit-bot: I haz the power, 2014-08-14 23:40:00]

Failed to apply patch for
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
  Hunk #1 FAILED at 4.
  1 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html.rej

Patch:      
LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
Index: LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
diff --git
a/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
b/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
index
0f2fd5a3706a7812bec20a8a964f6f3bd7f3e8bb..9d4816488bdeb7001c09c3af9a7b6f1de8a1746e
100644
--- a/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
+++ b/LayoutTests/http/tests/serviceworker/postmessage-msgport-to-client.html
@@ -4,26 +4,23 @@
 <script src="../resources/testharnessreport.js"></script>
 <script src="resources/test-helpers.js"></script>
 <script>
-var t = async_test('postMessage MessagePorts from ServiceWorker to Client');
-t.step(function() {
-    var scope = 'resources/blank.html'
+async_test(function(t) {
+    var scope = 'resources/blank.html';
     service_worker_unregister_and_register(
-        t, 'resources/postmessage-msgport-to-client-worker.js',
scope).then(t.step_func(onRegister));
-
-    function onRegister(worker) {
-        worker.addEventListener('statechange', t.step_func(function(event) {
-            if (event.target.state == 'activated')
-                onActive();
-        }));
-    }
-
-    function onActive() {
-        with_iframe(scope, t.step_func(function(frame) {
-            var w = frame.contentWindow;
-            w.onmessage = t.step_func(onMessage);
-            w.navigator.serviceWorker.controller.postMessage('ping');
-        }));
-    }
+        t, 'resources/postmessage-msgport-to-client-worker.js', scope)
+      .then(function(registration) {
+          return wait_for_update(t, registration);
+        })
+      .then(function(sw) {
+          return wait_for_state(t, sw, 'activated');
+        })
+      .then(function() { return with_iframe(scope); })
+      .then(function(frame) {
+          var w = frame.contentWindow;
+          w.onmessage = t.step_func(onMessage);
+          w.navigator.serviceWorker.controller.postMessage('ping');
+        })
+      .catch(unreached_rejection(t));
 
     var result = [];
     var expected = [
@@ -32,20 +29,22 @@ t.step(function() {
     ];
 
     function onMessage(e) {
-        var message = e.data;
-        if ('port' in message) {
-            var port = message.port;
-            port.postMessage({value: 1});
-            port.postMessage({value: 2});
-            port.postMessage({done: true});
-        } else if ('ack' in message) {
-            result.push(message.ack);
-        } else if ('done' in message) {
-            assert_array_equals(result, expected, 'Worker should post back
expected values via MessagePort.');
-            service_worker_unregister_and_done(t, scope);
-        } else {
-            assert_unreached('Got unexpected message from ServiceWorker');
-        }
+      var message = e.data;
+      if ('port' in message) {
+        var port = message.port;
+        port.postMessage({value: 1});
+        port.postMessage({value: 2});
+        port.postMessage({done: true});
+      } else if ('ack' in message) {
+        result.push(message.ack);
+      } else if ('done' in message) {
+        assert_array_equals(
+            result, expected,
+            'Worker should post back expected values via MessagePort.');
+        service_worker_unregister_and_done(t, scope);
+      } else {
+        assert_unreached('Got unexpected message from ServiceWorker');
+      }
     }
-});
+  }, 'postMessage MessagePorts from ServiceWorker to Client');
 </script>

[nhiroki, 2014-08-15 00:20:20]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-15 00:21:09]

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

[commit-bot: I haz the power, 2014-08-15 02:05:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[falken, 2014-08-15 02:15:22]

On 2014/08/15 00:20:20, nhiroki wrote:
> The CQ bit was checked by mailto:nhiroki@chromium.org

It's a bit late, but the CL description should credit Horo-san as well.

[commit-bot: I haz the power, 2014-08-15 02:32:06]

Committed patchset #9 (300001) as 180317

[nhiroki, 2014-08-15 02:57:32]

On 2014/08/15 02:15:22, falken wrote:
> On 2014/08/15 00:20:20, nhiroki wrote:
> > The CQ bit was checked by mailto:nhiroki@chromium.org
> 
> It's a bit late, but the CL description should credit Horo-san as well.

Oops... you're right! I'm really sorry, horo-san. I just updated the CL
description, it won't affect the commit log though...