Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(319)

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

Created:
1 year, 1 month ago by Raphael Kubo da Costa (rakuco)
Modified:
1 year, 1 month 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

Messages

Total messages: 74 (44 generated)
Raphael Kubo da Costa (rakuco)
PTAL. falken, nhiroki for //content/browser/service_worker haraken for //third_party/WebKit/Source/modules tyoshino, yhirano for //third_party/WebKit/Source/modules/fetch
1 year, 1 month 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 ...
1 year, 1 month ago (2017-03-31 00:06:28 UTC) #15
Raphael Kubo da Costa (rakuco)
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: ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month ago (2017-04-06 08:06:21 UTC) #22
Raphael Kubo da Costa (rakuco)
On 2017/04/06 08:06:21, haraken wrote: > On 2017/04/06 08:01:47, yhirano wrote: > > Sorry for ...
1 year, 1 month ago (2017-04-06 17:48:45 UTC) #23
falken
Note: I was going to ask about the web exposed change. It looks like the ...
1 year, 1 month 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: ...
1 year, 1 month ago (2017-04-07 01:48:29 UTC) #25
Raphael Kubo da Costa (rakuco)
On 2017/04/07 01:48:29, haraken wrote: > What kind of bookkeeping is needed with HashMap<String, Vector<String>>? ...
1 year, 1 month 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: ...
1 year, 1 month ago (2017-04-07 09:27:16 UTC) #27
Raphael Kubo da Costa (rakuco)
Patch v4 contains a smaller version of comment #26 explaining why we had to use ...
1 year, 1 month 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 ...
1 year, 1 month ago (2017-04-11 14:03:05 UTC) #40
Raphael Kubo da Costa (rakuco)
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: ...
1 year, 1 month ago (2017-04-11 14:22:19 UTC) #41
Raphael Kubo da Costa (rakuco)
Patch v7 is up for review: I've adjusted the 'const auto&' -> 'auto' bits, use ...
1 year, 1 month 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 ...
1 year, 1 month ago (2017-04-11 15:05:54 UTC) #45
yhirano
lgtm
1 year, 1 month 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 = ...
1 year, 1 month 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 ...
1 year, 1 month ago (2017-04-12 06:58:55 UTC) #50
Raphael Kubo da Costa (rakuco)
On 2017/04/12 06:58:55, nhiroki wrote: > On 2017/04/07 01:43:59, falken wrote: > > Note: I ...
1 year, 1 month ago (2017-04-12 09:17:02 UTC) #51
Raphael Kubo da Costa (rakuco)
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 > ...
1 year, 1 month ago (2017-04-12 09:27:55 UTC) #52
Raphael Kubo da Costa (rakuco)
haraken, can I have your l-g-t-m as a Source/modules owner?
1 year, 1 month ago (2017-04-12 09:28:17 UTC) #53
Raphael Kubo da Costa (rakuco)
On 2017/04/07 01:43:59, falken wrote: > Note: I was going to ask about the web ...
1 year, 1 month ago (2017-04-12 10:37:12 UTC) #56
Raphael Kubo da Costa (rakuco)
ping?
1 year, 1 month ago (2017-04-14 08:05:19 UTC) #59
falken
On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote: > ping? sorry i didn't mean ...
1 year, 1 month ago (2017-04-14 10:05:49 UTC) #60
Raphael Kubo da Costa (rakuco)
On 2017/04/14 10:05:49, falken wrote: > On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote: ...
1 year, 1 month ago (2017-04-14 11:36:31 UTC) #61
Mike West
RS LGTM for BUILD.gn change.
1 year, 1 month 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 year, 1 month 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 year, 1 month 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 year, 1 month ago (2017-04-18 11:49:22 UTC) #71
commit-bot: I haz the power
1 year, 1 month 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...

Powered by Google App Engine
This is Rietveld 408576698