Bug 117342

For example if the server returns the following headers: X-MY: one X-MY: two then the client should merge these values into a single: X-MY: one, two entry. This is tested with the 'http/tests/xmlhttprequest/xmlhttprequest-test-custom-headers.html' testcase.

Created attachment 204033 [details] proposed patch The current implement used the setHTTPHeaderField method which replaced the previous header. There the addHTTPHeaderField method which will concatenate the values if required.

Comment on attachment 204033 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204033&action=review > Source/WebCore/ChangeLog:9 > + No new tests, already covered by existing ones, for example: > + http/tests/xmlhttprequest/xmlhttprequest-test-custom-headers.html why no updated results?

(In reply to comment #3) > (From update of attachment 204033 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204033&action=review > > > Source/WebCore/ChangeLog:9 > > + No new tests, already covered by existing ones, for example: > > + http/tests/xmlhttprequest/xmlhttprequest-test-custom-headers.html > > why no updated results? The test results are ok, it's just that the test was failing before.

Comment on attachment 204033 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204033&action=review > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:320 > + d->m_response.addHTTPHeaderField(header.left(splitPos), header.substring(splitPos+1).stripWhiteSpace()); Are there ever cases where we do want to replace the HTTP header field, rather than appending?

(In reply to comment #5) > (From update of attachment 204033 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204033&action=review > > > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:320 > > + d->m_response.addHTTPHeaderField(header.left(splitPos), header.substring(splitPos+1).stripWhiteSpace()); > > Are there ever cases where we do want to replace the HTTP header field, rather than appending? I was wondering on that also, so I've re-read the rfc multiple times and as far as I can see most of the headers are allowed to be merged. If I remember correctly libsoup merges all HTTP headers. In the blackberry they have a white list to specify which headers can be merged. Should I do something like the blackberry guys and add a white list?

Created attachment 204578 [details] advanced patch So then... After studying the HTTP RFC and other things I've created a slightly more advanced implementation to merge HTTP header values.

Attachment 204578 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/curl/ResourceHandleManager.cpp']" exit_code: 1 Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:274: Omit int when using unsigned [runtime/unsigned] [1] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 204580 [details] style fixed patch fixed the style error

Comment on attachment 204580 [details] style fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204580&action=review Looks great! I had a couple of minor suggestions. If you can clarify that comment I will be happy to approve the patch. > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:241 > + static const char *appendableHeaders[] = { This should be "static const char*" The fact that this is a "const char pointer" is part of the type. > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:270 > + // Custom headers usually starts with 'X-', allow them. "starts" should be "start" (syntax). If they usually start with "X-", why do we test with "x-" (case insensitive)? Do they usually start with either "X-" or "x-" and we are handling this case and exiting early? Maybe the comments should be "Custom headers start with 'X-', and need no further checking.

(In reply to comment #10) > (From update of attachment 204580 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204580&action=review > > Looks great! I had a couple of minor suggestions. If you can clarify that comment I will be happy to approve the patch. > > > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:241 > > + static const char *appendableHeaders[] = { > > This should be "static const char*" The fact that this is a "const char pointer" is part of the type. > Ok. > > Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:270 > > + // Custom headers usually starts with 'X-', allow them. > > "starts" should be "start" (syntax). > > If they usually start with "X-", why do we test with "x-" (case insensitive)? Do they usually start with either "X-" or "x-" and we are handling this case and exiting early? > > Maybe the comments should be "Custom headers start with 'X-', and need no further checking. The http headers are case insensitive (at least that's what I read in the rfc) and most of the times the first character is in uppercase. I'll update the comment. .

Created attachment 204625 [details] updated patch

Comment on attachment 204625 [details] updated patch r=me. Thanks for working on this!

Comment on attachment 204625 [details] updated patch Clearing flags on attachment: 204625 Committed r151580: <http://trac.webkit.org/changeset/151580>

All reviewed patches have been landed. Closing bug.