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.

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

Merged
merged 3 commits into from
Aug 9, 2017
Merged

Define the HTTP Refresh header #2892

merged 3 commits into from
Aug 9, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented Aug 4, 2017

@annevk
Copy link
Member Author

annevk commented Aug 4, 2017

Tests: web-platform-tests/wpt#6606

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 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>
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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>
Copy link
Member

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...

Copy link
Member Author

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.

Copy link
Member

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="&#x0058;&#x0058;&#x0058;">Multiple `<code data-x="http-refresh">Refresh</code>`
headers.</p></li>
Copy link
Member Author

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.

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.

Still LGTM

jgraham pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 9, 2017
@annevk annevk merged commit 7e9f6b6 into master Aug 9, 2017
@annevk annevk deleted the annevk/refresh branch August 9, 2017 16:57
@bzbarsky
Copy link
Contributor

@annevk Do you know whether there are bugs tracking browser compliance here?

@annevk
Copy link
Member Author

annevk commented Aug 10, 2017

@bzbarsky I didn't spot browser differences other than in the parser, which I think is always shared with <meta http-equiv=refresh> and I filed bugs for those (see #2844 (comment)). And I separately filed a bug against Firefox to decide on multiple header handling, which is also tracked by #2900. Did you notice something else?

@bzbarsky
Copy link
Contributor

No, that's it. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants