Re: [w3c/ServiceWorker] Add an option to include frozen documents. (#1442)

jakearchibald requested changes on this pull request.

This is looking much better. A few comments/nits:

> @@ -3465,6 +3496,20 @@ 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="get-client-state-algorithm"><dfn>Get Client State</dfn></h3>
+
+      : Input
+      :: |client|, a [=/service worker client=]
+      : Output
+      :: |state|, a string
+
+      1. Let |state| be {{ClientState/"active"}}.
+      1. If |client|'s [=environment settings object/global object=]'s [=owning document=] is [=frozen=], set |state| to be {{ClientState/"frozen"}}.

Since the client is an [environment](https://html.spec.whatwg.org/multipage/webappapis.html#environment), can you just do "client's responsible document"?

> @@ -3316,6 +3345,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
       1. Set |windowClient|'s [=frame type=] to |frameType|.
       1. Set |windowClient|'s [=visibility state=] to |visibilityState|.
       1. Set |windowClient|'s [=focus state=] to |focusState|.
+      1. Set |windowClient|'s [=state=] to |state|.

This is linking to service worker state rather than client's state.

> @@ -1073,6 +1078,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A {{Client}} object has an associated <dfn id="dfn-service-worker-client-frame-type" for="Client">frame type</dfn>, which is one of "`auxiliary`", "`top-level`", "`nested`", and "`none`". Unless stated otherwise it is "`none`".
 
+    A {{Client}} object has an associated <dfn id="dfn-service-worker-client-state" for="Client">client state</dfn>, which is one of "`active`", and "`frozen`". Unless stated otherwise it is "`active`".

Nit: Rename to "state" since it's already on the client.

> @@ -1113,6 +1120,12 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
             1. Return {{ClientType/"window"}}.
     </section>
 
+    <section>
+      <h4 id="client-state">{{Client/state}}</h4>
+
+      The <dfn attribute for="Client"><code>state</code></dfn> attribute *must* return the <a>context object</a>'s <a>client state</a>.

We're not massively consistent here, but we try to use bikeshed shorthand where possible:

```
The <dfn attribute for="Client">state</dfn> attribute *must* return the [=context object=]'s [=Client/client state=].
```

> @@ -1073,6 +1078,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A {{Client}} object has an associated <dfn id="dfn-service-worker-client-frame-type" for="Client">frame type</dfn>, which is one of "`auxiliary`", "`top-level`", "`nested`", and "`none`". Unless stated otherwise it is "`none`".
 
+    A {{Client}} object has an associated <dfn id="dfn-service-worker-client-state" for="Client">client state</dfn>, which is one of "`active`", and "`frozen`". Unless stated otherwise it is "`active`".

Also, we can say that this is a {{ClientState}} rather than listing the values.

> @@ -1241,6 +1257,13 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
         "all"
       };
     </pre>
+    <pre class="idl" id="client-state-enum">
+      enum ClientState {
+        "active",
+        "frozen",
+        "all"
+      };
+    </pre>

I think we should break this out into multiple enums, since a client can never have a state of "all".

I think enums can inherit, so ClientStateQuery (which adds "all") can inherit from ClientState.

-- 
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/1442#pullrequestreview-261296313