Fetch API: Stop lowercasing header names.

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

[Raphael Kubo da Costa (rakuco), 2017-03-30 16:43:09]

Description was changed from

==========
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.

To accommodate the change, turn FetchHaderList::m_headerList 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.

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

to

==========
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.

To accommodate the change, turn FetchHaderList::m_headerList 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.

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

[Raphael Kubo da Costa (rakuco), 2017-03-30 16:43:10]

raphael.kubo.da.costa@intel.com changed reviewers:
+ haraken@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-03-30 16:43:18]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 16:43:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-03-30 16:46:13]

Description was changed from

==========
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.

To accommodate the change, turn FetchHaderList::m_headerList 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.

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

to

==========
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.

To accommodate the change, turn FetchHeaderList::m_headerList 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
==========

[commit-bot: I haz the power, 2017-03-30 17:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 17:13:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raphael Kubo da Costa (rakuco), 2017-03-30 18:03:59]

Description was changed from

==========
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.

To accommodate the change, turn FetchHeaderList::m_headerList 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
==========

to

==========
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::m_headerList 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
==========

[Raphael Kubo da Costa (rakuco), 2017-03-30 18:04:00]

raphael.kubo.da.costa@intel.com changed reviewers:
+ falken@chromium.org, nhiroki@chromium.org

[Raphael Kubo da Costa (rakuco), 2017-03-30 18:04:12]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:04:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-03-30 18:06:19]

PTAL.

falken, nhiroki for //content/browser/service_worker
haraken for //third_party/WebKit/Source/modules
tyoshino, yhirano for //third_party/WebKit/Source/modules/fetch

[commit-bot: I haz the power, 2017-03-30 21:29:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 21:29:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[haraken, 2017-03-31 00:06:28]

https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:209: if
(equalIgnoringCase(header.first, "vary")) {

Should we probably use equalIgnoringASCIICase instead?

[Raphael Kubo da Costa (rakuco), 2017-03-31 10:22:21]

https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/2787003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:209: if
(equalIgnoringCase(header.first, "vary")) {
On 2017/03/31 00:06:28, haraken wrote:
> 
> Should we probably use equalIgnoringASCIICase instead?

Sure. I've done that in patch v3.

This actually reminds me if the new implementation of
FetchHeaderList::sortAndCombine() looks OK: there's no WTFString::lowerASCII(),
so I end up calling WTFString::lower() even though the spec says the header
names should only be "byte-lowercased".

[Raphael Kubo da Costa (rakuco), 2017-03-31 10:22:29]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 10:22:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 12:27:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 12:27:40]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-04-06 08:01:47]

Sorry for the late response.

This change uses std::multimap<String, String>. haraken@, is it allowed to use a
stl-container with a blink type?

[haraken, 2017-04-06 08:06:21]

On 2017/04/06 08:01:47, yhirano wrote:
> Sorry for the late response.
> 
> This change uses std::multimap<String, String>. haraken@, is it allowed to use
a
> stl-container with a blink type?

Yeah, we prefer using HashMap<String, HashSet<String>>.

[Raphael Kubo da Costa (rakuco), 2017-04-06 17:48:45]

On 2017/04/06 08:06:21, haraken wrote:
> On 2017/04/06 08:01:47, yhirano wrote:
> > Sorry for the late response.
> > 
> > This change uses std::multimap<String, String>. haraken@, is it allowed to
use
> a
> > stl-container with a blink type?
> 
> Yeah, we prefer using HashMap<String, HashSet<String>>.

I was under the impression that HashMap was just a map, not a multimap. I
originally tried using HashMap<String, Vector<String>>. While it is possible to
do that, it means one has to do some additional bookkeeping that is done
automatically by std::multimap, and implementing FetchHeaderList::list() with a
HashMap means either converting that into a more suitable data structure on
every invocation or requiring all callers to do additional work.

[falken, 2017-04-07 01:43:59]

Note: I was going to ask about the web exposed change. 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/0a3f56149526619aaf9bd60af66c...

We should import the XMLHttpRequest tests.

[haraken, 2017-04-07 01:48:29]

On 2017/04/06 17:48:45, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/06 08:06:21, haraken wrote:
> > On 2017/04/06 08:01:47, yhirano wrote:
> > > Sorry for the late response.
> > > 
> > > This change uses std::multimap<String, String>. haraken@, is it allowed to
> use
> > a
> > > stl-container with a blink type?
> > 
> > Yeah, we prefer using HashMap<String, HashSet<String>>.
> 
> I was under the impression that HashMap was just a map, not a multimap. I
> originally tried using HashMap<String, Vector<String>>. While it is possible
to
> do that, it means one has to do some additional bookkeeping that is done
> automatically by std::multimap, and implementing FetchHeaderList::list() with
a
> HashMap means either converting that into a more suitable data structure on
> every invocation or requiring all callers to do additional work.

What kind of bookkeeping is needed with HashMap<String, Vector<String>>?

I agree that in general std::multimap would be more convenient, but I'm a bit
hesitating to introducing a new data structure to Blink unless we have a strong
reason to do so.

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:24:37]

