Re: [w3c/ServiceWorker] Clients.get: block on reserved clients. (#1323)

jakearchibald requested changes on this pull request.

My comment are mostly nits. This is looking good!

> @@ -222,6 +222,13 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A <dfn export id="dfn-service-worker-client" for="">service worker client</dfn> is an [=environment=].
 
+    A [=/service worker client=] has an associated <dfn export>discarded flag</dfn>. It is initially unset.
+
+   This specification defines the following [=environment discarding steps=] for a [=/service worker client=] |client|:

I think this should be written

```
Each [=/service worker client=] has the following [=environment discarding steps=]:
```

This avoids "this specification defines" which is a little redundant, because everything normative in this spec is something this spec defines.

> @@ -222,6 +222,13 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A <dfn export id="dfn-service-worker-client" for="">service worker client</dfn> is an [=environment=].
 
+    A [=/service worker client=] has an associated <dfn export>discarded flag</dfn>. It is initially unset.
+
+   This specification defines the following [=environment discarding steps=] for a [=/service worker client=] |client|:
+        1. Set |client|'s [=discarded flag=].
+
+    Note: Implementations can essentially discard clients whose [=discarded flag=] is set.

Nit: lose "essentially". Also, is this defined somewhere?

> @@ -3216,6 +3204,41 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
           Note: When an exception is [=throw|thrown=], the implementation does undo (roll back) any changes made to the cache storage during the batch operation job.
   </section>
+
+  <section algorithm>
+    <h3 id="resolve-get-client-promise-algorithm"><dfn>Resolve Get Client Promise</dfn></h3>
+
+      : Input
+      :: |client|, a [=/service worker client=]
+      :: |promise|, a <a>promise</a>
+
+      : Output
+      :: none
+
+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.

For consistency, can we go with `[=secure context=]`?

> @@ -3216,6 +3204,41 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
           Note: When an exception is [=throw|thrown=], the implementation does undo (roll back) any changes made to the cache storage during the batch operation job.
   </section>
+
+  <section algorithm>
+    <h3 id="resolve-get-client-promise-algorithm"><dfn>Resolve Get Client Promise</dfn></h3>
+
+      : Input
+      :: |client|, a [=/service worker client=]
+      :: |promise|, a <a>promise</a>
+
+      : Output
+      :: none
+
+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. Else:
+          1. If |client|’s <a>creation URL</a> is not a <a>potentially trustworthy URL</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.

Same here, can we use `[=term=]` instead of `<a>term</a>`?

> +  <section algorithm>
+    <h3 id="resolve-get-client-promise-algorithm"><dfn>Resolve Get Client Promise</dfn></h3>
+
+      : Input
+      :: |client|, a [=/service worker client=]
+      :: |promise|, a <a>promise</a>
+
+      : Output
+      :: none
+
+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. Else:
+          1. If |client|’s <a>creation URL</a> is not a <a>potentially trustworthy URL</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. If |client| is an [=environment settings object=] and is not a [=window client=], then:
+          1. Let |clientObject| be the result of running <a>Create Client</a> algorithm with |client| as the argument.

`[=Create Client=]`

> +  <section algorithm>
+    <h3 id="resolve-get-client-promise-algorithm"><dfn>Resolve Get Client Promise</dfn></h3>
+
+      : Input
+      :: |client|, a [=/service worker client=]
+      :: |promise|, a <a>promise</a>
+
+      : Output
+      :: none
+
+      1. If |client| is an [=environment settings object=], then:
+          1. If |client| is not a <a>secure context</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. Else:
+          1. If |client|’s <a>creation URL</a> is not a <a>potentially trustworthy URL</a>, reject |promise| with a "{{SecurityError}}" {{DOMException}} and abort these steps.
+      1. If |client| is an [=environment settings object=] and is not a [=window client=], then:
+          1. Let |clientObject| be the result of running <a>Create Client</a> algorithm with |client| as the argument.

There are a few more in this algo too.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1323#pullrequestreview-127977412