[Fetch API] Implement Fetch API Headers

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

This bug is about implementing https://fetch.spec.whatwg.org/#headers-class

[youenn fablet]

Created attachment 267561 [details]
Patch

[WebKit Commit Bot]

Attachment 267561 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltins.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Mac EWS can't make up its mind yet, but looks like there are both flaky and reliably failing fetch tests.

[Build Bot]

Comment on attachment 267561 [details]
Patch

Attachment 267561 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/571952

New failing tests:
imported/w3c/web-platform-tests/fetch-api/headers/headers-basic.html

[Build Bot]

Created attachment 267566 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Comment on attachment 267561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267561&action=review

r- because we need another patch that addresses these comments, but most of this looks good.
headers-basic.html passes on my computer in WK1 and WK2.  I'm not sure why ews doesn't like it.

> Source/WebCore/CMakeLists.txt:9
> +    "${WEBCORE_DIR}/Modules/fetch-api"

Modules/fetch. No api.

> Source/WebCore/CMakeLists.txt:51
> +    "${WEBCORE_DIR}/fetch"

I don't think we should add this directory.  Why not just put everything in Modules/fetch?

> Source/WebCore/fetch/FetchHeaderList.cpp:45
> +    // FIXME: UserAgent and ContentTransferEncoding are not part of of the list.

They are part of the list.  Why not remove them?

