Changing skip-service-worker flag to use-service-workers enum #435

jakearchibald · 2016-12-13T17:02:07Z

This fixes #303. I don't think there's anything else to do with this PR, but I need to make changes to SW to set this flag on particular APIs.

jakearchibald · 2016-12-13T17:03:05Z

Technically, this and skip-service-worker could become an enum of "none", "local" and "all", but it feels like that'd break specs already using it. Is that the right thing to do?

jakearchibald · 2016-12-13T17:28:34Z

Related SW PR w3c/ServiceWorker#1025

annevk · 2016-12-15T07:54:52Z

I think I prefer the enum approach. There's not too many consumers as far as I know.

jakearchibald · 2016-12-15T07:59:59Z

Cool, I'll update the PR

annevk · 2016-12-15T08:15:48Z

Thanks. There's also #362 which relates to this flag somewhat.

annevk · 2016-12-16T08:07:47Z

fetch.bs

-
-<p>A <a for=/>request</a> has an associated <dfn export>skip-local-service-worker flag</dfn>. Unless
-stated otherwise it is unset.
+<dfn export>use-service-workers value</dfn>, that is "<code>all</code>",


This needs for=request too. Seems we forgot that until now.

annevk · 2016-12-16T08:08:33Z

So where do we check for the foreign value? If we only ever set it, it seems like we could also reuse another value, no?

annevk · 2016-12-16T08:09:33Z

Do we have WPT tests around this?

jakearchibald · 2016-12-16T11:26:57Z

So where do we check for the foreign value? If we only ever set it, it seems like we could also reuse another value, no?

Yeah, when it comes to performing foreign fetch we're only really concerned that the value isn't "none". Although I'm not sure what else could be reused.

Do we have WPT tests around this?

No, I guess this is something I should learn and do 😄

annevk · 2016-12-16T13:18:42Z

Yeah, when it comes to performing foreign fetch we're only really concerned that the value isn't "none".

And it cannot be "all" either I guess? I guess "foreign" is fine then. Can @mkruisselbrink have a look or is he still away?

annevk · 2016-12-16T13:19:15Z

As for the tests, the new rule is that they land together with the specification change. Let me know if you need help.

annevk · 2016-12-16T13:20:21Z

And only https://w3c.github.io/ServiceWorker/ seems to use the skip-service-worker flag currently. We should maybe prepare a PR there too so they can all land at the same time.

jakearchibald · 2016-12-16T13:23:09Z

w3c/ServiceWorker#1025

annevk · 2016-12-16T13:26:03Z

fetch.bs

@@ -728,8 +728,9 @@ explicitly set <a for=/>request</a>'s
 this flag set are subject to additional processing requirements.

 <p>A <a for=/>request</a> has an associated
-<dfn export>skip-service-worker flag</dfn>. Unless stated otherwise it is
-unset.
+<dfn export>use-service-workers value</dfn>, that is "<code>all</code>",


What do you think about <dfn export for=request>service-workers mode</dfn>? I think that would be more consistent with what we have thus far.

SGTM, I wasn't happy with the naming

mkruisselbrink · 2016-12-16T18:45:14Z

fetch.bs

@@ -2793,16 +2794,14 @@ optional <i>CORS flag</i> and <i>CORS-preflight flag</i>, run these steps:
 <li><p>Let <var>actualResponse</var> be null.

 <li>
-  <p>If <var>request</var>'s <a>skip-service-worker flag</a> is unset, then run these
+  <p>If <var>request</var>'s <a>service-workers mode</a> is not "<code>none</code>", then run these
  substeps:

  <ol>
   <li>
    <p>If <var>request</var>'s <a for=request>client</a> is null or


Do we still need the client is null check, or is just relying on the service-workers mode enough now? And in particular it seems wrong that service-workers mode==foreign and client==null will now result in handle fetch being called.

I think that relying on service-workers mode is enough. At least, I thought the idea was that having a client no longer mattered.

mkruisselbrink · 2016-12-16T18:49:23Z

fetch.bs

@@ -728,8 +728,9 @@ explicitly set <a for=/>request</a>'s
 this flag set are subject to additional processing requirements.

 <p>A <a for=/>request</a> has an associated
