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

@jakearchibald requested changes on this pull request.



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

```suggestion
              1. The user agent may, if the [=Empty Fetch Handler Identification=] algorithm with |workerGlobalScope| returns true, set |serviceWorker|'s [=all fetch listeners are empty flag=].
```

@domenic how do you feel about this wording? Alternatively, a first step in "Empty Fetch Handler Identification" could be "The user agent may return false".

I want to make it clear that the steps in "Empty Fetch Handler Identification" are optional.

>  
               1. Set |script|'s <a>has ever been evaluated flag</a>.
+              1. Unset the [=all fetch listeners are empty flag=].

```suggestion
              1. Unset the |serviceWorker|'s [=all fetch listeners are empty flag=].
```

There are many service workers, so when you're detailing a property of a particular service worker, make sure you state which service worker you're talking about.

>  
               1. Set |script|'s <a>has ever been evaluated flag</a>.
+              1. Unset the [=all fetch listeners are empty flag=].
+              1. If the [=Empty Fetch Handler Identification=] algorithm with |workerGlobalScope| returns true, the [=all fetch listeners are empty flag=] may be set.
+
+              Note: the user agents are encouraged to show warnings that the event listeners are no-op, and their executions can be skipped.

This note should be moved to the relevant part in "Empty Fetch Handler Identification".

>            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-fetch-handler-identification-algorithm"><dfn>Empty Fetch Handler Identification</dfn></h3>

I think this should be renamed "All fetch handlers are empty". With it current name, it sounds like it might return true if it 'identifies' a single empty fetch handler. Naming is hard 😄 

>            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-fetch-handler-identification-algorithm"><dfn>Empty Fetch Handler Identification</dfn></h3>
+
+      : Input
+      :: |workerGlobalScope|, a [=service worker/global object=].
+      : Output
+      :: a boolean
+
+      1. If |workerGlobalScope|'s [=set of event types to handle=] does not [=set/contain=] <code>fetch</code>, then returns false.

I still think this should return true, since every fetch listener (of which there are none) successfully passes the "is empty" check. This is how `[].every` works.

> +      1. Let |onFetchHandler| be |workerGlobalScope|'s <code>onfetch</code> attribute.
+      1. If |onFetchHandler|'s <a spec="html">listener</a> is null, then return false.

This doesn't seem correct. That attribute can be null even if there are many fetch listeners.

```js
addEventListener('fetch', () => {});
```

In this case, `onfetch` is null, but there's an empty listener.

>            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-fetch-handler-identification-algorithm"><dfn>Empty Fetch Handler Identification</dfn></h3>
+
+      : Input
+      :: |workerGlobalScope|, a [=service worker/global object=].
+      : Output
+      :: a boolean
+
+      1. If |workerGlobalScope|'s [=set of event types to handle=] does not [=set/contain=] <code>fetch</code>, then returns false.
+      1. Let |onFetchHandler| be |workerGlobalScope|'s <code>onfetch</code> attribute.
+      1. If |onFetchHandler|'s <a spec="html">listener</a> is null, then return false.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of [onFetchHandler|'s <a spec="html">listener</a>:

This doesn't seem correct. https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-listener is not a list, or something that otherwise makes sense to loop over.

> +    <h3 id="empty-fetch-handler-identification-algorithm"><dfn>Empty Fetch Handler Identification</dfn></h3>
+
+      : Input
+      :: |workerGlobalScope|, a [=service worker/global object=].
+      : Output
+      :: a boolean
+
+      1. If |workerGlobalScope|'s [=set of event types to handle=] does not [=set/contain=] <code>fetch</code>, then returns false.
+      1. Let |onFetchHandler| be |workerGlobalScope|'s <code>onfetch</code> attribute.
+      1. If |onFetchHandler|'s <a spec="html">listener</a> is null, then return false.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of [onFetchHandler|'s <a spec="html">listener</a>:
+          1. If |eventListener|'s <a spec="dom">type</a> is <code>fetch</code>, then [=list/append=] |eventListener| to |eventListenerList|.
+      1. [=list/For each=] |eventListener| of |eventListenerList|:
+          1. If |eventListener|'s [=callback=] is not null:
+              1. If |eventListener| is an EventListener of the type created by <a spec="html">activate an event handler</a>, then set |callback| to the result of [=converting|convert to an ECMAScript value=] |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value to an ECMAScript value.

@domenic ignoring the formatting issues, are you ok with this kind of hand wave? Alternative language could be: "Get the [event handler](https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers) with a [listener](https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-listener) of |eventListener|'s [=callback=], or null if none exists."

Then we can pull the value from that object.

>  
+  <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 return true.
+      1. Return false.
+  </section>

I still don't think we need this. It's just returning an existing boolean value.

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

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

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