On 2017/04/07 01:48:29, haraken wrote:
> What kind of bookkeeping is needed with HashMap<String, Vector<String>>?

The additional bookkeeping I mentioned is due to the fact that std::multimap is
"flat": its size() corresponds to the total number of headers and values on the
list, whereas the size() of a HashMap<String, Vector<String>> corresponds to the
number of unique header _names_ on the list.

In practice, this means:
- FHL::size() must always iterate through all HashMap keys and get the size of
each list corresponding to a key to return a final size.
- FHL::entry() returning a HashMap means callers who need to iterate through all
headers and values need to iterate through each key and then iterate through the
Vector<String> corresponding to a key.
- FHL::containsNonSimpleHeaders() must do something similar to the above.
- This is minor, but the std::multimap is always sorted, whereas with a HashMap
FHL::sortAndCombine() also needs to sort the keys before merging the values
(just like the existing code does).
- I'm not sure about this one, but I don't know if HashMap's keys and iterators
are always returned in the same order, which can introduce flakiness in tests
where callers expect headers to be returned in a certain order.

So yeah, it is possible to use a HashMap with all values stored in a Vector (not
a Set, as we need them to be ordered), I'd just like to make sure the
constraints I mentioned above are OK -- a lot of the issues above can be solved
by changing FHL's API or returning a "Blink-friendly" data structure in
FHL::entry() while internally using a multimap.

[haraken, 2017-04-07 09:27:16]

On 2017/04/07 09:24:37, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/07 01:48:29, haraken wrote:
> > What kind of bookkeeping is needed with HashMap<String, Vector<String>>?
> 
> The additional bookkeeping I mentioned is due to the fact that std::multimap
is
> "flat": its size() corresponds to the total number of headers and values on
the
> list, whereas the size() of a HashMap<String, Vector<String>> corresponds to
the
> number of unique header _names_ on the list.
> 
> In practice, this means:
> - FHL::size() must always iterate through all HashMap keys and get the size of
> each list corresponding to a key to return a final size.
> - FHL::entry() returning a HashMap means callers who need to iterate through
all
> headers and values need to iterate through each key and then iterate through
the
> Vector<String> corresponding to a key.
> - FHL::containsNonSimpleHeaders() must do something similar to the above.
> - This is minor, but the std::multimap is always sorted, whereas with a
HashMap
> FHL::sortAndCombine() also needs to sort the keys before merging the values
> (just like the existing code does).
> - I'm not sure about this one, but I don't know if HashMap's keys and
iterators
> are always returned in the same order, which can introduce flakiness in tests
> where callers expect headers to be returned in a certain order.
> 
> So yeah, it is possible to use a HashMap with all values stored in a Vector
(not
> a Set, as we need them to be ordered), I'd just like to make sure the
> constraints I mentioned above are OK -- a lot of the issues above can be
solved
> by changing FHL's API or returning a "Blink-friendly" data structure in
> FHL::entry() while internally using a multimap.

Thanks for the clarification. I now understand that not using std::multimap will
lead to a lot of complexity.

I'm fine with using std::multimap here if we add a good comment about why we
wanted to use it.

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:55:47]

Patch v4 contains a smaller version of comment #26 explaining why we had to use
std::multimap here.

[Raphael Kubo da Costa (rakuco), 2017-04-07 09:55:54]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 09:56:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 11:36:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 11:36:39]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-10 10:32:55]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-10 10:33:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-04-10 11:09:54]

Description was changed from

==========
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::m_headerList 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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-04-10 12:28:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-10 12:28:33]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-10 14:51:15]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-10 14:51:19]

The CQ bit was unchecked by raphael.kubo.da.costa@intel.com

[yhirano, 2017-04-11 14:03:05]

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right):

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:40: const auto&
header = header_list_.find(name);
Using |auto| instead of |const auto&| is easier to understand in this case, I
think.

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:55: const auto&
existingHeader = header_list_.find(name);
ditto

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:96: const auto&
range = header_list_.equal_range(name);
ditto

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:115: const auto&
range = header_list_.equal_range(name);
ditto

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp (right):

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:27: size_t i =
0;
Unused

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:50: size_t i =
0;
ditto

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:72: size_t i =
0;
ditto

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:154: size_t i =
0;
Unused

[Raphael Kubo da Costa (rakuco), 2017-04-11 14:22:19]

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp (right):

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:27: size_t i =
0;
On 2017/04/11 14:03:05, yhirano wrote:
> Unused

Hm? This is being used to iterate through |expectedHeaders| in the loop below
(the same applies to the other comments in this file).

[Raphael Kubo da Costa (rakuco), 2017-04-11 15:01:12]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 15:01:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-04-11 15:02:05]

Patch v7 is up for review: I've adjusted the 'const auto&' -> 'auto' bits, use
cbegin/cend instead of begin/end where possible and started using
WTFString::LowerASCII() which was added by
https://codereview.chromium.org/2814673002

[yhirano, 2017-04-11 15:05:54]

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp (right):

