-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Define the HTTP Refresh header #2892
Conversation
Tests: web-platform-tests/wpt#6606 |
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 although as you note in web-platform-tests/wpt#6606 there are a lot of potential tests to be written.
source
Outdated
headers.</p></li> | ||
|
||
<li><p>Let <var>value</var> be the value of the header with each byte mapped to a code point | ||
of equal value.</p></li> |
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.
It would be quite nice to test that this is the case and not, e.g., UTF-8 decoding.
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 have confirmed it locally.
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.
It's actually tested. Improved that test a bit.
@@ -115353,6 +115375,29 @@ interface <dfn>External</dfn> { | |||
</dl> | |||
|
|||
|
|||
<h3>`<dfn><code data-x="http-refresh">Refresh</code></dfn>`</h3> |
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 really thought we'd alphabetized the headers but I guess not...
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.
Do you want that done as part of this PR/commit? Happy to sort that out.
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.
Nah, we can leave it for later; I might be missing something too in how they're ordered.
source
Outdated
|
||
<ol> | ||
<li><p class="XXX">Multiple `<code data-x="http-refresh">Refresh</code>` | ||
headers.</p></li> |
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.
Shall I open an issue for this and point to that from here? Might be slightly better.
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.
Still LGTM
Tests for whatwg/html#2892.
@annevk Do you know whether there are bugs tracking browser compliance here? |
@bzbarsky I didn't spot browser differences other than in the parser, which I think is always shared with |
No, that's it. Thank you! |
Tests for whatwg/html#2892.
Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28339 and fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28563.