XMLHttpRequest: getAllResponseHeaders() should lowercase header names before sorting

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

Youenn advocated for simplifying getAllResponseHeaders() to expose less details (lowercasing, sorting) and align with Fetch, but Safari TP doesn't do this. Is this actually what WebKit wants?

web-platform-tests at:

  XMLHttpRequest/getallresponseheaders.htm

Standard issue at https://github.com/whatwg/xhr/issues/109.

[youenn fablet]

Created attachment 305017 [details]
Patch

[Chris Dumez]

Comment on attachment 305017 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        XMLHttpRequest: getAllResponseHeaders()

We already supported it so this should explain what you changed.

> Source/WebCore/ChangeLog:9
> +

Can you please point to the spec in your changeling so we know why this is the expected behavior?

> Source/WebCore/xml/XMLHttpRequest.cpp:930
> +

Why the extra line here?

> Source/WebCore/xml/XMLHttpRequest.cpp:938
> +        for (const auto& header : m_response.httpHeaderFields()) {

We usually omit the const here.

> Source/WebCore/xml/XMLHttpRequest.cpp:941
> +            stringBuilder.append(':');

appendLiteral(": ");

> Source/WebCore/xml/XMLHttpRequest.cpp:942
> +            stringBuilder.append(' ');

not needed.

> Source/WebCore/xml/XMLHttpRequest.cpp:944
> +            stringBuilder.append('\r');

appendLiteral("\r\n");

> Source/WebCore/xml/XMLHttpRequest.cpp:945
> +            stringBuilder.append('\n');

Not needed.

> LayoutTests/imported/w3c/ChangeLog:3
> +        fix-169286

?

[Chris Dumez]

Comment on attachment 305017 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        XMLHttpRequest: getAllResponseHeaders()
> 
> We already supported it so this should explain what you changed.

You basically now sort and lowercase them.

>> Source/WebCore/ChangeLog:9
>> +
> 
> Can you please point to the spec in your changeling so we know why this is the expected behavior?

Basically this link:
https://xhr.spec.whatwg.org/#the-getallresponseheaders()-method

[Chris Dumez]

Comment on attachment 305017 [details]
Patch

After reading the spec, change looks good otherwise.

[youenn fablet]

Sure, I'll add more information.
The thing is we should decide whether we want to go in that direction.

In WebKit, we do not currently handle header name casing for usual names.
To mitigate the issue of showing implementation differences through the API, the idea is to lowercase header names when web app reads them.
Hence the changes made to getAllResponseHeaders.

[Chris Dumez]

(In reply to youenn fablet from comment #5)
> Sure, I'll add more information.
> The thing is we should decide whether we want to go in that direction.
> 
> In WebKit, we do not currently handle header name casing for usual names.
> To mitigate the issue of showing implementation differences through the API,
> the idea is to lowercase header names when web app reads them.
> Hence the changes made to getAllResponseHeaders.

How do other browsers behave here? Do they match the current spec?

[youenn fablet]

(In reply to Chris Dumez from comment #6)
> (In reply to youenn fablet from comment #5)
> > Sure, I'll add more information.
> > The thing is we should decide whether we want to go in that direction.
> > 
> > In WebKit, we do not currently handle header name casing for usual names.
> > To mitigate the issue of showing implementation differences through the API,
> > the idea is to lowercase header names when web app reads them.
> > Hence the changes made to getAllResponseHeaders.
> 
> How do other browsers behave here? Do they match the current spec?

Other browsers are not lowercasing atm, since it is a new spec change.
Given that HTTP/2 is lowercasing headers and that it is exposing unnecessary implementation differences to web apps, it makes sense to me to go in that direction.

[youenn fablet]

Created attachment 305097 [details]
Patch

[Chris Dumez]

Comment on attachment 305097 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 305097 [details]
Patch

Clearing flags on attachment: 305097

Committed r214252: <http://trac.webkit.org/changeset/214252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Thie patch uses append("xxx") in a few places where it should instead be appendLiteral("xxx").