Re: [w3c/ServiceWorker] Add FetchEvent.handled (#1397) (#1496)

jakearchibald requested changes on this pull request.

This is heading in the right direction. Are you planning to write web platform tests too?

> @@ -2932,6 +2940,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
       1. Let |client| be |request|'s [=request/client=].
       1. Let |reservedClient| be |request|'s [=request/reserved client=].
       1. Let |preloadResponse| be a new [=promise=].
+      1. Let |eventHandled| be a new [=promise=].

We're a little inconsistent here because specs were still being figured out when we first wrote this, but the above should use [=a new promise=], which points to https://heycam.github.io/webidl/#a-new-promise.

You should also specify a realm for this, so this line should come just before line 20, when you have the active worker, and therefore "the [=relevant realm=] of the |activeWorker|'s [=service worker/global object=]".

> @@ -3007,6 +3017,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
           The |task| *must* use |activeWorker|'s <a>event loop</a> and the <a>handle fetch task source</a>.
 
       1. Wait for |task| to have executed or for |handleFetchFailed| to be true.
+      1. If |handleFetchFailed| is false and |eventCanceled| is false, then resolve |eventHandled| with undefined, otherwise reject it with "{{NetworkError}}" {{DOMException}}.

Use [=resolve=] and [=reject=]. You don't need to say "with undefined", that's the default.

"reject it with a" rather than just "reject it with". And again I'd specify the realm.

I'm not sure the condition is right here. It's only an exception if |respondWithEntered| is false and |eventCanceled| is true.

It might be better to handle resolving before each return in the next few lines, then we aren't duplicating the logic and risking them getting out of sync.

-- 
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/1496#pullrequestreview-341211282