Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2587463002:
[Fetch API] Appending headers of the same name should have their values (Closed)
DescriptionAppending headers of the same name should have their values
joined by ','.
Now, FetchHeaderList::get() is iterating all header list and
append strings to a result joined by ','.
BUG=645497
Committed: https://crrev.com/7ad32cbf574e3d263b64f1888520b9158798d41b
Cr-Commit-Position: refs/heads/master@{#439743}
Patch Set 1 #
Total comments: 9
Patch Set 2 : add unit test. #Patch Set 3 : fix for error case. change test #Patch Set 4 : it will be nicer. #
Total comments: 2
Patch Set 5 : minor fix #Patch Set 6 : fix #Patch Set 7 : add flag #Patch Set 8 : add test #Patch Set 9 : fix comment for changed implementation. #
Total comments: 3
Patch Set 10 : minor fix #Patch Set 11 : fix #Patch Set 12 : fix #Patch Set 13 : flag to found #
Total comments: 2
Patch Set 14 : Comment fix. #
Messages
Total messages: 44 (13 generated)
Description was changed from ========== Appending headers of the same name should have their values joined by ','. BUG=645497 ========== to ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 ==========
corona10@gmail.com changed reviewers: + jsbell@chromium.org, yhirano@chromium.org
Description was changed from ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 ========== to ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 ==========
jsbell@ yhirano@ PTAL
looks correct, but a handful of suggestions... https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:40: // 'set()' Do we have cases for invalid names/values anywhere? (In the context of this CL I want to see a test for empty values) https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:91: for (auto&& header : m_headerList) { Do we have a preference for auto&& vs. const auto& ? (I just learned about auto&& - thanks! - so I haven't seen it used in the code base before, but maybe I'm just looking at old code.) https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if (result.isEmpty()) { My reading of the spec is that values may not be empty (i.e. this is correct, and e.g. ",," would never be generated by this) Can you DCHECK(!header->second.isEmpty()); (or whatever the right incantation is) outside the if() blocks? https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return flag; Do we need flag, or can it just be !result.isEmpty() ?
https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:91: for (auto&& header : m_headerList) { On 2016/12/16 17:58:26, jsbell wrote: > Do we have a preference for auto&& vs. const auto& ? > > (I just learned about auto&& - thanks! - so I haven't seen it used in the code > base before, but maybe I'm just looking at old code.) I like your suggestion. https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if (result.isEmpty()) { On 2016/12/16 17:58:26, jsbell wrote: > My reading of the spec is that values may not be empty (i.e. this is correct, > and e.g. ",," would never be generated by this) > > Can you DCHECK(!header->second.isEmpty()); (or whatever the right incantation > is) outside the if() blocks? > jsbell@ If we meet a first key value matched. In that case result.size() will be zero. and there will be no any string. So in that case, we should append only a string without appending ','. https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return flag; On 2016/12/16 17:58:26, jsbell wrote: > Do we need flag, or can it just be !result.isEmpty() ? I like this suggestion also.
On 2016/12/16 18:33:35, dhna wrote: > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:91: for (auto&& > header : m_headerList) { > On 2016/12/16 17:58:26, jsbell wrote: > > Do we have a preference for auto&& vs. const auto& ? > > > > (I just learned about auto&& - thanks! - so I haven't seen it used in the code > > base before, but maybe I'm just looking at old code.) > > I like your suggestion. > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if > (result.isEmpty()) { > On 2016/12/16 17:58:26, jsbell wrote: > > My reading of the spec is that values may not be empty (i.e. this is correct, > > and e.g. ",," would never be generated by this) > > > > Can you DCHECK(!header->second.isEmpty()); (or whatever the right incantation > > is) outside the if() blocks? > > > > jsbell@ > If we meet a first key value matched. > In that case result.size() will be zero. and there will be no any string. > So in that case, we should append only a string without appending ','. > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return flag; > On 2016/12/16 17:58:26, jsbell wrote: > > Do we need flag, or can it just be !result.isEmpty() ? > > I like this suggestion also. jsbell@ Fixed it as your suggestion. PTAL. and I leave some comment on your review.
https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if (result.isEmpty()) { On 2016/12/16 18:33:35, dhna wrote: > On 2016/12/16 17:58:26, jsbell wrote: > > My reading of the spec is that values may not be empty (i.e. this is correct, > > and e.g. ",," would never be generated by this) > > > > Can you DCHECK(!header->second.isEmpty()); (or whatever the right incantation > > is) outside the if() blocks? > > > > jsbell@ > If we meet a first key value matched. > In that case result.size() will be zero. and there will be no any string. > So in that case, we should append only a string without appending ','. To clarify: I agree the code is correct. I do not think it should change. As a reviewer, I was not sure that header values could not be empty strings, and I had to check the spec. After reading the spec I agree that the code is correct. My suggestion is to add a DCHECK to reinforce that the header values cannot be empty, which would catch bugs elsewhere and also documents to a future reviewer/reader that values can not be empty.
sorry for the possible confusion... https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if (result.isEmpty()) { > To clarify: I agree the code is correct. I do not think it should change. And to clarify one more time: I mean that your proposed code (this CL, what is being reviewed) is correct.
On 2016/12/16 18:57:33, jsbell wrote: > sorry for the possible confusion... > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if > (result.isEmpty()) { > > To clarify: I agree the code is correct. I do not think it should change. > > And to clarify one more time: I mean that your proposed code (this CL, what is > being reviewed) is correct. jsbell@ Oh sorry for the late. I live in GMT+9. I understand what you are saying. I will add some logic kind of DCHECK.
On 2016/12/16 21:35:48, dhna wrote: > On 2016/12/16 18:57:33, jsbell wrote: > > sorry for the possible confusion... > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if > > (result.isEmpty()) { > > > To clarify: I agree the code is correct. I do not think it should change. > > > > And to clarify one more time: I mean that your proposed code (this CL, what is > > being reviewed) is correct. > > jsbell@ > Oh sorry for the late. > I live in GMT+9. I understand what you are saying. I will add some logic kind of > DCHECK. jsbell@ You're right. In this case var headers3 = new Headers(); headers3.append('test', 'a'); headers3.append('test', ''); headers3.append('test', 'b'); assert_equals(headers3.get('test'), 'a,b'); // will not passed because result will be 'a,,b'. So I add some logic. PTAL.
On 2016/12/16 22:00:18, dhna wrote: > On 2016/12/16 21:35:48, dhna wrote: > > On 2016/12/16 18:57:33, jsbell wrote: > > > sorry for the possible confusion... > > > > > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if > > > (result.isEmpty()) { > > > > To clarify: I agree the code is correct. I do not think it should change. > > > > > > And to clarify one more time: I mean that your proposed code (this CL, what > is > > > being reviewed) is correct. > > > > jsbell@ > > Oh sorry for the late. > > I live in GMT+9. I understand what you are saying. I will add some logic kind > of > > DCHECK. > > jsbell@ > You're right. In this case > > var headers3 = new Headers(); > headers3.append('test', 'a'); > headers3.append('test', ''); > headers3.append('test', 'b'); > assert_equals(headers3.get('test'), 'a,b'); // will not passed because result > will be 'a,,b'. > > So I add some logic. > PTAL. jsbell@ I changed append() not to add empty value. and add DCHECK at FetchHeaderList::get. PTAL
On 2016/12/16 22:06:18, dhna wrote: > On 2016/12/16 22:00:18, dhna wrote: > > On 2016/12/16 21:35:48, dhna wrote: > > > On 2016/12/16 18:57:33, jsbell wrote: > > > > sorry for the possible confusion... > > > > > > > > > > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2587463002/diff/1/third_party/WebKit/Source/m... > > > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if > > > > (result.isEmpty()) { > > > > > To clarify: I agree the code is correct. I do not think it should > change. > > > > > > > > And to clarify one more time: I mean that your proposed code (this CL, > what > > is > > > > being reviewed) is correct. > > > > > > jsbell@ > > > Oh sorry for the late. > > > I live in GMT+9. I understand what you are saying. I will add some logic > kind > > of > > > DCHECK. > > > > jsbell@ > > You're right. In this case > > > > var headers3 = new Headers(); > > headers3.append('test', 'a'); > > headers3.append('test', ''); > > headers3.append('test', 'b'); > > assert_equals(headers3.get('test'), 'a,b'); // will not passed because result > > will be 'a,,b'. > > > > So I add some logic. > > PTAL. > > jsbell@ > I changed append() not to add empty value. > and add DCHECK at FetchHeaderList::get. > PTAL Sorry for the frequent reply. In this case, headers3.set('test', ''); assert_equals(headers3.get('test'), ''); ---> should be null or "" ??
On further reading, either the spec's definition of value or our definition is wrong. Spec says "A value is a byte sequence that matches the field-content token production." FetchHeaderList::isValidHeaderValue() says it is roughly matching field-value. Per the grammar in: https://tools.ietf.org/html/rfc7230#section-3.2 ... it looks like field-value allows empty but field-content must be at least one char. We should follow up with annevk@ https://codereview.chromium.org/2587463002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2587463002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:176: headers3.append('test', ''); Hrm... I think this should throw TypeError: https://fetch.spec.whatwg.org/#concept-headers-append "If name is not a name or value is not a value, then throw a TypeError." https://fetch.spec.whatwg.org/#concept-header-value "A value is a byte sequence that matches the field-content token production." https://tools.ietf.org/html/rfc7230#section-3.2 field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] field-vchar = VCHAR / obs-text ... which seems to me that it must be non-empty. https://codereview.chromium.org/2587463002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:33: if (!value.isEmpty()) This seems incorrect - the caller (Headers::append) should validate this first.
Ah, the spec is known to be wrong here: https://github.com/whatwg/fetch/issues/332
So empty values *should* be allowed and thus getting "a,,b" out is expected. Sorry for the churn here. :( I didn't know the spec was buggy. PS#2 is close to correct, it will just need to not not rely on the result being empty and use a 'first' flag or something.
On 2016/12/16 22:34:18, jsbell wrote: > So empty values *should* be allowed and thus getting "a,,b" out is expected. > Sorry for the churn here. :( I didn't know the spec was buggy. > > PS#2 is close to correct, it will just need to not not rely on the result being > empty and use a 'first' flag or something. jsbell@ Yes, It looks like buggy. :-) IMO, spec should be more clarified. Anyway Done. PTAL
Thank you for sticking with this! https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:93: if (result.isEmpty()) { Shouldn't this test be (!flag) ? https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return !result.isEmpty(); Won't this return false if there is only one matching header, but it is empty? It seems this should be `return flag;` If so, it might make more sense to rename `flag` to `found`.
One more thing - can you add a test case where there are two values for a
header and the first is empty? i.e.
headers.append('foo', '');
headers.append('foo', 'a');
headers.get('foo');
I would expect ",a" in that case.
https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return !result.isEmpty(); On 2016/12/16 23:45:46, jsbell wrote: > Won't this return false if there is only one matching header, but it is empty? > It seems this should be `return flag;` > > If so, it might make more sense to rename `flag` to `found`. > Oh it's my mistake sorry.
On 2016/12/16 23:57:44, dhna wrote: > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return > !result.isEmpty(); > On 2016/12/16 23:45:46, jsbell wrote: > > Won't this return false if there is only one matching header, but it is empty? > > It seems this should be `return flag;` > > > > If so, it might make more sense to rename `flag` to `found`. > > > > Oh it's my mistake sorry. jsbell@ With some fixes for my mistake and add your test. but unfortunately, a result does not seem to be like our expected. I think i might be dig into this issue. var headers4 = new Headers(); headers4.append('foo', ''); headers4.append('foo', 'a'); headers4.get('foo'); --> 'a' not ',a' is wrong.
On 2016/12/17 00:09:54, dhna wrote: > On 2016/12/16 23:57:44, dhna wrote: > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return > > !result.isEmpty(); > > On 2016/12/16 23:45:46, jsbell wrote: > > > Won't this return false if there is only one matching header, but it is > empty? > > > It seems this should be `return flag;` > > > > > > If so, it might make more sense to rename `flag` to `found`. > > > > > > > Oh it's my mistake sorry. > > jsbell@ > With some fixes for my mistake and add your test. > but unfortunately, a result does not seem to be like our expected. > I think i might be dig into this issue. > > var headers4 = new Headers(); > headers4.append('foo', ''); > headers4.append('foo', 'a'); > headers4.get('foo'); --> 'a' not ',a' is wrong. jsbell@ Now, it works! PTAL.
On 2016/12/17 00:22:25, dhna wrote: > On 2016/12/17 00:09:54, dhna wrote: > > On 2016/12/16 23:57:44, dhna wrote: > > > > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > > > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return > > > !result.isEmpty(); > > > On 2016/12/16 23:45:46, jsbell wrote: > > > > Won't this return false if there is only one matching header, but it is > > empty? > > > > It seems this should be `return flag;` > > > > > > > > If so, it might make more sense to rename `flag` to `found`. > > > > > > > > > > Oh it's my mistake sorry. > > > > jsbell@ > > With some fixes for my mistake and add your test. > > but unfortunately, a result does not seem to be like our expected. > > I think i might be dig into this issue. > > > > var headers4 = new Headers(); > > headers4.append('foo', ''); > > headers4.append('foo', 'a'); > > headers4.get('foo'); --> 'a' not ',a' is wrong. > > jsbell@ > Now, it works! > PTAL. I know code is liitle bit strange. I will do more test when I get back home.
On 2016/12/17 05:03:18, dhna wrote: > On 2016/12/17 00:22:25, dhna wrote: > > On 2016/12/17 00:09:54, dhna wrote: > > > On 2016/12/16 23:57:44, dhna wrote: > > > > > > > > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2587463002/diff/100002/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:102: return > > > > !result.isEmpty(); > > > > On 2016/12/16 23:45:46, jsbell wrote: > > > > > Won't this return false if there is only one matching header, but it is > > > empty? > > > > > It seems this should be `return flag;` > > > > > > > > > > If so, it might make more sense to rename `flag` to `found`. > > > > > > > > > > > > > Oh it's my mistake sorry. > > > > > > jsbell@ > > > With some fixes for my mistake and add your test. > > > but unfortunately, a result does not seem to be like our expected. > > > I think i might be dig into this issue. > > > > > > var headers4 = new Headers(); > > > headers4.append('foo', ''); > > > headers4.append('foo', 'a'); > > > headers4.get('foo'); --> 'a' not ',a' is wrong. > > > > jsbell@ > > Now, it works! > > PTAL. > > I know code is liitle bit strange. I will do more test when I get back home. I fixed it. It will be looks good than PS11 PTAL.
https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/Headers.cpp (right): https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Headers.cpp:180: // "2. If there is no header in header list whose name is name, return null." whose name is |name| https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/Headers.cpp:181: // "3. Return |result| as the combined header list." Where does this statement come from? Isn't it "3. Return the combined value given |name| and header list."?
On 2016/12/19 11:43:05, yhirano wrote: > https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/Headers.cpp (right): > > https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/Headers.cpp:180: // "2. If there is no > header in header list whose name is name, return null." > whose name is |name| > > https://codereview.chromium.org/2587463002/diff/230001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/Headers.cpp:181: // "3. Return |result| > as the combined header list." > Where does this statement come from? Isn't it > > "3. Return the combined value given |name| and header list."? yhirano@ Done. PTAL
lgtm - thanks for your patience with the reviews!
On 2016/12/19 17:27:14, jsbell wrote: > lgtm - thanks for your patience with the reviews! Thanks!!
The CQ bit was checked by corona10@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/19 19:08:58, dhna wrote: > On 2016/12/19 17:27:14, jsbell wrote: > > lgtm - thanks for your patience with the reviews! > > Thanks!! yhirano@ Is there more comment for this CL?
lgtm
The CQ bit was checked by corona10@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by corona10@gmail.com
The CQ bit was checked by corona10@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 250001, "attempt_start_ts": 1482219146074310,
"parent_rev": "7de336e5ff41b598ed1afe78d0e0fd14becccd67", "commit_rev":
"7fc194f7215c36cbf81e58e627c4a8827729a933"}
Message was sent while issue was closed.
Description was changed from ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 ========== to ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 Review-Url: https://codereview.chromium.org/2587463002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001)
Message was sent while issue was closed.
Description was changed from ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 Review-Url: https://codereview.chromium.org/2587463002 ========== to ========== Appending headers of the same name should have their values joined by ','. Now, FetchHeaderList::get() is iterating all header list and append strings to a result joined by ','. BUG=645497 Committed: https://crrev.com/7ad32cbf574e3d263b64f1888520b9158798d41b Cr-Commit-Position: refs/heads/master@{#439743} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7ad32cbf574e3d263b64f1888520b9158798d41b Cr-Commit-Position: refs/heads/master@{#439743} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
