Upstream service worker `ready` test to WPT

Upstream service worker `ready` test to WPT

This test exists in both WPT and the Chromium project tree. Although the
two implementations assert the same behavior, the Chromium version is
preferable for a number of reasons:

- It uses the `promise_test` function and expresses the test body in
  terms of promises, resulting in improved test readability.
- It includes additional sub-tests

Update the upstream version of the test with these improvements. In
addition:

- remove unnecessary precaution from newly-introduced sub-tests
- remove the dynamically-generated iframe at the conclusion of each
  sub-test

Remove the Chromium-specific version of the test.

BUG=688116
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2880823002
Cr-Commit-Position: refs/heads/master@{#472021}
Committed: https://chromium.googlesource.com/chromium/src/+/981a96bc5df9e0c392193199c5a6d701a973f0f0

[mike3, 2017-05-12 18:54:22]

Hi Matt,

The change set here demonstrates the modifications to the WPT test, but it
obscures the additional alterations that I made. Those are described in the
commit message, but to make it more clear, I've generated a diff between the
old Chromium version of the test and the new WPT version:

https://gist.github.com/jugglinmike/97ee7ef533207b9c985f5e54dff1eb06

I'm particularly interested in your thoughts about my removal of that extra
state management precaution. I think it is unnecessary due to the Unregister
algorithm [1]. Specifically:

> Note: If Try Clear Registration does not trigger Clear Registration here,
> Clear Registration is tried again when the last client using the
> registration is unloaded or the extend lifetime promises for the
> registration’s service workers settle.

Thanks!

[1] https://w3c.github.io/ServiceWorker/#unregister-algorithm

[falken, 2017-05-15 05:48:59]

lgtm

I'm not sure I understand when to use add_cleanup vs add_result_callback vs
add_completion_callback, though you seem to consistently use them for different
things. Can you help me understand?

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html
(right):

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:61:
'controlled document should have a controller');
I think now the spec intends to require JS equality between ServiceWorker
objects representing the same service worker. So we should be able to change
this to:

assert_equals(frame.contentWindow.navigator.serviceWorker.controller,
 registration.active, 'the controller should be the active worker');

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:165:
}, 'ready on an iframe with installing a new service worker by itself');
"with installing" -> "that installs" and remove "by itself"?

[mike3, 2017-05-15 19:48:27]

I've been using `add_cleanup` for synchronous operations and
`add_completion_callback` for asynchronous operations. It doesn't matter
currently since neither approach can handle that use case, but I have hopes to
extend `testharness.js` with support. I've been making that distinction in the
hopes of simplifying future find-and-replace operations.

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html
(right):

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:61:
'controlled document should have a controller');
On 2017/05/15 05:48:59, falken wrote:
> I think now the spec intends to require JS equality between ServiceWorker
> objects representing the same service worker. So we should be able to change
> this to:
> 
> assert_equals(frame.contentWindow.navigator.serviceWorker.controller,
>  registration.active, 'the controller should be the active worker');

I'm not so sure about this. The spec language regarding ServiceWorker object
constancy seems to be limited to the behavior of specific property accessors,
e.g. `navigator.serviceWorker.controller` [1]:

> Note: navigator.serviceWorker.controller returns null if the request is a
> force refresh (shift+refresh). The ServiceWorker objects returned from this
> attribute getter that represent the same service worker are the same objects.

But when it comes to ServiceWorker objects *across* property accessors, I don't
think we have any guarantees. The following tends to discourage assumptions
like that [2]:

> A ServiceWorker object represents a service worker. Each ServiceWorker object
> is associated with a service worker. Multiple separate objects implementing
> the ServiceWorker interface across documents and workers can all be
> associated with the same service worker simultaneously.

...though we're not looking at references "across documents and workers."
Still, I can't find any normative text that supports the invariant, so I'm
inclined to leave this assertion as-is.

[1] https://w3c.github.io/ServiceWorker/#navigator-service-worker-controller
[2] https://w3c.github.io/ServiceWorker/#serviceworker-interface

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:165:
}, 'ready on an iframe with installing a new service worker by itself');
On 2017/05/15 05:48:59, falken wrote:
> "with installing" -> "that installs" and remove "by itself"?

Done.

[-jakearchibald-, 2017-05-15 20:44:55]

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

[-jakearchibald-, 2017-05-15 20:44:56]

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html
(right):

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:61:
'controlled document should have a controller');
I agree that the spec wording isn't great, but equality should work here. I've
filed https://github.com/w3c/ServiceWorker/issues/1140 to try and clear it up.

[mike3, 2017-05-15 21:48:27]

Think this is ready to land, Matt?

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html
(right):

https://codereview.chromium.org/2880823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/ready.https.html:61:
'controlled document should have a controller');
On 2017/05/15 20:44:56, -jakearchibald- wrote:
> I agree that the spec wording isn't great, but equality should work here. I've
> filed https://github.com/w3c/ServiceWorker/issues/1140 to try and clear it up.

Thanks, Jake! I've updated the patch to replace the above assertion with the
assertion that falken requested.

[falken, 2017-05-16 04:05:51]

lgtm, thanks Jake for chiming in!

[falken, 2017-05-16 04:06:03]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2017-05-16 04:06:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 05:39:45]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494907563232120,
"parent_rev": "d411c5d512aa58ebabaf8eb4f6594285d7bea86e", "commit_rev":
"981a96bc5df9e0c392193199c5a6d701a973f0f0"}

[commit-bot: I haz the power, 2017-05-16 05:39:55]

Description was changed from

==========
Upstream service worker `ready` test to WPT

This test exists in both WPT and the Chromium project tree. Although the
two implementations assert the same behavior, the Chromium version is
preferable for a number of reasons:

- It uses the `promise_test` function and expresses the test body in
  terms of promises, resulting in improved test readability.
- It includes additional sub-tests

Update the upstream version of the test with these improvements. In
addition:

- remove unnecessary precaution from newly-introduced sub-tests
- remove the dynamically-generated iframe at the conclusion of each
  sub-test

Remove the Chromium-specific version of the test.

BUG=688116
R=falken@chromium.org
==========

to

==========
Upstream service worker `ready` test to WPT

This test exists in both WPT and the Chromium project tree. Although the
two implementations assert the same behavior, the Chromium version is
preferable for a number of reasons:

- It uses the `promise_test` function and expresses the test body in
  terms of promises, resulting in improved test readability.
- It includes additional sub-tests

Update the upstream version of the test with these improvements. In
addition:

- remove unnecessary precaution from newly-introduced sub-tests
- remove the dynamically-generated iframe at the conclusion of each
  sub-test

Remove the Chromium-specific version of the test.

BUG=688116
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2880823002
Cr-Commit-Position: refs/heads/master@{#472021}
Committed:
https://chromium.googlesource.com/chromium/src/+/981a96bc5df9e0c392193199c5a6...
==========

[commit-bot: I haz the power, 2017-05-16 05:39:56]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/981a96bc5df9e0c392193199c5a6...