New issue
Advanced search Search tips

Issue 670439 link

Starred by 2 users
Status: Fixed
Owner: ----
Closed: Jun 2017
Cc:
jakearchibald@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Header values are not sorted and combined on iteration

Project Member Reported by jsb...@chromium.org, Dec 1 2016 
Fetch requires [1] that "the value pairs to iterate over are the return value of running sort and combine with the header list", which is to say we don't simply iterate over the internal data of Headers but make a sorted and combined copy [2] [3].

[1] https://fetch.spec.whatwg.org/#headers-class
[2] https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine
[3] https://fetch.spec.whatwg.org/#concept-header-value-combined

e.g.

const headers = new Headers();
headers.append('b', '1');
headers.append('a', '2');
headers.append('a', '3');
const keys = [], values = [];
for (let [k, v] of headers)
  console.log(`${k}: ${v}`);

Should log:

a: 2, 3
b: 1

See also:  issue 645497
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 3 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abbafbc0bacfca5beada1e3d1a5299569e0a5262

commit abbafbc0bacfca5beada1e3d1a5299569e0a5262
Author: jsbell <jsbell@chromium.org>
Date: Sat Dec 03 02:33:10 2016

Sort headers for iteration

Fetch requires [1] that "the value pairs to iterate over are the
return value of running sort and combine with the header list", which
is to say we don't simply iterate over the internal data of Headers
but make a sorted (and combined) copy [2]. This manifested in some
failing imported cache tests that were sensitive to the order.

This CL implements making a copy and sorting the list for iteration;
it does not implement combining[3]. So not perfect but an incremental
improvement.

[1] https://fetch.spec.whatwg.org/#headers-class
[2] https://fetch.spec.whatwg.org/#concept-header-list-sort-and-combine
[3] https://fetch.spec.whatwg.org/#concept-header-value-combined

BUG= 667376 , 670439 
R=yhirano@chromium.org

Review-Url: https://codereview.chromium.org/2542073005
Cr-Commit-Position: refs/heads/master@{#436147}

[modify] https://crrev.com/abbafbc0bacfca5beada1e3d1a5299569e0a5262/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/abbafbc0bacfca5beada1e3d1a5299569e0a5262/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js
[delete] https://crrev.com/5190330169f1e6c1d8418e6bcc0499fdc99ec287/third_party/WebKit/LayoutTests/imported/wpt/service-workers/cache-storage/window/cache-match.https-expected.txt
[delete] https://crrev.com/5190330169f1e6c1d8418e6bcc0499fdc99ec287/third_party/WebKit/LayoutTests/imported/wpt/service-workers/cache-storage/window/cache-put.https-expected.txt
[modify] https://crrev.com/abbafbc0bacfca5beada1e3d1a5299569e0a5262/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp
[modify] https://crrev.com/abbafbc0bacfca5beada1e3d1a5299569e0a5262/third_party/WebKit/Source/modules/fetch/FetchHeaderList.h
[modify] https://crrev.com/abbafbc0bacfca5beada1e3d1a5299569e0a5262/third_party/WebKit/Source/modules/fetch/Headers.cpp

Comment 2 by tyoshino@chromium.org, Dec 7 2016

Status: Available (was: Untriaged)
Sorting part has been fixed by the commit in #1.

Combining part still needs to be fixed. Ideally  bug 645497  should also get fixed at the same time.

Comment 3 Deleted

Comment 4 by coron...@gmail.com, Dec 11 2016 

Upload CL for this issue. (https://codereview.chromium.org/2559273005/)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da6e2412d9e3547954e1196c3b8d523be7090df6

commit da6e2412d9e3547954e1196c3b8d523be7090df6
Author: corona10 <corona10@gmail.com>
Date: Wed Dec 14 20:47:27 2016

[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
Cr-Commit-Position: refs/heads/master@{#438615}

[modify] https://crrev.com/da6e2412d9e3547954e1196c3b8d523be7090df6/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/headers.js
[modify] https://crrev.com/da6e2412d9e3547954e1196c3b8d523be7090df6/third_party/WebKit/Source/modules/fetch/FetchHeaderList.cpp

Comment 6 by raphael....@intel.com, Jun 2 2017 

I've just tried the sample from the bug description and got the expected result. I guess we can close this bug?

Comment 7 by jsb...@chromium.org, Jun 2 2017

Status: Fixed (was: Available)
Yep!
►

Sign in to add a comment