New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 700434 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
OoO until Feb 4th
Closed: May 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug

Blocked on:
issue 687155



Sign in to add a comment

Fetch: use ", " as value separator rather than ","

Project Member Reported by annevank...@gmail.com, Mar 10 2017

Issue description

The plan is to make Fetch and XMLHttpRequest as close as possible as they can be and not to have weird small divergences.

Standard change: https://github.com/whatwg/fetch/pull/504.

Tests: https://github.com/w3c/web-platform-tests/pull/5115. (There's many other tests around this too that have already landed. I suspect that any code changes would end up affecting more results than just these.)
 
Components: Blink>Network>XHR
Labels: -OS-Mac OS-All
Owner: raphael....@intel.com
Status: Started (was: Available)
I've got a patch to fix this in the Fetch code, but unfortunately XHR is handled separately and will need to be addressed later.
... though admittedly what's failing in XHR is XMLHttpRequest/getallresponseheaders-cl.htm's first test due to the lowercasing changes mentioned in  issue 687155 .

As for this issue, the XHR code already uses ", " in responses, and changing the request code is a one-liner.
Blockedon: 687155
While both issues are independent, it's easier to fix one after the other since both end up touching the same code, so I'm marking this one as dependent on  issue 687155 .
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933

commit fcbfde6ea277a07cddc3db613a3b4ff3de5d3933
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Wed Apr 19 08:58:44 2017

Fetch, XHR: Use ", " as header list value separator.

Adapt to https://github.com/whatwg/fetch/pull/504 and
https://github.com/whatwg/xhr/pull/130, which started requiring combined
header list values to be separated by ", " instead of just ",".

The XHR code handling responses already did that, but |ResourceRequest| and
|FetchHeaderList| both had to be updated.

As a bonus, this also makes us pass w-p-t's headers-combine.html.

BUG= 700434 ,705490
R=mkwst@chromium.org,tyoshino@chromium.org,yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2822183002
Cr-Commit-Position: refs/heads/master@{#465537}

[delete] https://crrev.com/14bcd90c3e83b60fb9ecce8504356b81af4c6209/third_party/WebKit/LayoutTests/external/wpt/fetch/api/headers/headers-combine-expected.txt
[modify] https://crrev.com/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933/third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js
[modify] https://crrev.com/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js
[modify] https://crrev.com/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
[modify] https://crrev.com/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp
[modify] https://crrev.com/fcbfde6ea277a07cddc3db613a3b4ff3de5d3933/third_party/WebKit/Source/platform/loader/fetch/ResourceRequest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e935156eb8bfd135d843804d2c62b2e3d742945

commit 6e935156eb8bfd135d843804d2c62b2e3d742945
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Wed Apr 19 15:38:28 2017

wpt: Roll wptserve to c7bf515d74c16320455f68b61c113a9012743ff1

git log --oneline --no-decorate 071c51e26..c7bf515d7
 c7bf515 Expose raw_headers (#113)
 662d5ed Disable E305 pyflakes lint
 6059a5e Remove a check from resolve_content() that is never hit. (#107)
 2ce4303 Use HTTPError correctly. (#106)
 8a981b7 Merge pull request #98 from w3c/six
 69298c6 Link to docs in README
 5e9f4c2 Import quote and unquote from six.
 b9b2eae Import parse_qs and urljoin from six.
 7311324 Import parse_qsl from six.
 5a3336d Import urlsplit from six.
 3157b3d Import urlunsplit from six.
 639329c Import Request from six.
 f1988dd Import urlopen from six.
 b7e6bb2 Import urlencode from six.
 411a415 Import HTTPError from six.

Commit c7bf515 ("Expose raw_headers") is required for some XMLHttpRequest
tests to pass.

Bug:  700434 
Change-Id: Ief6d4d19c1cd40ea6107eec2d1998511aa8393c0
Reviewed-on: https://chromium-review.googlesource.com/481760
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#465617}
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/getallresponseheaders-cl-expected.txt
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/WPTHeads
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/checkout.sh
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/wptserve/README.md
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/wptserve/wptserve/handlers.py
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/wptserve/wptserve/pipes.py
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/wptserve/wptserve/request.py
[modify] https://crrev.com/6e935156eb8bfd135d843804d2c62b2e3d742945/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/wptserve/wptserve/server.py

Labels: M-60
r465537 will be in M60
I think it's fine to ship this given
- returning a combined data for getAllResponseHeaders() was introduced for M57 ( bug 645497 ). It's been there for 3 milestones.
- this is subtle change

Regarding lowercasing, Raphael has started the following I2I thread.
- https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/_oxlCPNsrck (active one)
- (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/cakEmcqoWoA)

It might be good to leave this bug tracking ", -> ", " change and create a new one for lowercasing.
Status: Fixed (was: Started)
Done. I've filed  bug 716994  to track the lowercasing change. I'm also closing this one since the value separator bits have landed.
Project Member

Comment 10 by bugdroid1@chromium.org, May 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99c274ae8e7d366261dcfb89e0b98e733fb9d5f4

commit 99c274ae8e7d366261dcfb89e0b98e733fb9d5f4
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Mon May 22 13:28:21 2017

XHR: Lowercase header names returned by getAllResponseHeaders().

Adapt to https://github.com/whatwg/xhr/issues/109 and its related
https://github.com/whatwg/xhr/commit/ecce3904ace, which aligned some of
XHR's behavior with Fetch's.

Specifically in this case, getAllResponseHeaders() is supposed to execute
Fetch's "sort and combine" steps, which include lowercasing all header
names.

Note that this commit does not fully implement the "sort" bit: that one is
currently done only in the Fetch code, and we should instead move that part
of the code to core/ or platform/loader/ instead.

Intent to implement thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/_oxlCPNsrck/2FePT4QaBwAJ

BUG= 700434 
R=mkwst@chromium.org,tyoshino@chromium.org,yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2828753002
Cr-Commit-Position: refs/heads/master@{#473567}

[modify] https://crrev.com/99c274ae8e7d366261dcfb89e0b98e733fb9d5f4/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/50bbde70a79349d7c71655cb1042fa559fc4a568/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/getallresponseheaders-cl-expected.txt
[modify] https://crrev.com/99c274ae8e7d366261dcfb89e0b98e733fb9d5f4/third_party/WebKit/LayoutTests/external/wpt/XMLHttpRequest/getallresponseheaders-expected.txt
[modify] https://crrev.com/99c274ae8e7d366261dcfb89e0b98e733fb9d5f4/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-data-url.html
[modify] https://crrev.com/99c274ae8e7d366261dcfb89e0b98e733fb9d5f4/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp

Sign in to add a comment