Bug 109844 – Add addHTTPHeaderField() method to ResourceResponse

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

ResourceRequestBase provides both setHTTPHeaderField() and addHTTPHeaderField(). However, ResourceResponseBase only provides setHTTPHeaderField(). This is a bit inconsistent. As a result, the addHTTPHeaderField() functionality's implementation is duplicated in several ports (at least chromium and soup).

I think it would be beneficial to implement addHTTPHeaderField() once in ResourceResponseBase to avoid code duplication and make the ResourceResponse's API more consistent with the ResourceRequest one.

[Chris Dumez]

Created attachment 188385 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188385 [details]
Patch

Clearing flags on attachment: 188385

Committed r142902: <http://trac.webkit.org/changeset/142902>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I do not think that the patch is right. There should be is no expectation of consistency between ResourceRequest and ResourceResponse - they are totally asymmetric by design.

The response comes form the network, and no one should be adding header fields to it, which is why there should be no public addHTTPHeaderField function.

[Martin Robinson]

At least in the case of the soup backend, updateFromSoupMessageHeaders is used to update a ResourceRequest with information from the network.

[Adam Barth]

This patch just moves duplicate code into a location where it can be shared.

> The response comes form the network, and no one should be adding header fields to it, which is why there should be no public addHTTPHeaderField function.

Chromium as a public API for adding HTTP header fields to ResourceResponses.  We can go look at the callers if you want to see why they need that facility.

[Alexey Proskuryakov]

> At least in the case of the soup backend, updateFromSoupMessageHeaders is used to update a ResourceRequest with information from the network.

There is certainly a place where a port needs to set the headers from a low level network response, bit it doesn't need a public function for this.

In fact, I don't think that this patch is a big deal - we already have all kinds of setters on ResourceResponseBase because of the way blobs were implemented. But I think that strategically, ResourceResponse should be immutable, and we should be removing setters, not adding them.

> Chromium as a public API for adding HTTP header fields to ResourceResponses.  We can go look at the callers if you want to see why they need that facility.

Interesting!

[Adam Barth]

https://code.google.com/p/chromium/codesearch#search/&q=addHTTPHeaderField&sq=package:chromium&type=cs

Looks like it's one of the ways the network stack builds a ResourceResponse.