Re: [w3c/ServiceWorker] Skip executing fetch handlers if it is no-op. (PR #1672)

@jakearchibald requested changes on this pull request.



> @@ -2973,16 +2984,48 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/

               1. For each |eventType| of |settingsObject|'s [=environment settings object/global object=]'s associated list of <a>event listeners</a>' event types:
                   1. [=set/Append=] |eventType| to |workerGlobalScope|'s associated [=ServiceWorkerGlobalScope/service worker=]'s <a>set of event types to handle</a>.
 
-                  Note: If the global object's associated list of event listeners does not have any event listener added at this moment, the service worker's set of event types to handle remains an empty set. The user agents are encouraged to show a warning that the event listeners must be added on the very first evaluation of the worker script.
+                  Note: If the global object's associated list of event listeners does not have any event listener added at this moment, the service worker's <a>set of event types to handle</a> remains an empty set.

```suggestion
                  Note: If the global object's associated list of event listeners does not have any event listener added at this moment, the service worker's [=set of event types to handle=] remains an empty set.
```

The spec isn't consistent here at all, but I'm going with Bikeshed shorthands where they exist.

>  
               1. Set |script|'s <a>has ever been evaluated flag</a>.
+              1. If the [=Empty Handler identification=] algorithm with |workerGlobalScope| returns true, the [=all fetch listeners are empty flag=] is set.

Is this the logic we're implementing? It doesn't seem right to me.

Consider the:

```js
onfetch = Math.round(Math.random()) ? () => {} : notEmpty;
```

…case.

According to this, `[=all fetch listeners are empty flag=]` is set when a service worker run completes and all the fetch listeners are empty. But if, on a subsequent run, non-empty fetch listeners are added, `[=all fetch listeners are empty flag=]` stays set, so the name is wrong at least.

I see a couple of options:

Unset the flag if non-empty fetch listeners are added on a later run.

Or:

Rename `[=all fetch listeners are empty flag=]` to something like `[=all fetch listeners always empty=]`, a boolean, that starts `true`, and is only ever set to `false`. That means, in cases where the listeners are 'unstable', like the random case above, [=all fetch listeners always empty=]` will become false and stay false.

I thought we settled on the second option in discussions, but maybe I misunderstood.

>            1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
           1. [=map/Clear=] |workerGlobalScope|'s [=map of active timers=].
       1. Wait for |serviceWorker| to be [=running=], or for |startFailed| to be true.
       1. If |startFailed| is true, then return *failure*.
       1. Return |serviceWorker|'s [=start status=].
   </section>
 
+  <section algorithm>
+    <h3 id="empty-handler-identification-algorithm"><dfn>Empty Handler identification</dfn></h3>

Since this relates only to fetch listeners, the name should reflect that.

>            1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
           1. [=map/Clear=] |workerGlobalScope|'s [=map of active timers=].
       1. Wait for |serviceWorker| to be [=running=], or for |startFailed| to be true.
       1. If |startFailed| is true, then return *failure*.
       1. Return |serviceWorker|'s [=start status=].
   </section>
 
+  <section algorithm>
+    <h3 id="empty-handler-identification-algorithm"><dfn>Empty Handler identification</dfn></h3>
+
+      : Input
+      :: |workerGlobalScope|, a [=service worker/global object=].
+      : Output
+      :: a boolean
+
+      1. If |workerGlobalScope|'s <a>set of event types to handle</a> does not [=set/contain=] <code>fetch</code>, then returns false.

This is kinda surprising. Surely if, there are no fetch listeners, then by definition they're all empty?

Eg:

```js
const result = [].every((value) => value === 'hello');
```

The above is `true`.

>            1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
           1. [=map/Clear=] |workerGlobalScope|'s [=map of active timers=].
       1. Wait for |serviceWorker| to be [=running=], or for |startFailed| to be true.
       1. If |startFailed| is true, then return *failure*.
       1. Return |serviceWorker|'s [=start status=].
   </section>
 
+  <section algorithm>
+    <h3 id="empty-handler-identification-algorithm"><dfn>Empty Handler identification</dfn></h3>
+
+      : Input
+      :: |workerGlobalScope|, a [=service worker/global object=].
+      : Output
+      :: a boolean
+
+      1. If |workerGlobalScope|'s <a>set of event types to handle</a> does not [=set/contain=] <code>fetch</code>, then returns false.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of |workerGlobalScope|'s <a>set of event types to handle</a>:

This isn't right. `<a>set of event types to handle</a>` is a set of strings, not listeners.

In cases like this, where I want to get something like "all event listener callbacks of a particular type", I try to think of existing things that do that already that I could take inspiration from. In this case, I'd look at `target.dispatchEvent`, since it has to run all of the callbacks of a particular type.

So, I'd start at https://dom.spec.whatwg.org/#dom-eventtarget-dispatchevent, and follow the algorithm, and see how they gather up the callbacks. We probably need to do a similar thing here.

Following the trail takes me to https://dom.spec.whatwg.org/#concept-event-listener-invoke, which shows me how to get the listeners for a particular event target.

Then it goes to https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke, which shows me how to filter by event type.

Then it goes to https://webidl.spec.whatwg.org/#call-a-user-objects-operation which shows me how to get the callback from the two different forms of listener.

> @@ -3130,6 +3179,17 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/

       1. Return |response|.
   </section>
 
+  <section algorithm>
+    <h3 id="skippable-fetch-handler-algorithm"><dfn>Skippable Fetch Handler</dfn></h3>
+      : Input
+      :: |serviceWorker|, a [=/service worker=]
+      : Output
+      :: a boolean
+
+      1. If |serviceWorker| has [=all fetch listeners are empty flag=] and set, then the user agent *may* return true.

I think the "may" condition should sit in "Empty Handler identification", since that's the part of the process that's optional.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1672#pullrequestreview-1307019683

You are receiving this because you are subscribed to this thread.

Message ID: <w3c/ServiceWorker/pull/1672/review/1307019683@github.com>