-<dfn export>skip-service-worker flag</dfn>. Unless stated otherwise it is
-unset.
+<dfn for=request export>service-workers mode</dfn>, that is "<code>all</code>",


Would it make sense to add some non-normative text somewhere to explain what this mode actually means? Of course you can figure that out by reading the algorithms, but I find the non-normative notes that many of the requests attributes have to be quite helpful in quickly figuring out what a particular thing is for.

Yeah, sounds good.

mkruisselbrink · 2016-12-16T18:53:20Z

Do we have WPT tests around this?

No, I guess this is something I should learn and do

Hmm, I should probably also work on actually upstreaming the existing foreign fetch tests we have in blink. Although not sure what WPT tests would be desired for this specific change, as this change by itself doesn't seem to have any behavioral changes?

annevk · 2016-12-20T08:32:05Z

Although not sure what WPT tests would be desired for this specific change, as this change by itself doesn't seem to have any behavioral changes?

The fact that certain fetches from the service worker can now invoke the service worker is a normative change that will result in behavioral changes.

jakearchibald · 2016-12-20T09:59:53Z

I agree this is a change that needs tests. Will pick this up after the Christmas break.

jakearchibald · 2017-01-10T12:48:23Z

Tests: jakearchibald/web-platform-tests@0fa7353

@annevk would be grateful for a review. Be kind!

annevk · 2017-01-10T13:00:36Z

@jakearchibald probably best to create a PR on web-platform-tests for review. Overall I think it's fine, but since you need permissions you'll have to use the -manual suffix I think.

jakearchibald · 2017-01-10T14:42:36Z

web-platform-tests/wpt#4518

jakearchibald · 2017-02-14T10:41:34Z

@annevk tests merged. Happy to merge this?

annevk · 2017-02-14T11:33:23Z

fetch.bs

+"<code>all</code>".
+
+<div class=note>
+ This determines which service workers will receive a {{fetch!!event}} event for this fetch.


This should start with a <p>.

annevk · 2017-02-14T11:47:47Z

One more question, in a commit you stated #362 is fixed by this, but that seems unlikely as service workers are skipped for redirects with this change. So are we leaving that for that issue?

Could you write a commit message for this?

jakearchibald · 2017-02-14T12:03:20Z

@annevk uhhhh good catch. Sorry about that. As far as I can tell it just needed that "none" changed to "foreign".

jakearchibald · 2017-02-14T13:01:27Z

Commit message:

Add "service-workers mode" flag to allow particular fetches within a service worker to trigger "fetch" events.

Fixes: #303, #362.
Related service worker PR: https://github.com/w3c/ServiceWorker/pull/1025.
Tests: https://github.com/w3c/web-platform-tests/pull/4518.

annevk · 2017-02-14T13:08:16Z

Thanks! (Adjusted the message a bit to fit style guidelines.)

This was changed in the following PR to the Fetch standard: whatwg/fetch#435 BUG=592188 Review-Url: https://codereview.chromium.org/2714423002 Cr-Commit-Position: refs/heads/master@{#453432}

Introduce service-worker mode Bringing in the spec changes from whatwg/fetch#435.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17491)

…date-fetch); r=jdm Bringing in the spec changes from whatwg/fetch#435. Source-Repo: https://github.com/servo/servo Source-Revision: e2a26e7bd0d8089a441bc3072cf15351b0ef1252 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 05549195b43020dcd02768bcd79cef5f7ed4b813

…date-fetch); r=jdm Bringing in the spec changes from whatwg/fetch#435. Source-Repo: https://github.com/servo/servo Source-Revision: e2a26e7bd0d8089a441bc3072cf15351b0ef1252

…date-fetch); r=jdm Bringing in the spec changes from whatwg/fetch#435. Source-Repo: https://github.com/servo/servo Source-Revision: e2a26e7bd0d8089a441bc3072cf15351b0ef1252 UltraBlame original commit: 7ad2e4b94b03bb9e821f6962fc7df6fc0f936519

jakearchibald mentioned this pull request Dec 13, 2016

Opt cache.add/addAll and importScripts out of a local service worker w3c/ServiceWorker#1025

Merged

jungkees mentioned this pull request Dec 15, 2016

Navigation preload w3c/ServiceWorker#983

Merged

annevk reviewed Dec 16, 2016

View reviewed changes

