Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set request's reload-navigation flag when the navigation is reload-trigerred #3592

Merged
merged 5 commits into from
Apr 27, 2018
Merged

Set request's reload-navigation flag when the navigation is reload-trigerred #3592

merged 5 commits into from
Apr 27, 2018

Conversation

yutakahirano
Copy link
Member

@yutakahirano yutakahirano commented Mar 27, 2018 •
edited by pr-preview bot
Loading

@yutakahirano
Copy link
Member Author

@annevk @wanderview @jakearchibald

annevk
annevk reviewed Mar 29, 2018
View reviewed changes
source Outdated
@@ -81809,6 +81810,13 @@ interface <dfn>Location</dfn> { // but see also <a href="#the-location-interface
data-export=""><var>exceptions enabled flag</var></dfn>, the user agent must run these steps:</p>

<ol>
<li><p>If <var>resource</var> is a <span data-x="concept-request">request</span>, then set
<var>resource</var> to a copy of <var>resource</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change the algorithm may set a flag on the request. I thought that call-sites might be surprised by that behavior, and made a copy in case. If that's not the case I can remove this statement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the callers of navigate don't hold onto the objects they pass. And I think only <form> passes request objects, so that should be easy to verify.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Which also means that location.reload() doesn't, which means this patch doesn't do what it wants I think.)

Copy link
Member Author

@yutakahirano yutakahirano Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what kind of object does location.reload() pass? If it's neither URL nor request, is it handled by "If resource is a request whose url's scheme is a fetch scheme..." statement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I think location.reload() does pass a URL at the moment, but I think it should pass a "session history entry" since it's supposed to take into account whether the document was POSTed and such.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I deleted this statement.

source Outdated

<li><p>If <var>resource</var> is a <span data-x="concept-request">request</span> and this is
a <dfn>reload-triggered navigation</dfn>, then set <var>resource</var>'s <span
data-x="concept-request-reload-navigation-flag">reload-navigation flag</span>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that a manual refresh wouldn't set the flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what do you mean by "manual refresh"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user-initiated refresh.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I put the statement in a wrong place. Fixed.

@yutakahirano
Copy link
Member Author

@annevk, are you happy with this change?

@annevk
Copy link
Member

annevk commented Apr 10, 2018

This doesn't work when resource is a session history entry, which I'm pretty sure is a thing it can be.

@yutakahirano
Copy link
Member Author

This doesn't work when resource is a session history entry, which I'm pretty sure is a thing it can be.

When resource is a session history entry, no substep in

  1. This is the step that attempts to obtain resource, if necessary. Jump to the first appropriate substep:

works, so I think that's out-of-scope? Or are you asking me to fix the existing spec first?

@annevk
Copy link
Member

annevk commented Apr 10, 2018

I'll defer to @domenic I suppose. But this is the main problem I had with adding more features on top of navigation, before fixing the specification. It becomes rather unclear how everything works since the current algorithm is broken and not fully tested. It's like building a house on a swamp or some such.

@domenic
Copy link
Member

domenic commented Apr 10, 2018

I'm fine filing bugs about broken navigation cases and letting this move forward.

@yutakahirano yutakahirano mentioned this pull request Apr 12, 2018
Closed
@yutakahirano
Copy link
Member Author

I'm fine filing bugs about broken navigation cases and letting this move forward.

Done.

@yutakahirano
Copy link
Member Author

#3625

@yutakahirano yutakahirano mentioned this pull request Apr 19, 2018
Merged
@annevk annevk requested a review from domenic April 24, 2018 10:16
domenic
domenic approved these changes Apr 25, 2018
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit

source Outdated
@@ -81850,7 +81855,7 @@ interface <dfn>Location</dfn> { // but see also <a href="#the-location-interface
<span>prompt to unload</span> algorithm.</p></li>
<!-- https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=1946 to 1955 -->

<li id="navigate-fragid-step"><p>If this is not a <dfn>reload-triggered navigation</dfn>,
<li id="navigate-fragid-step"><p>If this is not a <span>reload-triggered navigation</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comma here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 27, 2018
@yutakahirano @annevk
Add tests for Request.isReloadNavigation
58ee169 
See whatwg/fetch#685, whatwg/html#3592, and discussion in w3c/ServiceWorker#1167.
annevk pushed a commit to whatwg/fetch that referenced this pull request Apr 27, 2018
@yutakahirano @annevk
Add Request's isReloadNavigation
42ae7a1 
And also a member on the request concept (reload-navigation flag) to support this API. See w3c/ServiceWorker#1167 for the discussion that led to this.

Tests: web-platform-tests/wpt#10192.

Corresponding HTML change: whatwg/html#3592.
@annevk annevk merged commit 3072f1d into whatwg:master Apr 27, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 2, 2018
@yutakahirano @jgraham
Bug 1449107 [wpt PR 10192] - Add tests for Request.isReloadNavigation…
acd8e36 
…, 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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
@marco-c
Bug 1449107 [wpt PR 10192] - Add tests for Request.isReloadNavigation…
b258e0f 
…, 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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
@marco-c
Bug 1449107 [wpt PR 10192] - Add tests for Request.isReloadNavigation…
26c7d04 
…, 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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
@marco-c
Bug 1449107 [wpt PR 10192] - Add tests for Request.isReloadNavigation…
8f14926 
…, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk left review comments

@domenic domenic domenic approved these changes

Assignees
No one assigned
Labels
topic: navigation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yutakahirano @annevk @domenic