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.
Created attachment 305017 [details] Patch
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 ?
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
Comment on attachment 305017 [details] Patch After reading the spec, change looks good otherwise.
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.
(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?
(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.
Created attachment 305097 [details] Patch
Comment on attachment 305097 [details] Patch r=me
Comment on attachment 305097 [details] Patch Clearing flags on attachment: 305097 Committed r214252: <http://trac.webkit.org/changeset/214252>
All reviewed patches have been landed. Closing bug.
Thie patch uses append("xxx") in a few places where it should instead be appendLiteral("xxx").