Introduce reload-navigation flag #685

yutakahirano · 2018-03-26T09:31:06Z

This change introduces reload-navigation flag and isReloadNavigation
attribute on Request class.

yutakahirano · 2018-03-26T09:31:43Z

I'll work on the HTML spec to set the flags.

annevk

Would be great if @wanderview and @jakearchibald could have a look at these additions.

I'd like to see tests for all combinations here as well, in particular because this is introducing changes to poorly understood algorithms. We better cover all angles. E.g., both GET and POST form submission, that in combination with history traversal and reloads, etc.

annevk · 2018-03-26T12:00:53Z

fetch.bs

+
+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=submission-navigation-flag>submission-navigation flag</dfn>. Unless stated
+otherwise, it is unset.


It would be good to add a note here why we need these. I also wonder if they should be tri-state since they don't make sense for the Cache API.

Tri-state so it can have an 'unset' value? Seems like it'd be less confusing to just store these values in the cache API, even though it isn't particularly useful.

Yeah, to indicate it's not relevant/known. I guess I'm fine with storing more bits though.

In general I think any field that is added to Request must be stored in cache API. At least that is what we currently do and I think it would be unexpected for cache.keys() to return something that differs from what was passed to cache.put().

jakearchibald · 2018-03-26T13:50:04Z

fetch.bs

+ <a>keepalive flag</a> is <var>request</var>'s <a>keepalive flag</a>,
+ <a>reload-navigation flag</a> is  <var>request</var>'s <a>reload-navigation flag</a>,
+ <a>history-navigation flag</a> is  <var>request</var>'s <a>history-navigation flag</a>, and
+ <a>submission-navigation flag</a> is  <var>request</var>'s <a>submission-navigation flag</a>.


@annevk do you think this should become an unordered list? It's becoming a bit tough to read.

We could do that, though if we do we should perhaps also clean up the other 2-3 "a new request" places to make them all similar.

Also, if we don't, we should remove the double spaces here.

I guess it should be a definition list rather than an unordered list.

jakearchibald · 2018-03-26T14:04:34Z

LGTM if the double spaces issue is resolved.

domenic · 2018-04-03T22:59:08Z

For anyone wondering, I figured out from around w3c/ServiceWorker#1167 (comment) and nearby comments that form submission and reload can both be true. (That's why it's three booleans instead of an enum.) Maybe a non-normative note explaining the design would be helpful.

wanderview

Sorry, I tried to leave this comment before, but I think it was stuck in github drafts or something.

wanderview · 2018-03-26T18:20:09Z

fetch.bs

+ <a>keepalive flag</a> is <var>request</var>'s <a>keepalive flag</a>,
+ <a>reload-navigation flag</a> is  <var>request</var>'s <a>reload-navigation flag</a>,
+ <a>history-navigation flag</a> is  <var>request</var>'s <a>history-navigation flag</a>, and
+ <a>submission-navigation flag</a> is  <var>request</var>'s <a>submission-navigation flag</a>.


So if a fetch event handler dealing with a navigation reload does:

let r2 = new Request(evt.request)

Then r2.mode will be "same-origin", but r2.isReloadNavigation will be true?

That seems a bit unexpected. Shouldn't we try to keep isReloadNavigation in sync with mode being 'navigate'?

I think r2.mode will be "navigation"... or are you talking about "If any of init’s members are present,..."?

Right. I was thinking of that clause. Should we reset this property in the same case?

Nice catch, thank you. Fixed.

yutakahirano · 2018-04-04T00:41:32Z

For anyone wondering, I figured out from around w3c/ServiceWorker#1167 (comment) and nearby comments that form submission and reload can both be true. (That's why it's three booleans instead of an enum.) Maybe a non-normative note explaining the design would be helpful.

Added some texts.

yutakahirano · 2018-04-06T13:24:19Z

@wanderview, do you have any other comments?

wanderview · 2018-04-06T13:43:42Z

Sorry, I suck at github review interface.