> Source/WebCore/fetch/FetchHeaderList.cpp:92
> +        {String mimeType = extractMIMETypeFromMediaType(value);

This is unused.  Also, this is strange use of {

> Source/WebCore/fetch/FetchHeaderList.cpp:109
> +    m_headers.removeAllMatching([&] (Header& header) -> bool {
> +        return lowerCasedName == header.name;
> +    });

There can only be one header with the given name, right?  We could break early after we found it if we wrote our own loop.  See the later comment about HashSets, though.

> Source/WebCore/fetch/FetchHeaderList.h:60
> +    Vector<Header> m_headers;

I'm not sure we should be using a Vector here.  I think we should use a HashSet.  That would reduce the worst-case time of remove, get, has, and set from O(n) to O(1), and it would make getAll increase from O(n) to O(n log(n)), right?  https://fetch.spec.whatwg.org/#dom-headers-getall says it must return the strings "in list order," but that's the only restriction, right?

> LayoutTests/imported/w3c/web-platform-tests/fetch-api/headers/headers-idl-expected.txt:2
> +FAIL Headers interface: existence and properties of interface object assert_equals: class string of Headers expected "[object Function]" but got "[object HeadersConstructor]"

I assume these will be fixed in the future, right?

[youenn fablet]

Thanks for the early feedback, I will look into it more deeply tomorrow if possible.
The tests need some more work, cosmetic and more substantial according mac bot.

As for headers, the structure can contain several headers with the same name. They are not combined like in XHR.

As of WebCore/fetch and WebCore/Modules/fetch-api, the idea was to put in fetch-api, only the bits specific to fetch API and the bits specific to fetch algo in WebCore/fetch.
But maybe it is simpler to have one folder at the beginning?

[youenn fablet]

Thanks for the feedback.

> > Source/WebCore/CMakeLists.txt:9
> > +    "${WEBCORE_DIR}/Modules/fetch-api"
> 
> Modules/fetch. No api.
> 
> > Source/WebCore/CMakeLists.txt:51
> > +    "${WEBCORE_DIR}/fetch"
> 
> I don't think we should add this directory.  Why not just put everything in
> Modules/fetch?

As I said, this was to separate between fetch API (section 6 of https://fetch.spec.whatwg.org/) from the fetch algorithm.
Let's stick with one folder for the moment.

> > Source/WebCore/fetch/FetchHeaderList.cpp:45
> > +    // FIXME: UserAgent and ContentTransferEncoding are not part of of the list.
> 
> They are part of the list.  Why not remove them?

The FIXME is not clear.
isForbiddenHeaderName is code copied from XHR implementation (isForbiddenRequestHeader).

But the list in isForbiddenRequestHeader is the same as https://fetch.spec.whatwg.org/#forbidden-header-name except for UserAgent and ContentTransferEncoding.

> 
> > Source/WebCore/fetch/FetchHeaderList.cpp:92
> > +        {String mimeType = extractMIMETypeFromMediaType(value);
> 
> This is unused.  Also, this is strange use of {

Will fix it.

> > Source/WebCore/fetch/FetchHeaderList.h:60
> > +    Vector<Header> m_headers;
> 
> I'm not sure we should be using a Vector here.  I think we should use a
> HashSet.  That would reduce the worst-case time of remove, get, has, and set
> from O(n) to O(1), and it would make getAll increase from O(n) to O(n
> log(n)), right?  https://fetch.spec.whatwg.org/#dom-headers-getall says it
> must return the strings "in list order," but that's the only restriction,
> right?

That is a good point.
The spec is currently mandating a strong order between headers but it might be worth lessen it.
I filed https://github.com/whatwg/fetch/issues/189.

> > LayoutTests/imported/w3c/web-platform-tests/fetch-api/headers/headers-idl-expected.txt:2
> > +FAIL Headers interface: existence and properties of interface object assert_equals: class string of Headers expected "[object Function]" but got "[object HeadersConstructor]"
> 
> I assume these will be fixed in the future, right?

This is a bigger issue than Headers.
These failures should be fixed by the binding generator at some point.

[youenn fablet]

Thanks for the feedback.

> > Source/WebCore/CMakeLists.txt:9
> > +    "${WEBCORE_DIR}/Modules/fetch-api"
> 
> Modules/fetch. No api.
> 
> > Source/WebCore/CMakeLists.txt:51
> > +    "${WEBCORE_DIR}/fetch"
> 
> I don't think we should add this directory.  Why not just put everything in
> Modules/fetch?

As I said, this was to separate between fetch API (section 6 of https://fetch.spec.whatwg.org/) from the fetch algorithm.
Let's stick with one folder for the moment.

> > Source/WebCore/fetch/FetchHeaderList.cpp:45
> > +    // FIXME: UserAgent and ContentTransferEncoding are not part of of the list.
> 
> They are part of the list.  Why not remove them?

The FIXME is not clear, I will refresh it.

This code is copied from XHR implementation (isForbiddenRequestHeader).
But the list in isForbiddenRequestHeader is the same as https://fetch.spec.whatwg.org/#forbidden-header-name except for UserAgent and ContentTransferEncoding.

> 
> > Source/WebCore/fetch/FetchHeaderList.cpp:92
> > +        {String mimeType = extractMIMETypeFromMediaType(value);
> 
> This is unused.  Also, this is strange use of {

Will fix it.

> > Source/WebCore/fetch/FetchHeaderList.h:60
> > +    Vector<Header> m_headers;
> 
> I'm not sure we should be using a Vector here.  I think we should use a
> HashSet.  That would reduce the worst-case time of remove, get, has, and set
> from O(n) to O(1), and it would make getAll increase from O(n) to O(n
> log(n)), right?  https://fetch.spec.whatwg.org/#dom-headers-getall says it
> must return the strings "in list order," but that's the only restriction,
> right?

That is a good point.
The spec is currently mandating a strong order between headers but it might be worth lessen it.
I filed https://github.com/whatwg/fetch/issues/189.

For the moment, I plan to keep using a vector.
HTTPHeaderMap would need to be modified to a multimap ordered map to support fetch.
If the order is lessen, HTTPHeaderMap could directly be used by fetch.

> > LayoutTests/imported/w3c/web-platform-tests/fetch-api/headers/headers-idl-expected.txt:2
> > +FAIL Headers interface: existence and properties of interface object assert_equals: class string of Headers expected "[object Function]" but got "[object HeadersConstructor]"
> 
> I assume these will be fixed in the future, right?

This is a bigger issue than Headers.
These failures should be fixed by the binding generator at some point.

[Alex Christensen]

> I filed https://github.com/whatwg/fetch/issues/189.
Regardless of the outcome of that discussion, I still think we should use a HashSet to store them.  If there are indeed usually <=10 headers, then it will be fast to sort them.  We should not have a possible O(n^2) insertion time.

[youenn fablet]

(In reply to comment #10)
> > I filed https://github.com/whatwg/fetch/issues/189.
> Regardless of the outcome of that discussion, I still think we should use a
> HashSet to store them.  If there are indeed usually <=10 headers, then it
> will be fast to sort them.  We should not have a possible O(n^2) insertion
> time.

In both cases, appending a pair will be O(1), and probably faster with an array.
It is true that getting/setting a header will be O(n) vs O(1).

I am not sure to understand O(n^2) insertion time issue. Can you clarify where that would happen?

The HTTPHeaderMap/FetchAPIHeaders conversion has a cost and it would be good to just use one structure here.

Having HTTPHeaderMap implemented as a vector would allow fixing the case of multiple headers with the same name. As suggested on github, it may also bring some minor performance improvements (measurements maybe needed here?).

[Alex Christensen]

FetchHeaderList::get, FetchHeaderList::set, and FetchHeaderList::has all take O(n) time.  They should only need O(1) time.  If these operations are done for each header, that would reduce O(n^2) to O(n).

[youenn fablet]

(In reply to comment #12)
> FetchHeaderList::get, FetchHeaderList::set, and FetchHeaderList::has all
> take O(n) time.  They should only need O(1) time.  If these operations are
> done for each header, that would reduce O(n^2) to O(n).

Understood.
I guess that it could be in theory reduced to O(nlogn), the list being sorted, probably according the name.

I will try to come up with some measurements to see how big this issue is and how map's O(1) compares to vector's 0(n)

[youenn fablet]

> Having HTTPHeaderMap implemented as a vector would allow fixing the case of
> multiple headers with the same name. As suggested on github, it may also
> bring some minor performance improvements (measurements maybe needed here?).

I did some very quick measurements using STL vector & unordered_map as containers for header sets.
In my test set, vector is indeed (twice) faster when number of headers is around 10.
Performances are similar when the number of headers is ~50/100, which is not typical of current HTTP requests/responses.

I can do some additional measurements using a greater data set like https://github.com/http2/http_samples/ if that helps.

It sounds like changing HTTPHeaderMap implementation would allow fixing bugs and providing minor performance improvements.

[Alex Christensen]

(In reply to comment #14)
> I did some very quick measurements using STL vector & unordered_map as
> containers for header sets.
> In my test set, vector is indeed (twice) faster when number of headers is
> around 10.
> Performances are similar when the number of headers is ~50/100, which is not
> typical of current HTTP requests/responses.
I don't care what is typical of current HTTP requests/responses.  Somebody is going to try to put lots of headers in, and that shouldn't be slow.  Making this little operation twice as fast is not worth increasing a possible worst-case time from O(1) to O(n).  A 2x speedup here is insignificant compared to other operations done with the headers, like sending and receiving them over the network.  I will r- any patch that searches a Vector when doing simple operations that ought to be done with a HashSet or a HashMap in O(1) time.

[youenn fablet]

My plan is to directly use HTTPHeaderMap as internal header structure.
HTTPHeaderMap should be progressively accommodated according fetch API needs, starting from supporting several headers with the same name.

(In reply to comment #15)
> I don't care what is typical of current HTTP requests/responses.  Somebody
> is going to try to put lots of headers in, and that shouldn't be slow. 

Why? And compared to which baseline?
Other engines will be slow.
It is not unexpected that doing odd things in a given platform will lead to poor performances.

> Making this little operation twice as fast is not worth increasing a
> possible worst-case time from O(1) to O(n).  A 2x speedup here is
> insignificant compared to other operations done with the headers, like
> sending and receiving them over the network.

Agreed, but the argument works on both sides. Having a 2x/10x slow down is also insignificant.

> I will r- any patch that
> searches a Vector when doing simple operations that ought to be done with a
> HashSet or a HashMap in O(1) time.

I agree that using a HashMap makes more sense, particularly in terms of style so I would tend to use it.
But potential interop issues is a more important criteria for me.

[Alex Christensen]

(In reply to comment #16)
> My plan is to directly use HTTPHeaderMap as internal header structure.
> HTTPHeaderMap should be progressively accommodated according fetch API
> needs, starting from supporting several headers with the same name.
Then we should use a HashMap<String, HashSet<String>> or something similar or a std::multimap.
Also, let's say you append multiple headers with the same name, then call set.  Is it defined which one will be overwritten?  There should be a test for this.
> Other engines will be slow.
This is not an excuse for also being slow.  If anything, we should have greater capacity than other engines.

[Brady Eidson]

There is no reason to use a Vector in this way here when a HashSet will do.

A set makes the code itself more compact and readable.

Running a micro benchmark on this insignificant code is definitely a premature optimization, especially when the network itself is obviously orders of magnitude slower than the CPU time being spent.

[youenn fablet]

(In reply to comment #17)
> (In reply to comment #16)
> > My plan is to directly use HTTPHeaderMap as internal header structure.
> > HTTPHeaderMap should be progressively accommodated according fetch API
> > needs, starting from supporting several headers with the same name.
> Then we should use a HashMap<String, HashSet<String>> or something similar
> or a std::multimap.

Order of headers with the same name is significant.
I was thinking of a HashMap taking a linked list or vector as value.

> Also, let's say you append multiple headers with the same name, then call
> set.  Is it defined which one will be overwritten?  There should be a test
> for this.

set will remove all headers with the same name and append the new header.
There is a test for that in the patch.

(In reply to comment #18)
> There is no reason to use a Vector in this way here when a HashSet will do.

Vector is there for spec compliancy, to retain insertion order (under discussion on github) and support multiple headers with the same name.

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #18)
> > There is no reason to use a Vector in this way here when a HashSet will do.
> 
> Vector is there for spec compliancy, to retain insertion order (under
> discussion on github) and support multiple headers with the same name.

It is news to me that the spec mandates insertion order. Great!

The spec does *not* mandate using a Vector.
Please use a ListHashSet (A HashSet that maintains insertion order).

[Brady Eidson]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > There is no reason to use a Vector in this way here when a HashSet will do.
> > 
> > Vector is there for spec compliancy, to retain insertion order (under
> > discussion on github) and support multiple headers with the same name.
> 
> It is news to me that the spec mandates insertion order. Great!
> 
> The spec does *not* mandate using a Vector.
> Please use a ListHashSet (A HashSet that maintains insertion order).

I guess I missed that you can have the same entry multiple times, which a ListHashSet doesn't support.  Not sure what the requirements really are, anymore.

[Brady Eidson]

Looking at the patch, it appears that this really is a LIST, not a set, which was misunderstood earlier.

It's weird that FetchHeaderList::set only overwrites the value for the *first* matching header in the list, but whatever - I'll assume that's right.

Never mind, carryon.

[Alex Christensen]

I assume having multiple headers with the same name and same value is also allowed, even with headers between the two.  In that case, a multimap wouldn't work, either.

[Alex Christensen]

Comment on attachment 267561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=267561&action=review

After rethinking this, we do have to use a list, not a set, so we cannot use HashSets.  Using a linked list instead of a vector would make removing faster in some cases, though.  I also think there needs to be much better testing of strange cases like these:
append name1:value1, append name2:value2, append name1:value1, set name1:value3, make sure everything is as expected.
append name1:value1, append name1:value2, remove name1, make sure everything is as expected.
append name1:value1, append name1:value2, set name1:value3, make sure everything is as expected.
etc.

> LayoutTests/imported/w3c/web-platform-tests/fetch-api/headers/headers-nameshake.html:20
> +      var expectedDict = {"single": ["sinleValue"],

sinleValue->singleValue?

[Alex Christensen]

(In reply to comment #15)
> (In reply to comment #14)
> > I did some very quick measurements using STL vector & unordered_map as
> > containers for header sets.
> > In my test set, vector is indeed (twice) faster when number of headers is
> > around 10.
> > Performances are similar when the number of headers is ~50/100, which is not
> > typical of current HTTP requests/responses.
> I don't care what is typical of current HTTP requests/responses.  Somebody
> is going to try to put lots of headers in, and that shouldn't be slow. 
> Making this little operation twice as fast is not worth increasing a
> possible worst-case time from O(1) to O(n).  A 2x speedup here is
> insignificant compared to other operations done with the headers, like
> sending and receiving them over the network.  I will r- any patch that
> searches a Vector when doing simple operations that ought to be done with a
> HashSet or a HashMap in O(1) time.
I partially take this back.  A Vector is a perfectly fine data structure for this.  We can optimize later if needed.
I filed https://github.com/whatwg/fetch/issues/190

[Alex Christensen]

For the record, the confusion was because I didn't realize that there can be multiple headers with the same name and that the order of the headers is determined by the insertion order

[youenn fablet]

Are we agreeing to replace HTTPHeaderMap with HTTPHeaderList then?
I filed bug 152828 for that purpose.

See below for additional points.


> It's weird that FetchHeaderList::set only overwrites the value for the
> *first* matching header in the list, but whatever - I'll assume that's right.

FetchHeaderList::set is defined here (https://fetch.spec.whatwg.org/#concept-header-list-set).
It overwrites the first matching header and removes the other matching headers.

Doing set() is different from remove() then append().

(In reply to comment #24)
> After rethinking this, we do have to use a list, not a set, so we cannot use
> HashSets.  Using a linked list instead of a vector would make removing
> faster in some cases, though.

Agreed. Linked list is fine but vector would probably be faster in most cases.

> I also think there needs to be much better
> testing of strange cases like these:
> append name1:value1, append name2:value2, append name1:value1, set
> name1:value3, make sure everything is as expected.
> append name1:value1, append name1:value2, remove name1, make sure everything
> is as expected.
> append name1:value1, append name1:value2, set name1:value3, make sure
> everything is as expected.

Sure, we can beef-up the existing test set.
If we replace HTTPHeaderMap by HTTPHeaderList, it might just be simpler to add unit test targetting HTTPHeaderList.

> > LayoutTests/imported/w3c/web-platform-tests/fetch-api/headers/headers-nameshake.html:20
> > +      var expectedDict = {"single": ["sinleValue"],
> 
> sinleValue->singleValue?

I will fix it.

[Alex Christensen]

(In reply to comment #25)
> I partially take this back.  A Vector is a perfectly fine data structure for
> this.  We can optimize later if needed.
We can't just replace HTTPHeaderMap with a Vector, though.  Maybe we should actually make a good data structure for this in bug 152828.

[Alex Christensen]

(In reply to comment #28)
> (In reply to comment #25)
> > I partially take this back.  A Vector is a perfectly fine data structure for
> > this.  We can optimize later if needed.
> We can't just replace HTTPHeaderMap with a Vector, though.  Maybe we should
> actually make a good data structure for this in bug 152828.
On further thought, if we are going to replace HTTPHeaderMap, then we should not just use a Vector.  We shouldn't assume that the number of headers is always going to stay small.  That's how software becomes slow when it is pushed to its limits.

[youenn fablet]

Created attachment 269453 [details]
Using HTTPHeaderMap

[WebKit Commit Bot]

Attachment 269453 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchHeaders.h:65:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltins.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 269563 [details]
Adding some more tests

[youenn fablet]

(In reply to comment #32)
> Created attachment 269563 [details]
> Adding some more tests

There is not yet full coverage of the current code.
In particular Headers::guard is not tested except for None case.
This should be covered once Request and Response are actually added.

[WebKit Commit Bot]

Attachment 269563 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchHeaders.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltins.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 269570 [details]
Syncing tests

[WebKit Commit Bot]

Attachment 269570 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchHeaders.h:64:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltins.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 269570 [details]
Syncing tests

View in context: https://bugs.webkit.org/attachment.cgi?id=269570&action=review

Do we have test cases covering null strings?

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:77
> +    return name.startsWithIgnoringASCIICase(ASCIILiteral("Sec-")) || name.startsWithIgnoringASCIICase(ASCIILiteral("Proxy-"));

We need to overload startsWithIgnoringASCIICase to take either const char* or to take ASCIILiteral. It’s not OK to do it like this without changing the overloading, because this will create and destroy two String objects every time this is called; bad for performance.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:82
> +    return (equalIgnoringCase(name, "Set-Cookie") || equalIgnoringCase(name, "Set-Cookie2"));

This should be equalIgnoringASCIICase, not equalIgnoringCase, there is no reason for the general purpose Unicode case folding.

Also, no need for the parentheses here in WebKit coding style.

Now that my patch for https://bugs.webkit.org/show_bug.cgi?id=153266 has landed, this code should be:

    return equalLettersIgnoringASCIICase(name, "set-cookie") || equalLettersIgnoringASCIICase(name, "set-cookie2");

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:97
> +        {String mimeType = extractMIMETypeFromMediaType(value);
> +        return equalIgnoringCase(name, "application/x-www-form-urlencoded") || equalIgnoringCase(name, "multipart/form-data") || equalIgnoringCase(name, "text/plain");}

This is not correct WebKit coding style for the braces. Our style would be:

    case HTTPHeaderName::ContentType: {
        String mimeType = extractMIMETypeFromMediaType(value);
        return equalLettersIgnoringASCIICase(name, "application/x-www-form-urlencoded") || equalLettersIgnoringASCIICase(name, "multipart/form-data") || equalLettersIgnoringASCIICase(name, "text/plain");
    }

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:130
> +    if (!canWriteHeader(name, ASCIILiteral("invalid"), m_guard)) {

Seems a bit strange to use the actual string "invalid" here. Would the empty string work? If so that’s a bit more efficient than creating and destroying a String with "invalid" in it every time this function is called. There are other options if we really do need the string "invalid".

> Source/WebCore/Modules/fetch/FetchHeaders.h:35
> +#include <wtf/RefCounted.h>

Surprised this is needed. Doesn’t HTTPHeaderMap.h already include this?

> Source/WebCore/Modules/fetch/FetchHeaders.h:52
> +    ~FetchHeaders() { }

No need to write this explicitly; this is what C++ classes do by default if you don’t declare anything.

> Source/WebCore/Modules/fetch/FetchHeaders.h:60
> +    const HTTPHeaderMap& internalHeaders() const { return m_headers; }

Do these really need to be public? It’s strange to have this mixed in with actual public functions from the IDL. It should at least be in a different paragraph, and would be nice to be private instead of public.

> Source/WebCore/Modules/fetch/FetchHeaders.js:33
> +    if (headersInit === undefined)
> +            return this;

Incorrect indentation.

[youenn fablet]

Created attachment 269732 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to comment #37)
> Comment on attachment 269570 [details]
> Syncing tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269570&action=review
> 
> Do we have test cases covering null strings?

I updated fetch/api/headers-error.html and fetch/api/headers-basic.html to cover null,  undefined and other non-string values.

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:77
> > +    return name.startsWithIgnoringASCIICase(ASCIILiteral("Sec-")) || name.startsWithIgnoringASCIICase(ASCIILiteral("Proxy-"));
> 
> We need to overload startsWithIgnoringASCIICase to take either const char*
> or to take ASCIILiteral. It’s not OK to do it like this without changing the
> overloading, because this will create and destroy two String objects every
> time this is called; bad for performance.

OK.
I'll do that in a follow-up patch.

> 
> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:82
> > +    return (equalIgnoringCase(name, "Set-Cookie") || equalIgnoringCase(name, "Set-Cookie2"));
> 
> This should be equalIgnoringASCIICase, not equalIgnoringCase, there is no
> reason for the general purpose Unicode case folding.
> 
> Also, no need for the parentheses here in WebKit coding style.
> 
> Now that my patch for https://bugs.webkit.org/show_bug.cgi?id=153266 has
> landed, this code should be:
> 
>     return equalLettersIgnoringASCIICase(name, "set-cookie") ||
> equalLettersIgnoringASCIICase(name, "set-cookie2");

Fixed.

> > Source/WebCore/Modules/fetch/FetchHeaders.cpp:130
> > +    if (!canWriteHeader(name, ASCIILiteral("invalid"), m_guard)) {
> 
> Seems a bit strange to use the actual string "invalid" here. Would the empty
> string work? If so that’s a bit more efficient than creating and destroying
> a String with "invalid" in it every time this function is called. There are
> other options if we really do need the string "invalid".

I hesitated on this. "invalid" is part of the spec but empty string is just good enough.
I changed it.

> > Source/WebCore/Modules/fetch/FetchHeaders.h:60
> > +    const HTTPHeaderMap& internalHeaders() const { return m_headers; }
> 
> Do these really need to be public? It’s strange to have this mixed in with
> actual public functions from the IDL. It should at least be in a different
> paragraph, and would be nice to be private instead of public.

Removed it as it is not needed for this patch right now.

[WebKit Commit Bot]

Comment on attachment 269732 [details]
Patch for landing

Clearing flags on attachment: 269732

Committed r195530: <http://trac.webkit.org/changeset/195530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> > > Source/WebCore/Modules/fetch/FetchHeaders.cpp:77
> > > +    return name.startsWithIgnoringASCIICase(ASCIILiteral("Sec-")) || name.startsWithIgnoringASCIICase(ASCIILiteral("Proxy-"));
> > 
> > We need to overload startsWithIgnoringASCIICase to take either const char*
> > or to take ASCIILiteral. It’s not OK to do it like this without changing the
> > overloading, because this will create and destroy two String objects every
> > time this is called; bad for performance.
> 
> OK.
> I'll do that in a follow-up patch.

I filed bug 153419 for that purpose.