Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559273005:
[Fetch API] Implement combining of Headers with same keys. (Closed)
Description[Fetch API] Implement combining of Headers for same keys.
Currently FetchHeaderList::sortAndCombine() only sorts the list and does not
combine values for the same key. This CL implements the missing part in the
function.
BUG=670439, 667376
Committed: https://crrev.com/da6e2412d9e3547954e1196c3b8d523be7090df6
Cr-Commit-Position: refs/heads/master@{#438615}
Patch Set 1 #Patch Set 2 : Fix test #Patch Set 3 : Fix test #Patch Set 4 : String concat change from "+" to append() #Patch Set 5 : String concat change from "+" to append #Patch Set 6 : ', ' to ',' #
Total comments: 8
Patch Set 7 : Fix for jsbell comment. #
Total comments: 2
Patch Set 8 : Fix #Patch Set 9 : fix #
Total comments: 2
Patch Set 10 : fix nit #Patch Set 11 : webkit layout test fix #
Total comments: 1
Patch Set 12 : fix #
Messages
Total messages: 50 (24 generated)
Description was changed from ========== This CL is implement combining of Headers with same keys. but maybe not perfect implementation. BUG=670439 ========== to ========== This CL is implement combining of Headers with same keys. but maybe not perfect implementation. BUG=670439 ==========
corona10@gmail.com changed reviewers: + yhirano@chromium.org
yhirano@ PTAL. And should I fix third_party/WebKit/LayoutTests/TestExpectations also?
corona10@gmail.com changed reviewers: + jsbell@chromium.org
Description was changed from ========== This CL is implement combining of Headers with same keys. but maybe not perfect implementation. BUG=670439 ========== to ========== This CL is implement combining of Headers with same keys. but maybe not perfect implementation. BUG=670439, 667376 ==========
memo CLA: ok src/AUTHORS: ok
Sorry about the delay in replying - was out of office. https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:254: }, 'Iteration mutation'); Can you add value mutations to this test? https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:272: 'The values should be sorted.'); The spec does not indicate that values should be sorted. What is the behavior in Firefox? https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:273: },'Sorted and combined on iteration'); nit: space after comma https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:137: if (a->first == b->first) { nit: no {} needed here https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:138: return WTF::codePointCompareLessThan(a->second, b->second); Is the sorting here (by value) done to make the sort stable? It does not seem necessary per spec, which only defines sorting by names. This may require filing a spec issue. https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:143: if (m_headerList.size() > 1) { This test could be inverted to become an early exit and moved to the top of the method to avoid the indenting here. https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:148: m_headerList.remove(index, 1); This algorithm could be reversed to start at the end of the vector, and avoid shuffling the array down for each removal, but it's probably not worth it and would be more confusing. https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:150: index++; nit: prefer pre-increment when post-increment is not necessary
On 2016/12/12 16:54:51, jsbell wrote: > Sorry about the delay in replying - was out of office. > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js > (right): > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:254: }, > 'Iteration mutation'); > Can you add value mutations to this test? > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:272: > 'The values should be sorted.'); > The spec does not indicate that values should be sorted. What is the behavior in > Firefox? > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:273: > },'Sorted and combined on iteration'); > nit: space after comma > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:137: if (a->first == > b->first) { > nit: no {} needed here > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:138: return > WTF::codePointCompareLessThan(a->second, b->second); > Is the sorting here (by value) done to make the sort stable? It does not seem > necessary per spec, which only defines sorting by names. > > This may require filing a spec issue. > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:143: if > (m_headerList.size() > 1) { > This test could be inverted to become an early exit and moved to the top of the > method to avoid the indenting here. > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:148: > m_headerList.remove(index, 1); > This algorithm could be reversed to start at the end of the vector, and avoid > shuffling the array down for each removal, but it's probably not worth it and > would be more confusing. > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:150: index++; > nit: prefer pre-increment when post-increment is not necessary jsbell@ Thank you for review my code. I checked the behavior of FF just a second but not dev version. It dosen't merge their values but they looks like sorted by only key. So I will revert it also.
On 2016/12/12 19:56:31, dhna wrote: > On 2016/12/12 16:54:51, jsbell wrote: > > Sorry about the delay in replying - was out of office. > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > > File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js > > (right): > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:254: > }, > > 'Iteration mutation'); > > Can you add value mutations to this test? > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:272: > > 'The values should be sorted.'); > > The spec does not indicate that values should be sorted. What is the behavior > in > > Firefox? > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Lay... > > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:273: > > },'Sorted and combined on iteration'); > > nit: space after comma > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:137: if (a->first > == > > b->first) { > > nit: no {} needed here > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:138: return > > WTF::codePointCompareLessThan(a->second, b->second); > > Is the sorting here (by value) done to make the sort stable? It does not seem > > necessary per spec, which only defines sorting by names. > > > > This may require filing a spec issue. > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:143: if > > (m_headerList.size() > 1) { > > This test could be inverted to become an early exit and moved to the top of > the > > method to avoid the indenting here. > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:148: > > m_headerList.remove(index, 1); > > This algorithm could be reversed to start at the end of the vector, and avoid > > shuffling the array down for each removal, but it's probably not worth it and > > would be more confusing. > > > > > https://codereview.chromium.org/2559273005/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:150: index++; > > nit: prefer pre-increment when post-increment is not necessary > > jsbell@ > Thank you for review my code. > I checked the behavior of FF just a second but not dev version. > It dosen't merge their values but they looks like sorted by only key. > So I will revert it also. jsbell@ Done. PTAL
https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:259: "The values should be combined and seperated by ',' "); spelling: separated nit: remove trailing space in string https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:133: if (m_headerList.size() > 1) { This would be simpler as: if (m_headerList.isEmpty()) return; ... and then you don't need to indent the rest of the method. (Sorry, I should have made this clearer in my previous comment.)
On 2016/12/12 21:00:24, jsbell wrote: > https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js > (right): > > https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:259: > "The values should be combined and seperated by ',' "); > spelling: separated > > nit: remove trailing space in string > > https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp (right): > > https://codereview.chromium.org/2559273005/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp:133: if > (m_headerList.size() > 1) { > This would be simpler as: > > if (m_headerList.isEmpty()) > return; > > ... and then you don't need to indent the rest of the method. (Sorry, I should > have made this clearer in my previous comment.) jsbell@ Done with some extra fix.
lgtm with nits, but yhirano@ should definitely review as well. https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:252: for (let [key, value] of iterator){ nit: space between ) and { https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:255: } nit: } should line up with for
On 2016/12/12 22:38:31, jsbell wrote: > lgtm with nits, but yhirano@ should definitely review as well. > > https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js > (right): > > https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:252: for > (let [key, value] of iterator){ > nit: space between ) and { > > https://codereview.chromium.org/2559273005/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:255: } > nit: } should line up with for jsbell@ Thank you for your kind review. I fixed it as you guided. yhirano@ PTAL
lgtm Can you rewrite the issue description? Also, the issue title should be included in the "Description" section, just like https://codereview.chromium.org/2568343002/.
Description was changed from ========== This CL is implement combining of Headers with same keys. but maybe not perfect implementation. BUG=670439, 667376 ========== to ========== [Fetch API] Implement combining of Headers with same keys. This CL is implement FetchHeaderList::sortAndCombine(). 1. Sort header lists by its key name. In case of header lists's length is zero, It will escape this function. 2. Merging values with same key from end to start. BUG=670439, 667376 ==========
On 2016/12/13 03:54:29, yhirano wrote: > lgtm > > Can you rewrite the issue description? Also, the issue title should be included > in the "Description" section, just like > https://codereview.chromium.org/2568343002/. yhirano@ Thank you for review. I updated issue description. Can you take a look?
Description was changed from ========== [Fetch API] Implement combining of Headers with same keys. This CL is implement FetchHeaderList::sortAndCombine(). 1. Sort header lists by its key name. In case of header lists's length is zero, It will escape this function. 2. Merging values with same key from end to start. BUG=670439, 667376 ========== to ========== [Fetch API] Implement combining of Headers with same keys. This CL is implement FetchHeaderList::sortAndCombine(). 1. Sort header lists by its key name. In case of header lists's length is zero, It will escape this function. 2. Merging values with same key from end to start. BUG=670439, 667376 ==========
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...
On 2016/12/13 04:05:53, dhna wrote: > On 2016/12/13 03:54:29, yhirano wrote: > > lgtm > > > > Can you rewrite the issue description? Also, the issue title should be > included > > in the "Description" section, just like > > https://codereview.chromium.org/2568343002/. > > yhirano@ > Thank you for review. > I updated issue description. > Can you take a look? The sorting part was already implemented, right? How about the following? Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function.
Description was changed from ========== [Fetch API] Implement combining of Headers with same keys. This CL is implement FetchHeaderList::sortAndCombine(). 1. Sort header lists by its key name. In case of header lists's length is zero, It will escape this function. 2. Merging values with same key from end to start. BUG=670439, 667376 ========== to ========== [Fetch API] Implement combining of Headers with same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 ==========
On 2016/12/13 04:28:58, yhirano wrote: > On 2016/12/13 04:05:53, dhna wrote: > > On 2016/12/13 03:54:29, yhirano wrote: > > > lgtm > > > > > > Can you rewrite the issue description? Also, the issue title should be > > included > > > in the "Description" section, just like > > > https://codereview.chromium.org/2568343002/. > > > > yhirano@ > > Thank you for review. > > I updated issue description. > > Can you take a look? > > The sorting part was already implemented, right? How about the following? > > Currently FetchHeaderList::sortAndCombine() only sorts the list and does not > combine values for the same key. This CL implements the missing part in the > function. yhirano@ Great~! PTAL
Description was changed from ========== [Fetch API] Implement combining of Headers with same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 ========== to ========== [Fetch API] Implement combining of Headers for same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 ==========
On 2016/12/13 04:31:41, dhna wrote: > On 2016/12/13 04:28:58, yhirano wrote: > > On 2016/12/13 04:05:53, dhna wrote: > > > On 2016/12/13 03:54:29, yhirano wrote: > > > > lgtm > > > > > > > > Can you rewrite the issue description? Also, the issue title should be > > > included > > > > in the "Description" section, just like > > > > https://codereview.chromium.org/2568343002/. > > > > > > yhirano@ > > > Thank you for review. > > > I updated issue description. > > > Can you take a look? > > > > The sorting part was already implemented, right? How about the following? > > > > Currently FetchHeaderList::sortAndCombine() only sorts the list and does not > > combine values for the same key. This CL implements the missing part in the > > function. > > yhirano@ > Great~! PTAL Still LGTM. Thank you!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/12/13 04:33:18, yhirano wrote: > On 2016/12/13 04:31:41, dhna wrote: > > On 2016/12/13 04:28:58, yhirano wrote: > > > On 2016/12/13 04:05:53, dhna wrote: > > > > On 2016/12/13 03:54:29, yhirano wrote: > > > > > lgtm > > > > > > > > > > Can you rewrite the issue description? Also, the issue title should be > > > > included > > > > > in the "Description" section, just like > > > > > https://codereview.chromium.org/2568343002/. > > > > > > > > yhirano@ > > > > Thank you for review. > > > > I updated issue description. > > > > Can you take a look? > > > > > > The sorting part was already implemented, right? How about the following? > > > > > > Currently FetchHeaderList::sortAndCombine() only sorts the list and does > not > > > combine values for the same key. This CL implements the missing part in the > > > function. > > > > yhirano@ > > Great~! PTAL > > Still LGTM. Thank you! Some webkit test are failed. May be this CL affect to result. Is there any guide to dig into this issue?
On 2016/12/13 06:54:25, dhna wrote: > On 2016/12/13 04:33:18, yhirano wrote: > > On 2016/12/13 04:31:41, dhna wrote: > > > On 2016/12/13 04:28:58, yhirano wrote: > > > > On 2016/12/13 04:05:53, dhna wrote: > > > > > On 2016/12/13 03:54:29, yhirano wrote: > > > > > > lgtm > > > > > > > > > > > > Can you rewrite the issue description? Also, the issue title should be > > > > > included > > > > > > in the "Description" section, just like > > > > > > https://codereview.chromium.org/2568343002/. > > > > > > > > > > yhirano@ > > > > > Thank you for review. > > > > > I updated issue description. > > > > > Can you take a look? > > > > > > > > The sorting part was already implemented, right? How about the following? > > > > > > > > Currently FetchHeaderList::sortAndCombine() only sorts the list and does > > not > > > > combine values for the same key. This CL implements the missing part in > the > > > > function. > > > > > > yhirano@ > > > Great~! PTAL > > > > Still LGTM. Thank you! > > Some webkit test are failed. > May be this CL affect to result. > Is there any guide to dig into this issue? Same result on fetch webkit layout test FAIL Headers assert_equals: headers size should increase by 1. expected 5 but got 4 FAIL Headers assert_equals: headers size should increase by 1. expected 5 but got 4
On 2016/12/13 08:43:33, dhna wrote: > On 2016/12/13 06:54:25, dhna wrote: > > On 2016/12/13 04:33:18, yhirano wrote: > > > On 2016/12/13 04:31:41, dhna wrote: > > > > On 2016/12/13 04:28:58, yhirano wrote: > > > > > On 2016/12/13 04:05:53, dhna wrote: > > > > > > On 2016/12/13 03:54:29, yhirano wrote: > > > > > > > lgtm > > > > > > > > > > > > > > Can you rewrite the issue description? Also, the issue title should > be > > > > > > included > > > > > > > in the "Description" section, just like > > > > > > > https://codereview.chromium.org/2568343002/. > > > > > > > > > > > > yhirano@ > > > > > > Thank you for review. > > > > > > I updated issue description. > > > > > > Can you take a look? > > > > > > > > > > The sorting part was already implemented, right? How about the > following? > > > > > > > > > > Currently FetchHeaderList::sortAndCombine() only sorts the list and > does > > > not > > > > > combine values for the same key. This CL implements the missing part in > > the > > > > > function. > > > > > > > > yhirano@ > > > > Great~! PTAL > > > > > > Still LGTM. Thank you! > > > > Some webkit test are failed. > > May be this CL affect to result. > > Is there any guide to dig into this issue? > Same result on fetch webkit layout test > FAIL Headers assert_equals: headers size should increase by 1. expected 5 but > got 4 > > FAIL Headers assert_equals: headers size should increase by 1. expected 5 but > got 4 function size(headers) { var count = 0; for (var header of headers) { ++count; } return count; } I found why this happens I will change expected result.
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/13 09:12:23, dhna wrote: > On 2016/12/13 08:43:33, dhna wrote: > > On 2016/12/13 06:54:25, dhna wrote: > > > On 2016/12/13 04:33:18, yhirano wrote: > > > > On 2016/12/13 04:31:41, dhna wrote: > > > > > On 2016/12/13 04:28:58, yhirano wrote: > > > > > > On 2016/12/13 04:05:53, dhna wrote: > > > > > > > On 2016/12/13 03:54:29, yhirano wrote: > > > > > > > > lgtm > > > > > > > > > > > > > > > > Can you rewrite the issue description? Also, the issue title > should > > be > > > > > > > included > > > > > > > > in the "Description" section, just like > > > > > > > > https://codereview.chromium.org/2568343002/. > > > > > > > > > > > > > > yhirano@ > > > > > > > Thank you for review. > > > > > > > I updated issue description. > > > > > > > Can you take a look? > > > > > > > > > > > > The sorting part was already implemented, right? How about the > > following? > > > > > > > > > > > > Currently FetchHeaderList::sortAndCombine() only sorts the list and > > does > > > > not > > > > > > combine values for the same key. This CL implements the missing part > in > > > the > > > > > > function. > > > > > > > > > > yhirano@ > > > > > Great~! PTAL > > > > > > > > Still LGTM. Thank you! > > > > > > Some webkit test are failed. > > > May be this CL affect to result. > > > Is there any guide to dig into this issue? > > Same result on fetch webkit layout test > > FAIL Headers assert_equals: headers size should increase by 1. expected 5 but > > got 4 > > > > FAIL Headers assert_equals: headers size should increase by 1. expected 5 but > > got 4 > > function size(headers) { > var count = 0; > for (var header of headers) { > ++count; > } > return count; > } > > I found why this happens I will change expected result. jsbell@ yhirano@ For reviewers, I modify expected result of script-tests/headers.js. Because 'function size(headers)'s result is changed by this CL. But I don't know assert_equals's message is proper. PTAL
https://codereview.chromium.org/2559273005/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js (right): https://codereview.chromium.org/2559273005/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:135: headers.append('X-FETCH-TEST', 'response test field - append'); Can you append one another header (say X-FETCH-TEST-2) and see the size increase?
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...
On 2016/12/13 11:14:54, yhirano wrote: > https://codereview.chromium.org/2559273005/diff/200001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js > (right): > > https://codereview.chromium.org/2559273005/diff/200001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js:135: > headers.append('X-FETCH-TEST', 'response test field - append'); > Can you append one another header (say X-FETCH-TEST-2) and see the size > increase? yhirano@ Yes, It is increased to 5. PTAL
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/13 11:56:55, yhirano wrote: > lgtm yhirano@ Thanks~! jsbell@ All test were passed. If you approved it I will commit it.
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...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481741997252260,
"parent_rev": "02ddf9ce045a0b44dc095d8562e6684388f8ad86", "commit_rev":
"f4bb20cd7d67faba04d33c6579b78ac4397d4145"}
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Implement combining of Headers for same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 ========== to ========== [Fetch API] Implement combining of Headers for same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 Review-Url: https://codereview.chromium.org/2559273005 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [Fetch API] Implement combining of Headers for same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 Review-Url: https://codereview.chromium.org/2559273005 ========== to ========== [Fetch API] Implement combining of Headers for same keys. Currently FetchHeaderList::sortAndCombine() only sorts the list and does not combine values for the same key. This CL implements the missing part in the function. BUG=670439, 667376 Committed: https://crrev.com/da6e2412d9e3547954e1196c3b8d523be7090df6 Cr-Commit-Position: refs/heads/master@{#438615} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/da6e2412d9e3547954e1196c3b8d523be7090df6 Cr-Commit-Position: refs/heads/master@{#438615} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
