-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fetch: fragments in responses #7261
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % trouble of understanding redirect.py
return fetch("./#test").then(res => { | ||
assert_equals(res.url, new URL("./", location).href); | ||
return fetch("../resources/redirect.py?location=/#test").then(res2 => { | ||
assert_equals(res2.url, new URL("/?location=%2F&count=1", location).href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit took a while to understand, but I think it's correct. You really have to read redirect.py to make sense of it though, first I assumed redirect.py would simply redirect. Maybe comment "/redirect.py redirects to this funny URL, and the UA should not append a fragment"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
Hmm, I should maybe also test a redirect to a URL that includes a fragment, for completeness. |
@@ -0,0 +1,7 @@ | |||
<p><img src=/images/green.svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this isn't ready for review yet since this isn't used, will hold off until poked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking of using a reference test for this, but I first need to determine if everyone is okay with passing along fragments all the time.
assert_equals(res2.url, new URL("/?location=%2F&count=1", location).href); | ||
// redirect.py ends up appending some stuff to the fragment, but that does not affect this | ||
return fetch("../resources/redirect.py?location=/%23test").then(res3 => { | ||
assert_equals(res3.url, new URL("/", location).href); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised here, but I guess the deal is that fragments on redirects are not preserved and if they're included they're thrown away, except for navigations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the current deal, yes. Perhaps we should alter the deal though.
f96f44a
to
62a5aad
Compare
}); | ||
self.addEventListener("load", t.step_func_done(() => { | ||
ctx.drawImage(img, 0, 0); | ||
assert_true(isColor(ctx.getImageData(0, 0, 100, 25).data, 0, 255, 0, 255)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this bit could use a comment to explain what the structure of the image is and why these asserts would fail if something were wrong with how fragments are handled on subresource redirects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an explanation.
Tests for whatwg/fetch#505.
e64ef83
to
3714176
Compare
@@ -4,6 +4,17 @@ | |||
<script src=/resources/testharness.js></script> | |||
<script src=/resources/testharnessreport.js></script> | |||
<div id=log></div> | |||
<!-- | |||
The source image is 50h x 100w lime green. However, if the viewBox 25,25,50,50 is correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great documentation, thanks!
img.remove(); | ||
ctx.clearRect(0, 0, 100, 50); | ||
}); | ||
self.addEventListener("load", t.step_func_done(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Road
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event listener
Test for whatwg/fetch#1167.