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

@jakearchibald requested changes on this pull request.

This is really close to ready!

The PR uses `<a spec="">…</a>` links to link to concepts that haven't been exported by other specs. Instead, PRs should be made against those specs to export those terms. Example: https://github.com/whatwg/html/pull/8924


>            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="all-fetch-listeners-are-empty-algorithm"><dfn>All Fetch Listeners Are Empty</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 true.

```suggestion
      1. If |workerGlobalScope|'s [=set of event types to handle=] does not [=set/contain=] <code>fetch</code>, then return 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="all-fetch-listeners-are-empty-algorithm"><dfn>All Fetch Listeners Are Empty</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 true.
+      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value.

```suggestion
      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["onfetch"]'s value.
```

As per https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-name, handler names always start "`on`".

>            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="all-fetch-listeners-are-empty-algorithm"><dfn>All Fetch Listeners Are Empty</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 true.
+      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of |workerGlobalScope|'s [=event listener list=]:
+          1. If |eventListener|'s <a spec="dom">type</a> is <code>fetch</code>, then [=list/append=] |eventListener| to |eventListenerList|.

```suggestion
          1. If |eventListener|'s <a spec="dom">type</a> is "<code>fetch</code>", then [=list/append=] |eventListener| to |eventListenerList|.
```

>            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="all-fetch-listeners-are-empty-algorithm"><dfn>All Fetch Listeners Are Empty</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 true.
+      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of |workerGlobalScope|'s [=event listener list=]:
+          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:

```suggestion
          1. If |eventListener|'s [=event listener/callback=] is null, then [=continue=].
```

Then un-indent the subsequent lines. This is mostly personal preference. I prefer early-return/continue rather than nesting everything in an `if`.

I swapped `[=callback=]` for `[=event listener/callback=]`. It isn't an issue now, but it feels like this could become ambiguous in future.

> +  <section algorithm>
+    <h3 id="all-fetch-listeners-are-empty-algorithm"><dfn>All Fetch Listeners Are Empty</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 true.
+      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of |workerGlobalScope|'s [=event listener list=]:
+          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| equals |eventHandler|'s <a spec="html">listener</a>, then set |callback| to the result of [=converting|convert to an ECMAScript value=] |eventHandler|'s value to an ECMAScript value.

"|eventHandler|" may be null here.

Also, "value" in "|eventHandler|'s value" needs to be referenced properly.

> +      1. Let |eventHandler| be |workerGlobalScope|'s <a spec="html">event handler map</a>["fetch"]'s value.
+      1. Let |eventListenerList| be an empty [=list=].
+      1. [=list/For each=] |eventListener| of |workerGlobalScope|'s [=event listener list=]:
+          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| equals |eventHandler|'s <a spec="html">listener</a>, then set |callback| to the result of [=converting|convert to an ECMAScript value=] |eventHandler|'s value to an ECMAScript value.
+              1. Otherwise, set |callback| to the result of [=converting|convert to an ECMAScript value=] |eventListener|'s [=callback=] to an ECMAScript value.
+              1. If [$IsCallable$](|callback|) is false:
+                  1. Let |getResult| be [=Completion=]([$Get$](|callback|), <code>handleEvent</code>).
+                  1. If |getResult| is [=abrupt completion=], then return false.
+                  1. Set |callback| to |getResult|.
+              1. If [$IsCallable$](|callback|) is false, then return false.
+              1. If |callback|'s [=function body=] is not empty (i.e. either a [=statement=] or [=declaration=] exist), then return false.
+
+              Note: it detects something like `onfetch = () => {}`. Some sites have a fetch event listener with empty body to make them recognized as progressive web application (PWA).

```suggestion
              Note: This detects "<code>fetch</code>" listeners like `() => {}`. Some sites have a fetch event listener with empty body to make them recognized by Chromium as a progressive web application (PWA).
```

> +      1. [=list/For each=] |eventListener| of |eventListenerList|:
+          1. If |eventListener|'s [=callback=] is not null:
+              1. If |eventListener| equals |eventHandler|'s <a spec="html">listener</a>, then set |callback| to the result of [=converting|convert to an ECMAScript value=] |eventHandler|'s value to an ECMAScript value.
+              1. Otherwise, set |callback| to the result of [=converting|convert to an ECMAScript value=] |eventListener|'s [=callback=] to an ECMAScript value.
+              1. If [$IsCallable$](|callback|) is false:
+                  1. Let |getResult| be [=Completion=]([$Get$](|callback|), <code>handleEvent</code>).
+                  1. If |getResult| is [=abrupt completion=], then return false.
+                  1. Set |callback| to |getResult|.
+              1. If [$IsCallable$](|callback|) is false, then return false.
+              1. If |callback|'s [=function body=] is not empty (i.e. either a [=statement=] or [=declaration=] exist), then return false.
+
+              Note: it detects something like `onfetch = () => {}`. Some sites have a fetch event listener with empty body to make them recognized as progressive web application (PWA).
+
+      1. Return true.
+
+      Note: the user agents are encouraged to show warnings that the event listeners are no-op, and their executions can be skipped.

```suggestion
      Note: User agents are encouraged to show a warning indicating that empty "<code>fetch</code>" listeners are unnecessary, and may have a negative performance impact.
```

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

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

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