https://codereview.chromium.org/2787003002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchHeaderListTest.cpp:27: size_t i =
0;
On 2017/04/11 14:22:19, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/11 14:03:05, yhirano wrote:
> > Unused
> 
> Hm? This is being used to iterate through |expectedHeaders| in the loop below
> (the same applies to the other comments in this file).

Ah, sorry, I misunderstood.

[commit-bot: I haz the power, 2017-04-11 17:05:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 17:05:33]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-04-12 03:15:39]

lgtm

[nhiroki, 2017-04-12 06:53:05]

lgtm with a minor comment

https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/cachestorage/Cache.cpp:212: for (size_t j = 0;
j < fields.size(); ++j) {
Can you replace this with range-based-for?

[nhiroki, 2017-04-12 06:58:55]

On 2017/04/07 01:43:59, falken wrote:
> Note: I was going to ask about the web exposed change. 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/0a3f56149526619aaf9bd60af66c...
> 
> We should import the XMLHttpRequest tests.

rakuco, did you check this change passes the wpt tests? It'd be nice if we can
import them before landing this change so that we can confirm this doesn't break
other things.

[Raphael Kubo da Costa (rakuco), 2017-04-12 09:17:02]

On 2017/04/12 06:58:55, nhiroki wrote:
> On 2017/04/07 01:43:59, falken wrote:
> > Note: I was going to ask about the web exposed change. 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/0a3f56149526619aaf9bd60af66c...
> > 
> > We should import the XMLHttpRequest tests.
> 
> rakuco, did you check this change passes the wpt tests? It'd be nice if we can
> import them before landing this change so that we can confirm this doesn't
break
> other things.

I did. This CL is only a first step towards getting all tests to pass, as they
require a few other fixes (see bug 700434, for example).

[Raphael Kubo da Costa (rakuco), 2017-04-12 09:27:55]

On 2017/04/12 06:53:05, nhiroki wrote:
> lgtm with a minor comment
> 
>
https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/cachestorage/Cache.cpp (right):
> 
>
https://codereview.chromium.org/2787003002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/cachestorage/Cache.cpp:212: for (size_t j =
0;
> j < fields.size(); ++j) {
> Can you replace this with range-based-for?

I've uploaded patch v8, where I replaced all the fors in that function with
Get() + std::any_of().

[Raphael Kubo da Costa (rakuco), 2017-04-12 09:28:17]

haraken, can I have your l-g-t-m as a Source/modules owner?

[Raphael Kubo da Costa (rakuco), 2017-04-12 09:28:24]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 09:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-04-12 10:37:12]

On 2017/04/07 01:43:59, falken wrote:
> Note: I was going to ask about the web exposed change. 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/0a3f56149526619aaf9bd60af66c...
> 
> We should import the XMLHttpRequest tests.

Finally done in https://codereview.chromium.org/2808403003/

[commit-bot: I haz the power, 2017-04-12 11:17:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 11:17:46]

Dry run: This issue passed the CQ dry run.

[Raphael Kubo da Costa (rakuco), 2017-04-14 08:05:19]

ping?

[falken, 2017-04-14 10:05:49]

On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote:
> ping?

sorry i didn't mean to block this CL and i think you have the needed lg-tms so
dont need mine. thanks for importing the WPT test. i think you may need to
rebase and upload the new test expectation for the test.

[Raphael Kubo da Costa (rakuco), 2017-04-14 11:36:31]

On 2017/04/14 10:05:49, falken wrote:
> On 2017/04/14 08:05:19, Raphael Kubo da Costa (rakuco) wrote:
> > ping?
> 
> sorry i didn't mean to block this CL and i think you have the needed lg-tms so
> dont need mine. thanks for importing the WPT test. i think you may need to
> rebase and upload the new test expectation for the test.

Sorry, this wasn't a ping directly at you :-) I'm still missing an l-g-t-m for
Source/modules/BUILD.gn. I did rebase my branch locally and ran the XHR layout
tests. No expectations needed to change, as some of the tests will only pass
once I land my other, related fixes.

[Mike West, 2017-04-18 11:13:22]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2017-04-18 11:13:23]

RS LGTM for BUILD.gn change.

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:15:07]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:15:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2787003002/#ps140001
(title: "Reimplement VaryHeaderContainsAsterisk")

[commit-bot: I haz the power, 2017-04-18 11:15:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 11:23:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 11:23:02]

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_presub...)

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:49:09]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-18 11:49:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, mkwst@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2787003002/#ps160001
(title: "Rebase for landing")

[commit-bot: I haz the power, 2017-04-18 11:49:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 13:36:08]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1492516149612700,
"parent_rev": "7affbfc7f68bd3d39cdc111f70779cccdb750689", "commit_rev":
"f8ebda9548f2a4bfc00d414f48512327b2d656d7"}

[commit-bot: I haz the power, 2017-04-18 13:36:19]

Description was changed from

==========
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
==========

to

==========
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/+/f8ebda9548f2a4bfc00d414f4851...
==========

[commit-bot: I haz the power, 2017-04-18 13:36:22]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f8ebda9548f2a4bfc00d414f4851...