jakearchibald changed the title ~~WIP: Making a skip-local-service-worker flag~~ WIP: Changing skip-service-worker flag to use-service-workers enum Dec 16, 2016

jakearchibald mentioned this pull request Dec 16, 2016

Consider mechanisms to bypass the service worker for things that we know won't be in it's cache w3c/ServiceWorker#1026

Open

annevk reviewed Dec 16, 2016

View reviewed changes

mkruisselbrink reviewed Dec 16, 2016

View reviewed changes

jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this pull request Jan 10, 2017

Tests for whatwg/fetch#435

0fa7353

jakearchibald added 4 commits January 10, 2017 14:43

Making a skip-local-service-worker flag

ccd19cb

Type

cb60254

Updating service worker flag to enum & also fixes #362

d4e5d83

service-workers mode

584fb3d

Adding note to “service-workers mode”

835862f

jakearchibald force-pushed the sw-fetch-within-sw branch from 7d2c1b5 to 835862f Compare January 10, 2017 15:22

jakearchibald changed the title ~~WIP: Changing skip-service-worker flag to use-service-workers enum~~ Changing skip-service-worker flag to use-service-workers enum Jan 10, 2017

jakearchibald added a commit to jakearchibald/web-platform-tests that referenced this pull request Feb 13, 2017

Tests for whatwg/fetch#435

61d3c7f

Ms2ger pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 14, 2017

Tests for whatwg/fetch#435 (#4518)

6dbe2c7

annevk reviewed Feb 14, 2017

View reviewed changes

jakearchibald added 2 commits February 14, 2017 11:42

Removing clients check

73630b8

Missing paragraph

e36eae6

Missed a bit when it came to #362

27ae1e1

annevk merged commit d41c238 into master Feb 14, 2017

annevk deleted the sw-fetch-within-sw branch February 14, 2017 13:08

KiChjang mentioned this pull request Jun 23, 2017

Introduce service-worker mode servo/servo#17491

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing skip-service-worker flag to use-service-workers enum #435

Changing skip-service-worker flag to use-service-workers enum #435

jakearchibald commented Dec 13, 2016

jakearchibald commented Dec 13, 2016

jakearchibald commented Dec 13, 2016

annevk commented Dec 15, 2016

jakearchibald commented Dec 15, 2016

annevk commented Dec 15, 2016

annevk Dec 16, 2016

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

jakearchibald commented Dec 16, 2016

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

jakearchibald commented Dec 16, 2016

annevk Dec 16, 2016

jakearchibald Dec 16, 2016

mkruisselbrink Dec 16, 2016

annevk Dec 20, 2016

mkruisselbrink Dec 16, 2016

annevk Dec 16, 2016

jakearchibald Jan 10, 2017

mkruisselbrink commented Dec 16, 2016

annevk commented Dec 20, 2016

jakearchibald commented Dec 20, 2016

jakearchibald commented Jan 10, 2017

annevk commented Jan 10, 2017

jakearchibald commented Jan 10, 2017

jakearchibald commented Feb 14, 2017

annevk Feb 14, 2017

annevk commented Feb 14, 2017

jakearchibald commented Feb 14, 2017

jakearchibald commented Feb 14, 2017

annevk commented Feb 14, 2017

Changing skip-service-worker flag to use-service-workers enum #435

Changing skip-service-worker flag to use-service-workers enum #435

Conversation

jakearchibald commented Dec 13, 2016

jakearchibald commented Dec 13, 2016

jakearchibald commented Dec 13, 2016

annevk commented Dec 15, 2016

jakearchibald commented Dec 15, 2016

annevk commented Dec 15, 2016

Choose a reason for hiding this comment

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

jakearchibald commented Dec 16, 2016

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

annevk commented Dec 16, 2016

jakearchibald commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkruisselbrink commented Dec 16, 2016

annevk commented Dec 20, 2016

jakearchibald commented Dec 20, 2016

jakearchibald commented Jan 10, 2017

annevk commented Jan 10, 2017

jakearchibald commented Jan 10, 2017

jakearchibald commented Feb 14, 2017

Choose a reason for hiding this comment

annevk commented Feb 14, 2017

jakearchibald commented Feb 14, 2017

jakearchibald commented Feb 14, 2017

annevk commented Feb 14, 2017