- From: Jake Archibald <notifications@github.com>
- Date: Wed, 15 Feb 2023 05:31:29 -0800
- To: w3c/ServiceWorker <ServiceWorker@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <w3c/ServiceWorker/pull/1672/review/1299456159@github.com>
@jakearchibald requested changes on this pull request.
>
1. Set |script|'s <a>has ever been evaluated flag</a>.
+ 1. If the [=No-Op Handler identification=] algorithm with |serviceWorker| returns true, the [=has had non-no-op fetch listeners flag=] is set to false.
This seems backwards. I don't think we ever want to unset this flag. Or have I misunderstood the design?
```suggestion
1. If [=No-Op Handler identification=] given |serviceWorker| is false, then set |serviceWorker|'s [=has had non-no-op fetch listeners flag=].
```
When referring to a property like `[=has had non-no-op fetch listeners flag=]`, be clear about which object you're referring to.
> 1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
1. Empty |workerGlobalScope|'s <a>list of active timers</a>.
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="no-op-handler-identification-algorithm"><dfn>No-Op Handler identification</dfn></h3>
+
+ : Input
+ :: |serviceWorker|, a [=/service worker=]
+ : Output
+ :: a boolean
+
+ 1. If |service worker|'s set of event types to handle does not contain "fetch", then returns false.
`|service worker|` should be `|serviceWorker|` (as defined in the input)
References need to be linked properly here.
> 1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
1. Empty |workerGlobalScope|'s <a>list of active timers</a>.
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="no-op-handler-identification-algorithm"><dfn>No-Op Handler identification</dfn></h3>
+
+ : Input
+ :: |serviceWorker|, a [=/service worker=]
+ : Output
+ :: a boolean
+
+ 1. If |service worker|'s set of event types to handle does not contain "fetch", then returns false.
+ 1. If the callback's function bodies (https://tc39.es/ecma262/#prod-FunctionBody) of all fetch event listener in |service worker| are empty, then the user agent *may* return true.
Things like "callback" need to be referenced properly. Eg, see https://dom.spec.whatwg.org/#event-listener-callback - you'll need to handle cases where the 'callback' is actually an `EventListener` object.
`|service worker|` should be `|serviceWorker|` (as defined in the input)
Rather than just paste the link of https://tc39.es/ecma262/#prod-FunctionBody, create a proper reference. See the `<pre class="anchors">` section at the top of the document.
"of all fetch event listener" needs to be defined correctly in terms of https://dom.spec.whatwg.org/#concept-event-listener.
"empty" needs to be defined more robustly.
> 1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
1. Empty |workerGlobalScope|'s <a>list of active timers</a>.
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="no-op-handler-identification-algorithm"><dfn>No-Op Handler identification</dfn></h3>
+
+ : Input
+ :: |serviceWorker|, a [=/service worker=]
+ : Output
+ :: a boolean
+
+ 1. If |service worker|'s set of event types to handle does not contain "fetch", then returns false.
+ 1. If the callback's function bodies (https://tc39.es/ecma262/#prod-FunctionBody) of all fetch event listener in |service worker| are empty, then the user agent *may* return true.
+
+ 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).
This comment should be expanded upon, but I can help with that.
> 1. Run the <a>responsible event loop</a> specified by |settingsObject| until it is destroyed.
1. Empty |workerGlobalScope|'s <a>list of active timers</a>.
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="no-op-handler-identification-algorithm"><dfn>No-Op Handler identification</dfn></h3>
+
+ : Input
+ :: |serviceWorker|, a [=/service worker=]
+ : Output
+ :: a boolean
+
+ 1. If |service worker|'s set of event types to handle does not contain "fetch", then returns false.
+ 1. If the callback's function bodies (https://tc39.es/ecma262/#prod-FunctionBody) of all fetch event listener in |service worker| are empty, then the user agent *may* return true.
+
+ 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 false.
```suggestion
1. Return false.
```
> @@ -3041,7 +3061,7 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. Set |registration| to the result of running <a>Match Service Worker Registration</a> given |storage key| and |request|'s [=request/url=].
1. If |registration| is null or |registration|'s <a>active worker</a> is null, return null.
1. If |request|'s [=request/destination=] is not {{RequestDestination/"report"}}, set |reservedClient|'s <a>active service worker</a> to |registration|'s <a>active worker</a>.
- 1. If |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, and |registration|'s [=active worker=]'s <a>set of event types to handle</a> [=set/contains=] <code>fetch</code>, then:
+ 1. If |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s <a>set of event types to handle</a> [=set/contains=] <code>fetch</code>, and the result of running [=Skippable Fetch Handler=] algorithm with [=active worker=] is false then:
```suggestion
1. If |request| is a [=navigation request=], |registration|'s [=navigation preload enabled flag=] is set, |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s <a>set of event types to handle</a> [=set/contains=] <code>fetch</code>, and the result of running [=Skippable Fetch Handler=] algorithm with |registration|'s [=active worker=] is false then:
```
> @@ -3077,6 +3097,12 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. If the result of running the [=Should Skip Event=] algorithm with "fetch" and |activeWorker| is true, then:
1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
1. Return null.
+ 1. If the result of running [=Skippable Fetch Handler=] algorithm with |activeWorker| is true, then:
```suggestion
1. If the result of running the [=Skippable Fetch Handler=] algorithm with |activeWorker| is true, then:
```
> @@ -3077,6 +3097,12 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. If the result of running the [=Should Skip Event=] algorithm with "fetch" and |activeWorker| is true, then:
1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
1. Return null.
+ 1. If the result of running [=Skippable Fetch Handler=] algorithm with |activeWorker| is true, then:
+ 1. run the following steps [=in parallel=]:
```suggestion
1. [=In parallel=]:
```
> @@ -3077,6 +3097,12 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
1. If the result of running the [=Should Skip Event=] algorithm with "fetch" and |activeWorker| is true, then:
1. If |shouldSoftUpdate| is true, then [=in parallel=] run the [=Soft Update=] algorithm with |registration|.
1. Return null.
+ 1. If the result of running [=Skippable Fetch Handler=] algorithm with |activeWorker| is true, then:
+ 1. run the following steps [=in parallel=]:
+ 1. If |activeWorker|'s state is "activating", wait for |activeWorker|'s state to become "activated".
```suggestion
1. If |activeWorker|'s [=service worker/state=] is "activating", then wait for |activeWorker|'s [=service worker/state=] to become "activated".
```
Make sure terms you use are referenced
> @@ -191,6 +191,8 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
A [=/service worker=] has an associated <dfn>start status</dfn> which can be null or a [=Completion=]. It is initially null.
+ A [=/service worker=] has an associated <dfn>has had non-no-op fetch listeners flag</dfn>. It is initially true.
Maybe rename this to reflect the uncertain nature?
```suggestion
A [=/service worker=] has an associated <dfn>may have had non-empty fetch listener flag</dfn>. It is initially unset.
```
> @@ -3132,6 +3158,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 |has non-no-op fetch listeners flag|, and the value is false, then the user agent *may* return true.
What's the reason for "may" here, given that "No-Op Handler identification" already uses "may"?
--
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1672#pullrequestreview-1299456159
You are receiving this because you are subscribed to this thread.
Message ID: <w3c/ServiceWorker/pull/1672/review/1299456159@github.com>
Received on Wednesday, 15 February 2023 13:31:43 UTC