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

Issue 687155 link

Starred by 4 users

Issue metadata

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

Blocking:
issue 700434



Sign in to add a comment

Stop lowercasing header names

Project Member Reported by annevank...@gmail.com, Jan 31 2017

Issue description

See https://github.com/whatwg/fetch/pull/476 and referenced issues and please provide input if you have any. It seems Chrome is currently the only browser that lowercases so most likely to run into issues.
 
Cc: tzik@chromium.org
Status: Available (was: Untriaged)
Owner: raphael....@intel.com
Status: Started (was: Available)
It looks like the WPT tests for this spec change are under XMLHttpRequest, which we don't currently import:
https://github.com/w3c/web-platform-tests/commit/0a3f56149526619aaf9bd60af66c008df9eab0c5

Could we pull in those tests?
Cc: jsb...@chromium.org
There's also https://github.com/w3c/web-platform-tests/pull/5115, but that one also requires  issue 700434 .

W3CImportExpectations shows jsbell@ is the owner for external/wpt/XMLHttpRequest, so I guess it's up to him to import them? I also mentioned in issue 705490 that we need at least part of that directory for some Fetch tests to pass as well.
Blocking: 700434
Labels: Hotlist-Interop
Components: Blink>Infra>Predictability
The # Owners annotation just applies to the #...[Skip] line right below it. So it's not me. :)

Anyone can be the "owner", just need to comment out the [Skip] line and wait for the next import. Imported tests that fail should automatically get appropriate expectations added.

Sorry for the confusion :-) I've finally uploaded a CL to import the XHR tests: https://codereview.chromium.org/2808403003/
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 12 2017

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

commit 5bc8d44f3ab226674d0dd2c52ef631d6d52dc464
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Wed Apr 12 15:06:27 2017

Import XMLHttpRequest wpt tests.

Some Fetch API tests require bits from the /XMLHttpRequest directory. Some
of the tests are also required for correctly testing behavior after changes
to the Fetch and XHR specs.

BUG= 687155 ,705490
R=falken@chromium.org,tyoshino@chromium.org,yhirano@chromium.org

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

[modify] https://crrev.com/5bc8d44f3ab226674d0dd2c52ef631d6d52dc464/third_party/WebKit/LayoutTests/W3CImportExpectations

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 18 2017

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

commit f8ebda9548f2a4bfc00d414f48512327b2d656d7
Author: raphael.kubo.da.costa <raphael.kubo.da.costa@intel.com>
Date: Tue Apr 18 13:36:04 2017

Fetch API: Stop lowercasing header names.

Adapt to https://github.com/whatwg/fetch/pull/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}

[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/content/browser/service_worker/service_worker_browsertest.cc
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/BUILD.gn
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/cachestorage/Cache.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/FetchHeaderList.h
[add] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/Headers.cpp
[modify] https://crrev.com/f8ebda9548f2a4bfc00d414f48512327b2d656d7/third_party/WebKit/Source/modules/fetch/Request.cpp

Status: Fixed (was: Started)
Status: Started (was: Fixed)
Is this fixed?

external/wpt/XMLHttpRequest/getallresponseheaders-cl.htm is still failing:

FAIL Casing of known headers assert_equals: expected "content-length: 0\r\n" but got "CONTENT-LENGTH: 0\r\n"
PASS Casing of known headers 1 
PASS Casing of known headers 2 
Harness: the test ran to completion.

That is a test for how response headers are exposed through the API. This bug is about not lowercasing request header names.
Well I'm confused, is there a web platform test that Chrome previously failed on and then started passing after comment 11 landed?
The two subtests that PASS didn't both PASS I think. In particular the last one used to fail due to fetch() lowercasing request header names. I was just explaining the FAIL case.
Status: Fixed (was: Started)
I see, an additional infra change was needed to get those test cases to pass:
https://chromium-review.googlesource.com/c/481760/

I think comment 11 plus that change got the tests to pass.
Labels: M-60
As for the remaining failure: see  issue 716994 .

Sign in to add a comment