Conversation
|
See w3c/wptserve#112 for what is blocking tests. See web-platform-tests/wpt@e94ccfe for some test sketches. The change shouldn't affect any JavaScript API behavior. They will still expose header names as lowercase even though the case given by the developer is used on the network. (Though see whatwg/xhr#108 and whatwg/xhr#109 for discussions on that front with regards to XMLHttpRequest, which ideally is somewhat aligned.) If we want a discussion on whether to allow case normalization for known headers to happen (Firefox and Safari do this) we should do that through a separate issue. For now I noted it in the source that this is a thing. I would appreciate review from @youennf and @jdm. You can view the specification with this PR applied at https://fetch.spec.whatwg.org/branch-snapshots/annevk/header-names/. Hopefully that simplifies reviewing. (Click in the title of the red blob to hide it.) |
|
|
http header names are case insensitive, and h2 forces them lower case. seems weird to have case sensitive comparisons in that case |
|
@mcmanus the only thing this patch does is to preserve the case, as enough users of h1 complained that lowercasing header names didn't work for them (Firefox never implemented that bit of Fetch). Comparisons are done using byte case-insensitive comparison, as is called out throughout this patch. Is there a particular instance where this is done wrong? |
|
Its all good - I misread part of the patch |
| <p class="note no-backref">This reuses the casing of the <a for=header>name</a> of the | ||
| <a>header</a> already in the <a for=/>header list</a>, if any. If there are multiple matched | ||
| <a>headers</a> their <a for=header>names</a> will all be identical. | ||
| <!-- XXX Firefox and Safari adjust known header names too. --> |
There was a problem hiding this comment.
I like the simplicity that provided case-insensitive header names to the spec.
Maybe a serialization context could have stored the casing information to leave the header names case-insensitive.
But anyway, this looks good to me as the PR is taking great care of making the specific casing unobservable from JS scripts.
I would tend to write the above statements as SHOULD/MUST/MAY, something like:
All matching headers must use the same casing.
The casing of the first header already in the list should be used for all matching headers.
The user agent may use its own casing for well-known headers.
There was a problem hiding this comment.
Note that all browsers currently make it observable through getAllResponseHeaders() so we might still get stuck there one way or another.
If we are to allow the user agent to pick casing for well-known headers I think we should probably standardize that and get everyone on board. Seems like it would be best done as a new issue.
There was a problem hiding this comment.
Isn't getAllResponseHeaders outputting lowercase header names with the proposed PR?
There was a problem hiding this comment.
It currently is, yes, but whatwg/xhr#109 is still unresolved and nobody implements lowercasing there.
fetch.bs
Outdated
| <a for=header>combined value</a> given <var>name</var> and | ||
| <var>list</var>. | ||
|
|
||
| <li><p>Set <var>name</var> to <var>name</var>, <a>byte-lowercased</a>. |
There was a problem hiding this comment.
Shouldn't we lowercase name before sorting? Imagine headers contains "a", "B", "c" and "D" as names. names will be ["B", "D", "a", "c"] and after lowercased it will be ["b", "d", "a", "c"].
| <a for="header list">does not contain</a> `<code>Accept-Language</code>`, user agents should | ||
| <a for="header list">append</a> | ||
| `<code>Accept-Language</code>`/an appropriate <a for=header>value</a> | ||
| to <var>request</var>'s <a for=request>header list</a>. |
There was a problem hiding this comment.
Should "hint_name is not in request's header list" below be fixed?
|
Thanks @yutakahirano, I believe I addressed your feedback. |
|
I'm assuming the text looks okay to everyone now. I'm mainly waiting on tests and the infrastructure needed for that. |
|
Ended up filing https://bugs.webkit.org/show_bug.cgi?id=168768 against WebKit. |
Adapt to whatwg/fetch#476, which basically says a header list should not lowercase the header names it is passed, but rather only do that in the "sort and combine" algorithm. This means several test expectations had to be updated because casing _will_ be preserved when a header is set. To accommodate the change, turn FetchHeaderList::header_list_ into an std::multimap with a custom comparison function in order to implement the concept of multiple headers with possibly different casing that should all be treated as the same header and kept sorted. Special care has been taken not to change FetchHeaderList's API unless absolutely necessary, but simply to avoid making the diff needlessly large. BUG=687155 R=haraken@chromium.org,tyoshino@chromium.org,yhirano@chromium.org Review-Url: https://codereview.chromium.org/2787003002 Cr-Commit-Position: refs/heads/master@{#465214}
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
This is unfortunately impossible to test, since the Node.js HTTP library lower-cases all incoming headers. However, this matters for outgoing HTTP requests. See the linked issues from the linked Fetch Standard pull request. See: whatwg/fetch#476
Instead compare header names using a byte-case-insensitive match and
lowercase them when exposed through the API.
Fixes #203 and fixes #304.