annevk · 2018-04-10T07:16:15Z

Can't isHistoryNavigation and isSubmissionNavigation be true at the same time as well? If you navigate back to a page that was the result of a form submission? What happens in that case?

yutakahirano · 2018-04-19T07:29:20Z

I come to think that narrowing down this change to reload-only is good. That way it will be aligned with other PRs(web-platform-tests/wpt#10192 and whatwg/html#3592) and we can land the changes without waiting for other features / tests. @annevk, @domenic , what do you think about the idea?

annevk · 2018-04-19T08:08:59Z

That sounds like a good idea; a lot less complexity to think through.

yutakahirano · 2018-04-23T06:14:39Z

Done.

annevk · 2018-04-23T13:32:19Z

fetch.bs

@@ -1162,6 +1162,10 @@ Unless stated otherwise, it is "<code>basic</code>".
 <p>A <a for=/>request</a> has an associated <dfn export for=request id=done-flag>done flag</dfn>.
 Unless stated otherwise, it is unset.

+<p>A <a for=/>request</a> has an associated
+<dfn export for=request id=concept-request-reload-navigation-flag>reload-navigation flag</dfn>.
+Unless stated otherwise, it is unset.


Let's add a note here that this is for the exclusive use of HTML's navigate algorithm.

annevk · 2018-04-23T13:32:42Z

fetch.bs

@@ -5153,6 +5162,8 @@ constructor must run these steps:
   "<code>navigate</code>", then set it to "<code>same-origin</code>".
   <!-- This works because we have reset request's client too. -->

+   <li><p>Unset <var>request</var>'s <a for=request>reload-navigation flag</a>.


Nice! Is this tested?

Added a test in web-platform-tests/wpt#10192 (fetch/api/request/request-reset-attributes.https.html)

annevk · 2018-04-23T13:37:05Z

Is this flag stored in the Cache API?

wanderview · 2018-04-23T13:38:03Z

Is this flag stored in the Cache API?

It should be, IMO.

annevk · 2018-04-23T13:45:07Z

Okay, then you can observe the "Nice!" bit I spotted above. By storing the request on the service worker side in the Cache API, then passing it to fetch() on the client side, and then checking on the service worker side whether the flag got reset or not based on the surrounding conditions. (I wonder if we sufficiently considered the implications of what I describe here for other Request bits by the way.)

wanderview · 2018-04-23T13:52:58Z

Okay, then you can observe the "Nice!" bit I spotted above. By storing the request on the service worker side in the Cache API, then passing it to fetch() on the client side, and then checking on the service worker side whether the flag got reset or not based on the surrounding conditions.

In theory we could use this still test to that validate fetch() correctly handles all the different conditions in step 14 of Request() algorithm.

annevk · 2018-04-24T10:19:42Z

I pushed some nits here. I think this is good to go.

Remaining:

Set request's reload-navigation flag when the navigation is reload-trigerred html#3592 needs to be reviewed and in order. I asked @domenic since I'm still a little unsure about the setup there.
Add tests for Request.isReloadNavigation web-platform-tests/wpt#10192 looks good, but it lacks a test for the Cache API. It seems good to add that given that we tend to neglect the Cache API...
We need links to browser bugs as per https://github.com/whatwg/meta/blob/master/MAINTAINERS.md#handling-pull-requests.

yutakahirano · 2018-04-25T06:02:16Z

Edge: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/17186383/
WebKit: https://bugs.webkit.org/show_bug.cgi?id=184957
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1456709
Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=836697

wanderview · 2018-04-25T14:26:37Z

For Firefox please see this bug instead:

https://bugzilla.mozilla.org/show_bug.cgi?id=1456479

Sorry I filed it before, but forgot to link it here.

yutakahirano · 2018-04-26T23:18:15Z

Remaining:

I think I've addressed all of them.

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.

See w3c/ServiceWorker#1167 for the discussion that led to this change and whatwg/fetch#685 for the infrastructure in Fetch this builds upon. Tests: web-platform-tests/wpt#10192.

annevk · 2018-04-27T07:21:05Z

Thanks @yutakahirano. Really great work!

…, a=testonly Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167. -- wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a wpt-pr: 10192

See w3c/ServiceWorker#1167 for the discussion that led to this change and whatwg/fetch#685 for the infrastructure in Fetch this builds upon. Tests: web-platform-tests/wpt#10192.

…, a=testonly Automatic update from web-platform-testsAdd tests for Request.isReloadNavigation See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167. -- wpt-commits: 58ee169367245c6fe5edc01177eac68f76c12f4a wpt-pr: 10192 UltraBlame original commit: 96c5380b8719433164c1c86dac5b4c8db50c1966

yutakahirano added 2 commits March 23, 2018 13:38

initial

9291a90

fix

f097cbf

annevk reviewed Mar 26, 2018

View reviewed changes

jakearchibald reviewed Mar 26, 2018

View reviewed changes

jakearchibald approved these changes Mar 26, 2018

View reviewed changes

yutakahirano added 2 commits March 27, 2018 12:58

wip

f7944f2

fix

741fd61

wanderview requested changes Apr 3, 2018

View reviewed changes

yutakahirano added 2 commits April 4, 2018 09:27

fix

73532d9

fix

ac1c0ed

wanderview approved these changes Apr 6, 2018

View reviewed changes

annevk added the needs tests Moving the issue forward requires someone to write tests label Apr 10, 2018

yutakahirano mentioned this pull request Apr 10, 2018

Add tests for Request.isReloadNavigation web-platform-tests/wpt#10192

Merged

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Apr 19, 2018

yutakahirano added 2 commits April 23, 2018 14:59

fix

5159ee2

fix

f1d4fe7

yutakahirano changed the title ~~Introduce {reload, history, submission}-navigation flags~~ Introduce reload-navigation flags Apr 23, 2018

yutakahirano changed the title ~~Introduce reload-navigation flags~~ Introduce reload-navigation flag Apr 23, 2018

fix

b5fd742

annevk reviewed Apr 23, 2018

View reviewed changes

yutakahirano and others added 2 commits April 24, 2018 16:37

fix

e90da5a

nits

685b0ea

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 27, 2018

Add tests for Request.isReloadNavigation

58ee169

See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.

annevk merged commit 42ae7a1 into master Apr 27, 2018

annevk deleted the yhirano/navigation-type branch April 27, 2018 07:19

annevk removed do not merge yet Pull request must not be merged per rationale in comment needs tests Moving the issue forward requires someone to write tests labels Apr 27, 2018

yutakahirano mentioned this pull request May 8, 2018

Set request's history-navigation flag for history traversal whatwg/html#3674

Merged

Introduce reload-navigation flag #685

Introduce reload-navigation flag #685

Conversation

yutakahirano commented Mar 26, 2018 • edited by pr-preview bot

yutakahirano commented Mar 26, 2018 • edited

annevk left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakearchibald commented Mar 26, 2018

domenic commented Apr 3, 2018 • edited

wanderview left a 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

yutakahirano commented Apr 4, 2018

yutakahirano commented Apr 6, 2018

wanderview commented Apr 6, 2018

annevk commented Apr 10, 2018

yutakahirano commented Apr 19, 2018

annevk commented Apr 19, 2018

yutakahirano commented Apr 23, 2018

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

annevk commented Apr 23, 2018

wanderview commented Apr 23, 2018

annevk commented Apr 23, 2018

wanderview commented Apr 23, 2018

annevk commented Apr 24, 2018

yutakahirano commented Apr 25, 2018

wanderview commented Apr 25, 2018

yutakahirano commented Apr 26, 2018

annevk commented Apr 27, 2018

yutakahirano commented Mar 26, 2018 •

edited by pr-preview bot

yutakahirano commented Mar 26, 2018 •

edited

domenic commented Apr 3, 2018 •

edited