When the header already exists the values is appended after a ',' in both cases, but for common headers ", " is used while for uncommon headers ',' is used. This makes test imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm to fail because of the space difference: -PASS XMLHttpRequest: getResponseHeader() case-insensitive matching +FAIL XMLHttpRequest: getResponseHeader() case-insensitive matching assert_equals: expected "1, 2" but got "1,2" This doesn't affect ports using CF, because CF already joins the headers, so they don't use HTTPHeaderMap::add(), but HTTPHeaderMap::set()
Created attachment 301153 [details] Patch
Comment on attachment 301153 [details] Patch Attachment 301153 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3062391 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html http/tests/xmlhttprequest/check-combining-headers.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm http/tests/xmlhttprequest/web-apps/005.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm
Created attachment 301154 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301153 [details] Patch Attachment 301153 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3062466 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html http/tests/xmlhttprequest/check-combining-headers.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm http/tests/xmlhttprequest/web-apps/005.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm
Created attachment 301155 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Maybe it's imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm the one wrong, and it should be ',' instead of ", ". For example, imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html was updated in r205743 removing the space.
Comment on attachment 301153 [details] Patch Attachment 301153 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3062468 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html http/tests/xmlhttprequest/check-combining-headers.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm http/tests/xmlhttprequest/web-apps/005.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm
Created attachment 301156 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301153 [details] Patch Attachment 301153 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3062472 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-combine.html http/tests/xmlhttprequest/check-combining-headers.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-case-insensitive.htm http/tests/xmlhttprequest/web-apps/005.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm
Created attachment 301157 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301153 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301153&action=review > Source/WebCore/ChangeLog:11 > + space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use And yet some tests are failing on Mac? :)
(In reply to comment #11) > Comment on attachment 301153 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301153&action=review > > > Source/WebCore/ChangeLog:11 > > + space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use > > And yet some tests are failing on Mac? :) I meant for that particular test, that is not failing on mac with the patch applied. I don't understand why the other tests can pass, though.
Ok, looking at the tests in detail now I understand better what is going on http/tests/xmlhttprequest/check-combining-headers.html imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm These have FAIL lines in the expectations, like: FAIL XMLHttpRequest: setRequestHeader() - combining headers (Authorization) assert_equals: Combined header value should be t1,t2 expected "t1,t2" but got "t1, t2" Whatever patch we write for this bug should make those pass. The other ones failing expect ',' not ", ". And finally the one I was trying to fix expects ", " but it should really expect ','. According to the HTTP spec, the space is optional, but fetch spec says it shouldn't be any trailing/leaving whitespace in values, see https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it was changed in w3c web platform tests, but I think they forgot about imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm. I guess it's not a good idea to change imported tests, so we can simply add a failure expectation for this test. So, let's be consistent and use ',' everywhere. I'll submit a new patch.
Created attachment 301252 [details] Updated patch
(In reply to comment #13) > And finally the one I was trying to fix expects ", " but it should really > expect ','. According to the HTTP spec, the space is optional, but fetch > spec says it shouldn't be any trailing/leaving whitespace in values, see > https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it > was changed in w3c web platform tests, but I think they forgot about > imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case- > insensitive.htm. I guess it's not a good idea to change imported tests, so > we can simply add a failure expectation for this test. (CC Youenn. How hard would it be to upstream this fix?)
(In reply to comment #15) > (In reply to comment #13) > > And finally the one I was trying to fix expects ", " but it should really > > expect ','. According to the HTTP spec, the space is optional, but fetch > > spec says it shouldn't be any trailing/leaving whitespace in values, see > > https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it > > was changed in w3c web platform tests, but I think they forgot about > > imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case- > > insensitive.htm. I guess it's not a good idea to change imported tests, so > > we can simply add a failure expectation for this test. > > (CC Youenn. How hard would it be to upstream this fix?) It is just a Pull Request on Github. It is not hard to fix a WPT.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > And finally the one I was trying to fix expects ", " but it should really > > > expect ','. According to the HTTP spec, the space is optional, but fetch > > > spec says it shouldn't be any trailing/leaving whitespace in values, see > > > https://fetch.spec.whatwg.org/#concept-header-list-combine. That's why it > > > was changed in w3c web platform tests, but I think they forgot about > > > imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case- > > > insensitive.htm. I guess it's not a good idea to change imported tests, so > > > we can simply add a failure expectation for this test. > > > > (CC Youenn. How hard would it be to upstream this fix?) > > It is just a Pull Request on Github. It is not hard to fix a WPT. https://github.com/w3c/web-platform-tests If it's too much trouble, send me a patch and I can help marking the PR on GitHub.
Comment on attachment 301252 [details] Updated patch Attachment 301252 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3068634 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm
Created attachment 301253 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301252 [details] Updated patch Attachment 301252 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3068636 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm
Created attachment 301254 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 301252 [details] Updated patch Attachment 301252 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3068645 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm
Created attachment 301258 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 301252 [details] Updated patch Attachment 301252 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3068641 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm
Created attachment 301259 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Fetch spec may change back to ", ". See https://github.com/whatwg/xhr/issues/108 for some discussions about it. Note that for some well-known headers, CFNetwork is converting "," in ", " on the fly. This may be an issue if a user specifically wants no space in such header values.
Also, bots are still failing with the latest patch....
(In reply to comment #27) > Also, bots are still failing with the latest patch.... Yes, precisely because CF generates ", ". If we decide to use ',' then CF based ports will need to add platform specific results for these tests.
Comment on attachment 301252 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=301252&action=review Please fix the failing test on the Cocoa platforms before landing. I suspect that the comments about this code only affecting ports that don’t use CF was incorrect, given that a test result changed! > LayoutTests/imported/w3c/ChangeLog:3 > + HTTPHeaderMap add methods are not consistent to each other Either "are not consistent" or "are not consistent with each other" would be OK. But "are not consistent to each other" is not correct English grammar. This is C++, so these are "functions", not "methods". But, also, why be so vague about what the inconsistency is? The comment should mention the space after the comma instead of leaving a mystery about what the difference is. I would use this title: REGRESSION (r206014): HTTPHeaderMap does not consistently use comma without space to separate values of header fields > Source/WebCore/ChangeLog:14 > + When the header already exists the values is appended after a ',' in both cases, but for common headers ", " is > + used while for uncommon headers ',' is used. This makes test > + imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm to fail because of the > + space difference. This doesn't affect ports using CF, because CF already joins the headers, so they don't use > + HTTPHeaderMap::add(), but HTTPHeaderMap::set(). According to the HTTP spec, the space is optional, but fetch > + spec says it shouldn't be any trailing/leading whitespace in values. So let's use ',' everywhere and update the > + tests results to expect that. If you do a bit more research on the history of this function, you will see that this inconsistency was introduced by <https://trac.webkit.org/changeset/206014>, a change that removed the space to match the Fetch specification. So I think the explanation is simpler than what you write here: We simply need to finish the job that was only partly done in that change.
(In reply to comment #28) > > Also, bots are still failing with the latest patch.... > > Yes, precisely because CF generates ", ". If we decide to use ',' then CF > based ports will need to add platform specific results for these tests. That is an interesting problem. Please talk with Youenn and other people from Apple about how we can resolve this!
We want WebKit to be consistent here. So the question we want to decide on is whether we want to use "," or ", ". The fetch spec is mandating "," atm but may change if there is not enough support for it. According IETF, they should be semantically equivalent... XHR implementation is using ", " but XHR spec is using whatever fetch spec prescribes. Other browsers afaik, are still using ", ". For the Mac port, WebKit should be able to send headers without CFNetwork changing them on the fly.
Created attachment 301490 [details] Patch for landing Let's do what the current tests expect for now then.
Comment on attachment 301490 [details] Patch for landing Attachment 301490 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3117296 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-case-insensitive.htm
Created attachment 301528 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 301588 [details] Patch for landing
Comment on attachment 301588 [details] Patch for landing Clearing flags on attachment: 301588 Committed r212355: <http://trac.webkit.org/changeset/212355>
All reviewed patches have been landed. Closing bug.