Upstream service worker `client` tests to WPT

Upstream service worker `client` tests to WPT

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

Review-Url: https://codereview.chromium.org/2778903002
Cr-Commit-Position: refs/heads/master@{#461000}
Committed: https://chromium.googlesource.com/chromium/src/+/b6b2ff25cd6773cca63208bd50cba3a20b31cfba

[mike3, 2017-03-27 21:34:22]

@falken There is a lot of churn in this patch. My apologies, but I couldn't see
a way around that. Migrating some tests properly involved introducing
assertions that Chromium currently fails. As in prior migration patches, I've
preserved test coverage by retaining copies of these tests in the `chromium/`
directory (each with a note referencing the upstream version).

Here's a report describing how each HTML file moved. Note that the diffs I'm
sharing describe how the file changed *as it was moved from the internal
Chromium test suite*. For tests that already exist in WPT, this is distinct
from how the WPT file changed (Rietveld should be able to provide that view
without trouble).

- `client-id.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? No
  - Migration notes - updating paths and relaxing assertions [1]
- `clients-get-client-types.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
- `clients-get-cross-origin.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? No
  - Migration notes - updating paths
- `clients-get.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? No
  - Migration notes - updating paths
- `clients-matchall-client-types.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? No
  - Migration notes - improving flow; upstream version has additional assertions
- `clients-matchall-include-uncontrolled.html`
  - Migrated? No
  - Retained? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Migration notes - upstream version differs, but only with improvements
- `clients-matchall-on-evaluation.html`
  - Migrated? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Retained? No
  - Migration notes - updating paths
- `clients-matchall.html`
  - Migrated? No
  - Retained? Yes, diff:
https://gist.github.com/jugglinmike/dcb2d39169f070036b83c884dfb6b352#file-cli...
  - Migration notes - Chromium has additional tests, but not upstreaming them
[2]

Notable modifications:

**1. Relaxing of assertions for client ID** As far as I can tell, the client
`id` does *not* have to be a UUID. The spec would seem to be satisfied by any
"opaque string that uniquely identifies the environment.":

https://html.spec.whatwg.org/multipage/webappapis.html#concept-environment-id

(Firefox happens to define them using UUIDs as well, so maybe we could petition
for a spec change. Would you see any value in that?)

I've relaxed the tests that assert the format of the value to instead only
require that they are not identical. If the format is relevant for Chromium's
internals (and if my interpretation is correct), then I think we'll need to add
another test in the `chromium/` directory.

**2. Omitting extra asertions** The extra assertions in Chromium's version of
this file concern the sequence of results in the returned array. WPT has since
received a new test file that is explicitly dedicated to testing this
functionality, and it does so much more thoroughly:
`clients-matchall-order.https.html`

Finally, here is a listing of added/removed test files:

    $ git diff master --stat --relative --stat-width 180 | grep html | grep -v
resources
    external/wpt/service-workers/service-worker/client-id.https.html            
                   | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
   
external/wpt/service-workers/service-worker/clients-get-client-types.https.html 
               | 69
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
   
external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html 
               | 44 ++++++++++++++++++++++------------------
    external/wpt/service-workers/service-worker/clients-get.https.html          
                   | 54 ++++++++++++++++++++++++-------------------------
   
external/wpt/service-workers/service-worker/clients-matchall-client-types.https.html
           | 28 +++++++++++++-------------
   
external/wpt/service-workers/service-worker/clients-matchall-on-evaluation.https.html
          | 24 ++++++++++++++++++++++
    http/tests/serviceworker/chromium/clients-get-client-types.html             
                   | 73
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
    http/tests/serviceworker/chromium/clients-matchall-include-uncontrolled.html
                   | 84
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
    http/tests/serviceworker/chromium/clients-matchall.html                     
                   | 66
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
    http/tests/serviceworker/client-id.html                                     
                   | 63
----------------------------------------------------------
    http/tests/serviceworker/clients-get-client-types.html                      
                   | 69
---------------------------------------------------------------
    http/tests/serviceworker/clients-get-cross-origin.html                      
                   | 48 --------------------------------------------
    http/tests/serviceworker/clients-get.html                                   
                   | 69
---------------------------------------------------------------
    http/tests/serviceworker/clients-matchall-client-types.html                 
                   | 76
---------------------------------------------------------------------
    http/tests/serviceworker/clients-matchall-include-uncontrolled.html         
                   | 80
-------------------------------------------------------------------------
    http/tests/serviceworker/clients-matchall-on-evaluation.html                
                   | 24 ----------------------
    http/tests/serviceworker/clients-matchall.html                              
                   | 62
---------------------------------------------------------

[mike3, 2017-03-27 21:35:41]

Sorry for the formatting issues! The included `git` command ought to produce
equivalent results in your terminal (provided `grep` is also available on your
system).

[falken, 2017-03-28 07:24:39]

Thanks for the detailed review comments.

It's not really from your patch, but it's hard to follow the information being
passed around in these tests: via onfetch or via onmessage, and between the
workers, iframes, and main frame. I think some comments could aid understanding.
WDYT?

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

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/client-id.https.html:40:
// All should be string values
nit: period at end of sentence

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html
(right):

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:14:
promise_test(function(t) {
I think this test could benefit from a high level comment about what it's doing.
It's cumbersome to open the iframe file, the service worker file, and this file
and try to piece it together.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:41:
assert_equals(e.data.result, undefined);
It's hard to tell what this test is trying to test, by reading this. Can we have
comments or assert_equals description?

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:42:
t.done();
This t.done() doesn't look right, because this is a promise_test. I think this
test would end immediately after the postMessage in line 44.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:1:
self.onfetch = function(e) {
Could we make a high-level comment  for what this service worker does?

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:4:
// On navigation, the client id should be null.
These indexOf calls are just to identify a navigation?  Can this use
e.request.mode == 'navigate' instead?

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:6:
e.respondWith(fetch(e.request));
Can just return instead of e.respondWith()

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:16:
var port = e.data.port;
Can we just use e.source.postMessage() instead of passing the port around?

[mike3, 2017-03-28 20:45:09]

> It's not really from your patch, but it's hard to follow the information being
> passed around in these tests: via onfetch or via onmessage, and between the
> workers, iframes, and main frame. I think some comments could aid
understanding.
> WDYT?

I think all your feedback was spot-on. I've been reluctant to make any changes
that were not strictly necessary because I wanted to keep the review process
manageable. (Although one of your requests related to an outright test bug, so
thank you for especially for catching that.)

I've been keeping a running list of "things to follow up with," and it includes
a few of the things you've mentioned. If you don't mind the added complexity of
addressing them as part of this patch, then I'm happy to oblige (except for one
case which I explain below).

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

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/client-id.https.html:40:
// All should be string values
On 2017/03/28 07:24:39, falken wrote:
> nit: period at end of sentence

Acknowledged.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html
(right):

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:14:
promise_test(function(t) {
On 2017/03/28 07:24:39, falken wrote:
> I think this test could benefit from a high level comment about what it's
doing.
> It's cumbersome to open the iframe file, the service worker file, and this
file
> and try to piece it together.

Acknowledged.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:41:
assert_equals(e.data.result, undefined);
On 2017/03/28 07:24:39, falken wrote:
> It's hard to tell what this test is trying to test, by reading this. Can we
have
> comments or assert_equals description?

Agreed. I have also moved the single assertion from the iframe source into this
file. This makes the test file more clear and improves error reporting at the
expense of extra complexity in the parent-to-iframe protocol. I think it's
worth it.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:42:
t.done();
On 2017/03/28 07:24:39, falken wrote:
> This t.done() doesn't look right, because this is a promise_test. I think this
> test would end immediately after the postMessage in line 44.

You are correct: that is a vestage of the WPT version which I neglected to
update. That assertion was still evaluated because the iframe is *also* running
the testing harness, so it produces a number of irrelevant `message` events.
Resolving this required formalizing the postMessage protocol (adding a `type`
attribute to all messages) so that we could wait on the correct message to
detect test completion.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:1:
self.onfetch = function(e) {
On 2017/03/28 07:24:39, falken wrote:
> Could we make a high-level comment  for what this service worker does?

Acknowledged.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:4:
// On navigation, the client id should be null.
On 2017/03/28 07:24:39, falken wrote:
> These indexOf calls are just to identify a navigation?  Can this use
> e.request.mode == 'navigate' instead?

I think we share the same underlying assessment here: that this kind of logic
doesn't belong in a "resource" file that is designed to be shared by many
tests. I've re-factored this event handler to behave in a more consistent
fashion so that it can be more easily reasoned about and maintained.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:6:
e.respondWith(fetch(e.request));
On 2017/03/28 07:24:39, falken wrote:
> Can just return instead of e.respondWith()

Acknowledged.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:16:
var port = e.data.port;
On 2017/03/28 07:24:39, falken wrote:
> Can we just use e.source.postMessage() instead of passing the port around?

We could, but this will likely involve more spanning changes as the client's
global `postMessage` is also being used by the test harness.

The client's global `postMessage` channel is currently being used by the WPT
test harness. This does not preclude its use for this purpose, but it does mean
that the transition will likely involve more spanning changes (since some tests
have been authored under the assumption that no other messages will be
broadcast).

Overall, I think that would be a better practice (both more explicit and
requiring less setup), but because the general pattern may be relevant in other
tests, I'd rather implement it in a follow-up patch to WPT directly. Does that
sound reasonable to you?

[falken, 2017-03-29 01:43:58]

Thanks, this lgtm. I added some comment suggestions and a question about the
'event origin' test.

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
(right):

https://codereview.chromium.org/2778903002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js:16:
var port = e.data.port;
On 2017/03/28 20:45:09, mike3 wrote:
> On 2017/03/28 07:24:39, falken wrote:
> > Can we just use e.source.postMessage() instead of passing the port around?
> 
> We could, but this will likely involve more spanning changes as the client's
> global `postMessage` is also being used by the test harness.
> 
> The client's global `postMessage` channel is currently being used by the WPT
> test harness. This does not preclude its use for this purpose, but it does
mean
> that the transition will likely involve more spanning changes (since some
tests
> have been authored under the assumption that no other messages will be
> broadcast).
> 
> Overall, I think that would be a better practice (both more explicit and
> requiring less setup), but because the general pattern may be relevant in
other
> tests, I'd rather implement it in a follow-up patch to WPT directly. Does that
> sound reasonable to you?

SGTM. FYI A lot of our tests use MessageChannel unnecessarily just because
e.source had not been implemented properly when the tests were created.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html
(right):

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:12:
'resources/clients-get-cross-origin-frame.html';
// The ID of a client from the same origin as us.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:25:
return with_iframe(scope);
// Create a same-origin client and use it to populate |my_origin_client_id|.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:38:
my_origin_client_id = client_id;
// Create a cross-origin client. We'll communicate with this client to test the
cross-origin service worker's behavior.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:42:
add_completion_callback(function() { frame2.remove(); });
I may be missing something, but it looks like the "event origin" is just testing
the cross-domain postMessage API between this page and the iframe. It doesn't
use service worker. If that's the case, I think we can remove this part.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:43:
var requestEventOrigin = new Promise(function(resolve) {
request_event_origin

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:53:
});
// Post the 'getClientId' message to the cross-origin client. The client will
then ask its service worker to look up |my_origin_client_id| via Clients#get.
Since this client ID is for a different origin, we expect the client to not be
found.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:54:
var requestClientId = new Promise(function(resolve) {
nit: snake_case

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:69:
var clientId = message[1];
nit: snake_case

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get.https.html
(right):

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get.https.html:21:
add_completion_callback(function() { frame1.remove(); });
Just FYI (you probably have already noticed): Chrome's test-helpers.js adds this
completion callback by default in with_iframe. Eventually I hope WPT
test-helpers.js can do this too, and we can remove a lot of these lines.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html
(right):

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html:32:
function getEventOrigin(e) {
I believe WPT style is mostly to use snake_case, but I may be wrong.

[mike3, 2017-03-29 15:15:23]

Thanks, @falken!

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html
(right):

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:12:
'resources/clients-get-cross-origin-frame.html';
On 2017/03/29 01:43:58, falken wrote:
> // The ID of a client from the same origin as us.

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:25:
return with_iframe(scope);
On 2017/03/29 01:43:58, falken wrote:
> // Create a same-origin client and use it to populate |my_origin_client_id|.

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:38:
my_origin_client_id = client_id;
On 2017/03/29 01:43:58, falken wrote:
> // Create a cross-origin client. We'll communicate with this client to test
the
> cross-origin service worker's behavior.

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:42:
add_completion_callback(function() { frame2.remove(); });
On 2017/03/29 01:43:58, falken wrote:
> I may be missing something, but it looks like the "event origin" is just
testing
> the cross-domain postMessage API between this page and the iframe. It doesn't
> use service worker. If that's the case, I think we can remove this part.

Ah yeah; you're right. This came from an equivalent assertion in the original
test (expressed from within the iframe):

https://chromium.googlesource.com/chromium/src.git/+/0ca977b/third_party/WebK...

Removing it streamlines things significantly!

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:43:
var requestEventOrigin = new Promise(function(resolve) {
On 2017/03/29 01:43:58, falken wrote:
> request_event_origin

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:53:
});
On 2017/03/29 01:43:58, falken wrote:
> // Post the 'getClientId' message to the cross-origin client. The client will
> then ask its service worker to look up |my_origin_client_id| via Clients#get.
> Since this client ID is for a different origin, we expect the client to not be
> found.

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:54:
var requestClientId = new Promise(function(resolve) {
On 2017/03/29 01:43:58, falken wrote:
> nit: snake_case

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:69:
var clientId = message[1];
On 2017/03/29 01:43:58, falken wrote:
> nit: snake_case

Acknowledged.

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html
(right):

https://codereview.chromium.org/2778903002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html:32:
function getEventOrigin(e) {
On 2017/03/29 01:43:58, falken wrote:
> I believe WPT style is mostly to use snake_case, but I may be wrong.

Got it. Since `getEventOrigin` is out-of-scope, I've removed both of these
function declarations and moved the body of `getClientId` into the `message`
event handler.

[falken, 2017-03-30 08:31:03]

Thanks, still lgtm.

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html
(right):

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:22:
add_completion_callback(function() { registration.unregister(); });
Based on Ben Kelly's email, I guess we should additionally call unregister()
explicitly at the end and so the promise_test doesn't finish until the promise
settles. But this is also probably a cleanup that we'll have to do for many
tests, and there may be an common solution possible by changing the infra, so I
won't block lgtm on this.

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/clients-get-cross-origin.https.html:30:
add_completion_callback(function() { frame1.remove(); });
Similarly, Mozilla seems to prefer we explicitly remove the frames before the
unregister() call occurs.

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html
(right):

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html:35:
}
Is it possible to remove early return? Who is sending this frame messages that
will be ignored?
(No worries if someone is, it'd just be simpler if we can remove this code.)

[mike3, 2017-03-30 23:11:01]

Hi @falken. I ran out of time today, so I only addressed one of your comments.
The others seem optional, but I'd be more than happy to tackle them tomorrow.
I'll leave that up to you: if you'd like to land this as-is, please go right
ahead. Otherwise, I'll see what I can do about that cleanup logic that bkelly
highlighted.

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html
(right):

https://codereview.chromium.org/2778903002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-cross-origin-frame.html:35:
}
On 2017/03/30 08:31:03, falken wrote:
> Is it possible to remove early return? Who is sending this frame messages that
> will be ignored?
> (No worries if someone is, it'd just be simpler if we can remove this code.)

Not required; you caught me being overly-defensive. This isn't application code,
though, so we ought to favor simplicity as you suggest.

[falken, 2017-03-31 01:20:21]

Thanks. Sure, I'll click the commit box.

[falken, 2017-03-31 01:20:27]

The CQ bit was checked by falken@chromium.org

[falken, 2017-03-31 01:20:28]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2778903002/#ps60001
(title: "Remove unnecessary branch")

[commit-bot: I haz the power, 2017-03-31 01:21:22]

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-03-31 02:28:25]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490923227553930,
"parent_rev": "d015b23fea83d7ae0c5aaa687a3c53f82ee91c21", "commit_rev":
"b6b2ff25cd6773cca63208bd50cba3a20b31cfba"}

[commit-bot: I haz the power, 2017-03-31 02:29:19]

Description was changed from

==========
Upstream service worker `client` tests to WPT

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

to

==========
Upstream service worker `client` tests to WPT

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

Review-Url: https://codereview.chromium.org/2778903002
Cr-Commit-Position: refs/heads/master@{#461000}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6b2ff25cd6773cca63208bd50cb...
==========

[commit-bot: I haz the power, 2017-03-31 02:29:20]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b6b2ff25cd6773cca63208bd50cb...