Bug 109844 - Add addHTTPHeaderField() method to ResourceResponse
Summary: Add addHTTPHeaderField() method to ResourceResponse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 10:14 PST by Chris Dumez
Modified: 2013-02-14 14:50 PST (History)
10 users (show)

See Also:


Attachments
Patch (7.83 KB, patch)
2013-02-14 10:57 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-02-14 10:14:49 PST
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.
Comment 1 Chris Dumez 2013-02-14 10:57:32 PST
Created attachment 188385 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-14 13:13:01 PST
Comment on attachment 188385 [details]
Patch

Clearing flags on attachment: 188385

Committed r142902: <http://trac.webkit.org/changeset/142902>
Comment 3 WebKit Review Bot 2013-02-14 13:13:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Alexey Proskuryakov 2013-02-14 14:10:00 PST
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.
Comment 5 Martin Robinson 2013-02-14 14:13:45 PST
At least in the case of the soup backend, updateFromSoupMessageHeaders is used to update a ResourceRequest with information from the network.
Comment 6 Adam Barth 2013-02-14 14:26:17 PST
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.
Comment 7 Alexey Proskuryakov 2013-02-14 14:47:27 PST
> 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!
Comment 8 Adam Barth 2013-02-14 14:50:41 PST
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.