Summary: | [curl] Merge http response header values | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Gal <galpeter> | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue | ||||||||||
Priority: | P2 | Keywords: | Curl | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 117300 | ||||||||||||
Attachments: |
|
Description
Peter Gal
2013-06-07 05:44:05 PDT
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. |