WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
169286
XMLHttpRequest: getAllResponseHeaders() should lowercase header names before sorting
https://bugs.webkit.org/show_bug.cgi?id=169286
Summary
XMLHttpRequest: getAllResponseHeaders() should lowercase header names before ...
Anne van Kesteren
Reported
2017-03-07 10:07:31 PST
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
.
Attachments
Patch
(5.74 KB, patch)
2017-03-21 13:14 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch
(5.86 KB, patch)
2017-03-22 08:37 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2017-03-21 13:14:34 PDT
Created
attachment 305017
[details]
Patch
Chris Dumez
Comment 2
2017-03-21 13:20:37 PDT
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 3
2017-03-21 13:23:21 PDT
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 4
2017-03-21 13:31:43 PDT
Comment on
attachment 305017
[details]
Patch After reading the spec, change looks good otherwise.
youenn fablet
Comment 5
2017-03-21 15:07:06 PDT
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
Comment 6
2017-03-21 21:28:30 PDT
(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
Comment 7
2017-03-22 08:17:48 PDT
(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
Comment 8
2017-03-22 08:37:12 PDT
Created
attachment 305097
[details]
Patch
Chris Dumez
Comment 9
2017-03-22 08:43:47 PDT
Comment on
attachment 305097
[details]
Patch r=me
WebKit Commit Bot
Comment 10
2017-03-22 10:14:40 PDT
Comment on
attachment 305097
[details]
Patch Clearing flags on attachment: 305097 Committed
r214252
: <
http://trac.webkit.org/changeset/214252
>
WebKit Commit Bot
Comment 11
2017-03-22 10:14:43 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12
2017-03-30 11:58:23 PDT
Thie patch uses append("xxx") in a few places where it should instead be appendLiteral("xxx").
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug