Chromium Code Reviews
Help | Chromium Project | Sign in
(26)

Issue 2787003002: Fetch API: Stop lowercasing header names. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 weeks ago by rakuco (OoO until May 1st)
Modified:
1 week, 2 days ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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} Committed: https://chromium.googlesource.com/chromium/src/+/f8ebda9548f2a4bfc00d414f48512327b2d656d7

Patch Set 1 #

Patch Set 2 : Fix failing tests #

Total comments: 2

Patch Set 3 : Use equalIgnoringASCIICase in cachestorage #

Patch Set 4 : Explain why we need to use an std::multimap #

Patch Set 5 : Rebase after The Big Blink Rename #

Total comments: 10

Patch Set 6 : Use WTFString::LowerASCII(), use auto instead of const auto& #

Patch Set 7 : Rebase again #

Total comments: 1

Patch Set 8 : Reimplement VaryHeaderContainsAsterisk #

Patch Set 9 : Rebase for landing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+307 lines, -137 lines) Patch
M content/browser/service_worker/service_worker_browsertest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/fetch/resources/thorough-util.js View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/modules/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/cachestorage/Cache.cpp View 1 2 3 4 5 6 7 1 chunk +7 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/FetchHeaderList.h View 1 2 3 4 5 4 chunks +25 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp View 1 2 3 4 5 4 chunks +84 lines, -80 lines 0 comments Download
A third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp View 1 2 3 4 5 6 7 8 1 chunk +163 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/FetchManager.cpp View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/FetchResponseData.cpp View 1 2 3 4 5 6 7 8 3 chunks +8 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/Headers.cpp View 1 2 3 4 5 6 7 8 4 chunks +6 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/modules/fetch/Request.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 74 (44 generated)
rakuco (OoO until May 1st)
PTAL. falken, nhiroki for //content/browser/service_worker haraken for //third_party/WebKit/Source/modules tyoshino, yhirano for //third_party/WebKit/Source/modules/fetch
4 weeks ago (2017-03-30 18:06:19 UTC) #12
haraken
https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right): https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp#newcode209 third_party/WebKit/Source/modules/cachestorage/Cache.cpp:209: if (equalIgnoringCase(header.first, "vary")) { Should we probably use equalIgnoringASCIICase ...
3 weeks, 6 days ago (2017-03-31 00:06:28 UTC) #15
rakuco (OoO until May 1st)
https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right): https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp#newcode209 third_party/WebKit/Source/modules/cachestorage/Cache.cpp:209: if (equalIgnoringCase(header.first, "vary")) { On 2017/03/31 00:06:28, haraken wrote: ...
3 weeks, 6 days ago (2017-03-31 10:22:21 UTC) #16
yhirano
Sorry for the late response. This change uses std::multimap<String, String>. haraken@, is it allowed to ...
3 weeks ago (2017-04-06 08:01:47 UTC) #21
haraken
On 2017/04/06 08:01:47, yhirano wrote: > Sorry for the late response. > > This change ...
3 weeks ago (2017-04-06 08:06:21 UTC) #22
rakuco (OoO until May 1st)
On 2017/04/06 08:06:21, haraken wrote: > On 2017/04/06 08:01:47, yhirano wrote: > > Sorry for ...
3 weeks ago (2017-04-06 17:48:45 UTC) #23
falken (away until May 8)
Note: I was going to ask about the web exposed change. It looks like the ...
2 weeks, 6 days ago (2017-04-07 01:43:59 UTC) #24
haraken
On 2017/04/06 17:48:45, Raphael Kubo da Costa (rakuco) wrote: > On 2017/04/06 08:06:21, haraken wrote: ...
2 weeks, 6 days ago (2017-04-07 01:48:29 UTC) #25
rakuco (OoO until May 1st)
On 2017/04/07 01:48:29, haraken wrote: > What kind of bookkeeping is needed with HashMap<String, Vector<String>>? ...
2 weeks, 6 days ago (2017-04-07 09:24:37 UTC) #26
haraken
On 2017/04/07 09:24:37, Raphael Kubo da Costa (rakuco) wrote: > On 2017/04/07 01:48:29, haraken wrote: ...
2 weeks, 6 days ago (2017-04-07 09:27:16 UTC) #27
rakuco (OoO until May 1st)
Patch v4 contains a smaller version of comment #26 explaining why we had to use ...
2 weeks, 6 days ago (2017-04-07 09:55:47 UTC) #28
yhirano
https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp#newcode40 third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:40: const auto& header = header_list_.find(name); Using |auto| instead of ...
2 weeks, 2 days ago (2017-04-11 14:03:05 UTC) #40
rakuco (OoO until May 1st)
https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp File third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp (right): https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp#newcode27 third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:27: size_t i = 0; On 2017/04/11 14:03:05, yhirano wrote: ...
2 weeks, 2 days ago (2017-04-11 14:22:19 UTC) #41
rakuco (OoO until May 1st)
Patch v7 is up for review: I've adjusted the 'const auto&' -> 'auto' bits, use ...
2 weeks, 2 days ago (2017-04-11 15:02:05 UTC) #44
yhirano
https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp File third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp (right): https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp#newcode27 third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:27: size_t i = 0; On 2017/04/11 14:22:19, Raphael Kubo ...
2 weeks, 2 days ago (2017-04-11 15:05:54 UTC) #45
yhirano
lgtm
2 weeks, 1 day ago (2017-04-12 03:15:39 UTC) #48
nhiroki
lgtm with a minor comment https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right): https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp#newcode212 third_party/WebKit/Source/modules/cachestorage/Cache.cpp:212: for (size_t j = ...
2 weeks, 1 day ago (2017-04-12 06:53:05 UTC) #49
nhiroki
On 2017/04/07 01:43:59, falken wrote: > Note: I was going to ask about the web ...
2 weeks, 1 day ago (2017-04-12 06:58:55 UTC) #50
rakuco (OoO until May 1st)
On 2017/04/12 06:58:55, nhiroki wrote: > On 2017/04/07 01:43:59, falken wrote: > > Note: I ...
2 weeks, 1 day ago (2017-04-12 09:17:02 UTC) #51
rakuco (OoO until May 1st)
On 2017/04/12 06:53:05, nhiroki wrote: > lgtm with a minor comment > > https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Source/modules/cachestorage/Cache.cpp > ...
2 weeks, 1 day ago (2017-04-12 09:27:55 UTC) #52
rakuco (OoO until May 1st)
haraken, can I have your l-g-t-m as a Source/modules owner?
2 weeks, 1 day ago (2017-04-12 09:28:17 UTC) #53
rakuco (OoO until May 1st)
On 2017/04/07 01:43:59, falken wrote: > Note: I was going to ask about the web ...
2 weeks, 1 day ago (2017-04-12 10:37:12 UTC) #56
rakuco (OoO until May 1st)
ping?
1 week, 6 days ago (2017-04-14 08:05:19 UTC) #59
falken (away until May 8)
On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote: > ping? sorry i didn't mean ...
1 week, 6 days ago (2017-04-14 10:05:49 UTC) #60
rakuco (OoO until May 1st)
On 2017/04/14 10:05:49, falken wrote: > On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote: ...
1 week, 6 days ago (2017-04-14 11:36:31 UTC) #61
Mike West
RS LGTM for BUILD.gn change.
1 week, 2 days ago (2017-04-18 11:13:23 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2787003002/140001
1 week, 2 days ago (2017-04-18 11:15:20 UTC) #66
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/413670)
1 week, 2 days ago (2017-04-18 11:23:02 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2787003002/160001
1 week, 2 days ago (2017-04-18 11:49:22 UTC) #71
commit-bot: I haz the power
1 week, 2 days ago (2017-04-18 13:36:22 UTC) #74
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f8ebda9548f2a4bfc00d414f4851...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46