Test when a SW replies with a fetch from a different URL #14288
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The resulting document's location should be the originally requested URL.
mfalken
reviewed
Nov 30, 2018
There was a problem hiding this comment.
Thanks! I'm somewhat surprised (but not very surprised) that we didn't' have test coverage for this, but I indeed can't find an existing test.
Maybe update the commit description first sentence to mention this is a test for navigations.
It'd also be worth having tests that combine redirects + fetch(other-url). E.g., url1 redirects to url2 and SW responds with fetch to url3... the location should be url2. But it doesn't have to be in this CL.
| @@ -668,6 +668,24 @@ | |||
| 'SW-fetched redirect to other-origin in-scope.'); | |||
|
|
|||
|
|
|||
| // SW responds with a fetch from a different url. | |||
There was a problem hiding this comment.
Maybe we can explicitly say that unlike the other test cases, this isn't a redirect (given the filename and redirect_test name, it may be confusing).
There was a problem hiding this comment.
Actually, I guess it's not needed if we're going to add redirect + fetch tests, which I think we should.
| [] | ||
| ], | ||
| 'x', | ||
| 'SW-fetched response from different URL, same-origin same-scope.'); |
There was a problem hiding this comment.
Optional: but I think you can collapse this more to be formatted like lines 650-655 to save some space without exceeding 80 cols.
mfalken
approved these changes
Nov 30, 2018
| @@ -668,6 +668,24 @@ | |||
| 'SW-fetched redirect to other-origin in-scope.'); | |||
|
|
|||
|
|
|||
| // SW responds with a fetch from a different url. | |||
There was a problem hiding this comment.
Actually, I guess it's not needed if we're going to add redirect + fetch tests, which I think we should.
domenic
pushed a commit
to whatwg/html
that referenced
this pull request
Dec 18, 2018
The "URL that was originally to be fetched" isn't clear about which URL that actually was. This makes it clear that we use the request's current URL when available, or the response's URL when not. For javascript: URLs, this fixes them to match 2/3 browser behavior. 6440cca regressed the algorithm by deleting the actual usage site of the address variable. This change reintroduces the use of address, although in a simpler way, by appending address to the request's URL list, instead of using the "override URL" concept which was removed. Fixes #3953. Closes #1565 by preserving the special case; discussions there and in https://bugzilla.mozilla.org/show_bug.cgi?id=1281375#c7 indicate that the special case is more web-compatible. Tests: * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/browsers/history/the-location-interface/location_hash.html#L24 * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/semantics/embedded-content/the-iframe-element/iframe_javascript_url_01.htm * web-platform-tests/wpt#14288 * web-platform-tests/wpt#14316
mustaqahmed
pushed a commit
to mustaqahmed/html
that referenced
this pull request
Feb 15, 2019
The "URL that was originally to be fetched" isn't clear about which URL that actually was. This makes it clear that we use the request's current URL when available, or the response's URL when not. For javascript: URLs, this fixes them to match 2/3 browser behavior. 6440cca regressed the algorithm by deleting the actual usage site of the address variable. This change reintroduces the use of address, although in a simpler way, by appending address to the request's URL list, instead of using the "override URL" concept which was removed. Fixes whatwg#3953. Closes whatwg#1565 by preserving the special case; discussions there and in https://bugzilla.mozilla.org/show_bug.cgi?id=1281375#c7 indicate that the special case is more web-compatible. Tests: * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/browsers/history/the-location-interface/location_hash.html#L24 * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/semantics/embedded-content/the-iframe-element/iframe_javascript_url_01.htm * web-platform-tests/wpt#14288 * web-platform-tests/wpt#14316
mustaqahmed
pushed a commit
to mustaqahmed/html
that referenced
this pull request
Feb 15, 2019
The "URL that was originally to be fetched" isn't clear about which URL that actually was. This makes it clear that we use the request's current URL when available, or the response's URL when not. For javascript: URLs, this fixes them to match 2/3 browser behavior. 6440cca regressed the algorithm by deleting the actual usage site of the address variable. This change reintroduces the use of address, although in a simpler way, by appending address to the request's URL list, instead of using the "override URL" concept which was removed. Fixes whatwg#3953. Closes whatwg#1565 by preserving the special case; discussions there and in https://bugzilla.mozilla.org/show_bug.cgi?id=1281375#c7 indicate that the special case is more web-compatible. Tests: * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/browsers/history/the-location-interface/location_hash.html#L24 * https://github.com/web-platform-tests/wpt/blob/7ed525acfb4133c1177fdb1ff8477e2a6469e6b4/html/semantics/embedded-content/the-iframe-element/iframe_javascript_url_01.htm * web-platform-tests/wpt#14288 * web-platform-tests/wpt#14316
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The resulting document's location should be the originally requested
URL.
This goes with whatwg/html#4205. Please let me know if there's a better way to approach this.