Upstream service worker `client` tests to WPT

third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js

Index: third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
diff --git a/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js b/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
index 9ac2c22645897ad1f15c19170b635cb4b3db077c..4705e358d889055014889e38db094dfce0277e8d 100644
--- a/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
+++ b/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/clients-get-worker.js
@@ -1,5 +1,7 @@
 self.onfetch = function(e) {

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

Could we make a high-level comment  for what this service worker does?

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

Acknowledged.

-  if (e.request.url.indexOf("clients-get-frame.html") >= 0) {
+  if (e.request.url.indexOf('clients-get-frame.html') >= 0 ||
+      e.request.url.indexOf('clients-get-client-types') >= 0) {
+    // On navigation, the client id should be null.

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

These indexOf calls are just to identify a navigation?  Can this use
e.request.mode == 'navigate' instead?

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

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.

     if (e.clientId === null) {
       e.respondWith(fetch(e.request));

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

Can just return instead of e.respondWith()

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

Acknowledged.

     } else {
@@ -12,42 +14,30 @@ self.onfetch = function(e) {
 
 self.onmessage = function(e) {
   var port = e.data.port;

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

Can we just use e.source.postMessage() instead of passing the port around?

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

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]

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

-  if (e.data.message == 'get_client_ids') {
-    var clientIds = e.data.clientIds;
-    var message = [];
+  var client_ids = e.data.clientIds;
+  var message = [];
 
-    Promise.all(
-        clientIds.map(function(clientId) {
-          return self.clients.get(clientId);
-        }).concat(self.clients.get("invalid-id"))
-      ).then(function(clients) {
-        clients.forEach(function(client) {
-            if (client instanceof Client) {
-              message.push([client.visibilityState,
-                            client.focused,
-                            client.url,
-                            client.frameType]);
-            } else {
-              message.push(client);
-            }
-          });
-        port.postMessage(message);
-      });
-  } else if (e.data.message == 'get_other_client_id') {
-    var clientId = e.data.clientId;
-    var message;
-
-    self.clients.get(clientId)
-      .then(function(client) {
-          if (client instanceof Client) {
-            message = [client.visibilityState,
-                       client.focused,
-                       client.url,
-                       client.frameType];
+  e.waitUntil(Promise.all(
+      client_ids.map(function(client_id) {
+          return self.clients.get(client_id);
+        }))
+      .then(function(clients) {
+          // No matching client for a given id or a matched client is off-origin
+          // from the service worker.
+          if (clients.length == 1 && clients[0] == undefined) {
+            port.postMessage(clients[0]);
           } else {
-            message = client;
+            clients.forEach(function(client) {
+                if (client instanceof Client) {
+                  message.push([client.visibilityState,
+                                client.focused,
+                                client.url,
+                                client.frameType]);
+                } else {
+                  message.push(client);
+                }
+              });
+            port.postMessage(message);
           }
-          port.postMessage(message);
-        });
-  }
+        }));
 };