Re: [w3c/ServiceWorker] Maintain timing for worker start/ready, for navigation requests. (#1575)

@jakearchibald commented on this pull request.

Thanks for the PR, and sorry for the delay reviewing. @mfalken and I took a look.

It isn't clear to me how these values will be read. I'm slightly worried that there's one service worker, therefore one "start time" and one "ready time", but many requests. It feels like "start time" and "ready time" might be overwritten by another request in a race.

Maybe these values should instead be associated with the response, since the timing is from the perspective of the request/response. But I'm just guessing, because I'm not sure how these values will be used.

For instance, if the service worker was started a year ago, and never closed for some reason, should "worker start" be the time a year ago, or should it be the time a given request decided it needed a service worker?

> @@ -2971,6 +2975,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

       1. Let |preloadResponse| be a new [=promise=].
       1. Let |workerRealm| be null.
       1. Let |eventHandled| be null.
+      1. Let |crossOriginIsolatedCapability| be |request|'s [=request/client=]'s [=environment settings object/cross-origin isolated capability=].

Has someone familiar with isolation looked at this? Two timings are taken, once from "in parallel", and another from the service worker thread. Neither of these are the request's client, so does the request's client's cross-origin isolated capability apply here?

> @@ -3020,6 +3025,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

           1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
           1. Return null.
       1. If |activeWorker|'s <a>state</a> is "`activating`", wait for |activeWorker|'s <a>state</a> to become "`activated`".
+      1. Let |workerStartTime| be the  [=coarsened shared current time=] given <var>crossOriginIsolatedCapability</var>.

```suggestion
      1. Let |workerStartTime| be the [=coarsened shared current time=] given |crossOriginIsolatedCapability|.
```
Nit

> @@ -3020,6 +3025,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

           1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
           1. Return null.
       1. If |activeWorker|'s <a>state</a> is "`activating`", wait for |activeWorker|'s <a>state</a> to become "`activated`".
+      1. Let |workerStartTime| be the  [=coarsened shared current time=] given <var>crossOriginIsolatedCapability</var>.

The previous step (`If |activeWorker|'s <a>state</a> is "activating"`) probably should be included in this measurement, since we've decided we need a service worker, but it might not be ready. If the site is seeing long delays due to activation delays, that feels important.

> @@ -3035,6 +3041,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe

               1. If |request| is a <a>non-subresource request</a> and |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, initialize |e|'s {{FetchEvent/resultingClientId}} attribute to |reservedClient|'s [=environment/id=], and to the empty string otherwise.
               1. If |request| is a <a>navigation request</a>, initialize |e|'s {{FetchEvent/replacesClientId}} attribute to |request|'s [=request/replaces client id=], and to the empty string otherwise.
               1. Initialize |e|’s {{FetchEvent/handled}} to |eventHandled|.
+              1. If |request| is a <a>navigation request</a>, then:

```suggestion
              1. If |request| is a [=navigation request=], then:
```

I realise we're not consistent with the syntax, but we're trying to use shorthand as we make changes.

-- 
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/1575#pullrequestreview-632198982