Bug 117342 – [curl] Merge http response header values

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Peter Gal]

If the server returns multiple headers with the same name then the values should be merged into one list where the values are separated by a ',' character. This behavior is described in the RFC 2616 section 4.2 (http://tools.ietf.org/html/rfc2616#section-4.2).

[Peter Gal]

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.

[Peter Gal]

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.

[Kenneth Rohde Christiansen]

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?

[Peter Gal]

(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.

[Brent Fulgham]

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?

[Peter Gal]

(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?

[Peter Gal]

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.

[WebKit Commit Bot]

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.

[Peter Gal]

Created attachment 204580 [details]
style fixed patch

fixed the style error

[Brent Fulgham]

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.

[Peter Gal]

(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.
.

[Peter Gal]

Created attachment 204625 [details]
updated patch

[Brent Fulgham]

Comment on attachment 204625 [details]
updated patch

r=me.  Thanks for working on this!

[WebKit Commit Bot]

Comment on attachment 204625 [details]
updated patch

Clearing flags on attachment: 204625

Committed r151580: <http://trac.webkit.org/changeset/151580>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.