Bug 161736

Created attachment 288261 [details] Patch

Comment on attachment 288261 [details] Patch Attachment 288261 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2034282 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

Created attachment 288265 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 288261 [details] Patch Attachment 288261 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2034285 New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed.htm

Created attachment 288266 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5

Created attachment 288267 [details] Patch

Comment on attachment 288267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288267&action=review > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100 > - result.iterator->value = result.iterator->value + ", " + value; > + result.iterator->value = result.iterator->value + "," + value; Somebody once told me makeString is faster than operator+ because this does unnecessary allocations and copies. > LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10 > -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) > +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got "" Should these tests be changed?

>LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10 > > -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) > > +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got "" > > Should these tests be changed? I tried making the error messages more descriptive, but they would be flaky (see previous patch). The tests do not need to be fixed. It is the lower HTTP library that would need to be fixed as they insert spaces for some headers: ct, pragma... That should be done elsewhere

Created attachment 288278 [details] Using makeString

Thanks for the comments > > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100 > > - result.iterator->value = result.iterator->value + ", " + value; > > + result.iterator->value = result.iterator->value + "," + value; > > Somebody once told me makeString is faster than operator+ because this does > unnecessary allocations and copies. Done. > > LayoutTests/imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-header-allowed-expected.txt:10 > > -PASS XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) > > +FAIL XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type) assert_equals: expected "content-type," but got "" > > Should these tests be changed? As said above, we should be able to pass these tests, but that should be left to another bug. It might be the case that other ports might pass this test directly.

Comment on attachment 288278 [details] Using makeString View in context: https://bugs.webkit.org/attachment.cgi?id=288278&action=review > LayoutTests/imported/w3c/ChangeLog:10 > + This is due to the fact that the underlying HTTP code is probably reprocessing those headers and readding ', ' in lieu of ','. But it wasn't before? These tests passed before this change. I'm not convinced. I don't think we should regress this test.

> > LayoutTests/imported/w3c/ChangeLog:10 > > + This is due to the fact that the underlying HTTP code is probably reprocessing those headers and readding ', ' in lieu of ','. > > But it wasn't before? These tests passed before this change. I'm not > convinced. I don't think we should regress this test. Is is true that these tests were passing before the changes. It is also true that these tests were not conforming with the spec. The fact that some of these tests were passing is a proof that WebKit was not compliant with the spec. Now that this patch makes the test conforming and updates part of the implementation, we see that some tests are passing (which is good), while some still do not work. They do not work for two reasons: - headers are not set according user input (previous failing tests). - headers are reserialised at a lower level with a '' between values (new failing tests, Content-Type e.g.). This should be fixed at CFNetwork level. Plan is to file a radar once WebKit layer gets fixed.

Created attachment 288402 [details] Adding new test to keep same level of test coverage

If web platform tests are not conforming with the spec, web platform tests ought to be changed.

(In reply to comment #14) > If web platform tests are not conforming with the spec, web platform tests > ought to be changed. headers-combined was already changed in wpt repo. I will upload the other changed WPT tests in this patch once it gets landed in WebKit.

Or I can prepare the PR in parallel, I'll do it when possible

(In reply to comment #14) > If web platform tests are not conforming with the spec, web platform tests > ought to be changed. PR was merged today. The WPT tests are now changed according to this patch in W3C repo.

Comment on attachment 288402 [details] Adding new test to keep same level of test coverage View in context: https://bugs.webkit.org/attachment.cgi?id=288402&action=review > Source/WebCore/loader/CrossOriginAccessControl.cpp:134 > + headerBuffer.appendLiteral(","); This can use headerBuffer.append(',')

Created attachment 289038 [details] Patch for landing

Comment on attachment 289038 [details] Patch for landing Rejecting attachment 289038 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 289038, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: n "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit LayoutTests/imported/w3c/web-platform-tests/fetch/api/headers/headers-combine-expected.txt: needs update LayoutTests/imported/w3cqweb-platform-tests/fetch/api/headers/headers-combine-expected.txt: needs update update-index --refresh: command returned error: 1 Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/2085802

Created attachment 289047 [details] Patch for landing

Comment on attachment 289047 [details] Patch for landing Clearing flags on attachment: 289047 Committed r206014: <http://trac.webkit.org/changeset/206014>

All reviewed patches have been landed. Closing bug.

Comment on attachment 289047 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=289047&action=review > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100 > + result.iterator->value = makeString(result.iterator->value, ",", value); I think it would be slightly more efficient if you used ',' instead of "," here, skipping a call to the strlen function. Unless makeString does not know how to handle single characters.

(In reply to comment #25) > Comment on attachment 289047 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289047&action=review > > > Source/WebCore/platform/network/HTTPHeaderMap.cpp:100 > > + result.iterator->value = makeString(result.iterator->value, ",", value); > > I think it would be slightly more efficient if you used ',' instead of "," > here, skipping a call to the strlen function. Unless makeString does not > know how to handle single characters. Filed bug 162140 for that purpose