Re: [w3c/ServiceWorker] Improve Activate with Try Activate (#1065)

jakearchibald requested changes on this pull request.



> @@ -168,6 +168,8 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
     A [=/service worker=] has an associated <dfn export id="dfn-set-of-event-types-to-handle">set of event types to handle</dfn> (a [=ordered set|set=]) whose [=item=] is an <a>event listener</a>'s event type. It is initially an empty set.
 
+    A [=/service worker=] has an associated <dfn export id="dfn-set-of-extended-events">set of extended events</dfn> (a [=ordered set|set=]) whose [=item=] is an [=event=]. It is initially an empty set.

Should `[=event=]` be `{{ExtendableEvent}}`?

> @@ -364,6 +366,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
             1. Else, let it be initialized to a new {{Client}} object that represents the worker associated with |incumbentGlobal|.
             1. Let the {{ExtendableMessageEvent/ports}} attribute of |e| be initialized to |newPorts|.
             1. <a>Dispatch</a> |e| at |destination|.
+            1. Invoke [=Update Service Worker Extended Events Set=] with |serviceWorker| and |e|.

So is this something every service worker event needs to do? Can we do this automatically when extendable events are created? Worried about every spec remembering to do this.

> @@ -2775,6 +2782,18 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
   </section>
 
   <section algorithm>
+    <h3 id="try-activate-algorithm"><dfn>Try Activate</dfn></h3>
+
+      : Input
+      :: |registration|, a [=/service worker registration=]
+      : Output
+      :: None
+
+      1. Assert: |registration|'s [=waiting worker=] is not null.

This is called when promises passed to `event.waitUntil` settle, which can happen when there isn't a waiting worker, so isn't it common for the assertion to fail?

> @@ -2742,10 +2752,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
       1. If |registration|'s <a>waiting worker</a> is null, abort these steps.
       1. Let |redundantWorker| be null.
-      1. If |registration|'s <a>active worker</a> is not null, then:
-          1. Set |redundantWorker| to |registration|'s <a>active worker</a>.
-          1. Wait for |redundantWorker| to finish handling any in-progress requests.
-          1. <a lt="Terminate Service Worker">Terminate</a> |redundantWorker|.
+      1. If |registration|'s [=active worker=] is not null, |redundantWorker| be |registration|'s [=active worker=].

nit: Missing let/set before `|redundantWorker|`?

> @@ -2742,10 +2752,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
 
       1. If |registration|'s <a>waiting worker</a> is null, abort these steps.
       1. Let |redundantWorker| be null.
-      1. If |registration|'s <a>active worker</a> is not null, then:
-          1. Set |redundantWorker| to |registration|'s <a>active worker</a>.
-          1. Wait for |redundantWorker| to finish handling any in-progress requests.
-          1. <a lt="Terminate Service Worker">Terminate</a> |redundantWorker|.

When are these workers terminated now?

> @@ -3100,6 +3122,21 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
   </section>
 
   <section algorithm>
+    <h3 id="update-service-worker-extended-events-set-algorithm"><dfn>Update Service Worker Extended Events Set</dfn></h3>

I'm a little worried about the lifecycle of this, but I might be worrying over nothing.

If a service worker abruptly terminates (due to long-running tasks or whatever), what happens to these events? Should we clear the set of extended events on service worker terminate?

I guess we'll need to handle timeouts to `waitUntil` extension promises at some point.

-- 
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/1065#pullrequestreview-20526655