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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by Raphael Kubo da Costa (rakuco)
Modified:
4 months 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)
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
4 months, 2 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 ...
4 months, 2 weeks 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: ...
4 months, 2 weeks 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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: ...
4 months, 1 week 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>>? ...
4 months, 1 week 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: ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week 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: ...
4 months, 1 week 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 ...
4 months, 1 week 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 ...
4 months, 1 week ago (2017-04-11 15:05:54 UTC) #45
yhirano
lgtm
4 months 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 = ...
4 months 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 ...
4 months 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 ...
4 months 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 > ...
4 months 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?
4 months 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 ...
4 months ago (2017-04-12 10:37:12 UTC) #56
Raphael Kubo da Costa (rakuco)
ping?
4 months 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 ...
4 months 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: ...
4 months ago (2017-04-14 11:36:31 UTC) #61
Mike West
RS LGTM for BUILD.gn change.
4 months 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
4 months 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)
4 months 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
4 months ago (2017-04-18 11:49:22 UTC) #71
commit-bot: I haz the power
4 months 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 b40b6558b