Improve merging of header modifications in webRequest.OnHeadersReceived

Improve merging of header modifications in webRequest.OnHeadersReceived

This CL enables two independent extensions to modify headers of HTTP responses in case they don't conflict (two extension try to edit the same header).


BUG=none
TEST=no


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110900

[battre, 2011-11-11 15:01:56]

Please review.

Thanks!
Dominic

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_apitest.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_apitest.cc:75:
IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
/*DISABLED_*/WebRequestComplex) {
I will revert this before committing.

[Matt Perry, 2011-11-11 22:05:08]

mostly lgtm, couple small issues

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_helpers.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_helpers.cc:384:
ResponseHeader toLowerCase(const ResponseHeader& header) {
ToLowerCase

also make this static, or put it in an anonymous namespace

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_helpers.cc:420: // intention
was to modify it to different values and consider this a
this seems like a strange assumption to me

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_unittest.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_unittest.cc:50: bool
SearchKey(const Collection& collection, const Key& key) {
rename to Contains?

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers.h
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:68: void RemoveHeader(const std::string& name,
const std::string& value);
google style forbids overloading function names. maybe RemoveHeaderWithValue

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:313: void MergeWithHeaders(const std::string&
raw_headers,
ditto about function name

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers_...
File net/http/http_response_headers_unittest.cc (right):

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers_...
net/http/http_response_headers_unittest.cc:1660: };
how about a test case where the name matches but the value doesn't, and ensure
that the header is not removed

[willchan no longer on Chromium, 2011-11-11 22:46:28]

LGTM mod Matt's comments.

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers.h
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:310: // |header_to_remove_name| are compared
case-insensitively.
s/are/is/?

[battre, 2011-11-17 12:01:04]

Forgot to send my comments. :-)

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_helpers.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_helpers.cc:384:
ResponseHeader toLowerCase(const ResponseHeader& header) {
On 2011/11/11 22:05:08, Matt Perry wrote:
> ToLowerCase
> 
> also make this static, or put it in an anonymous namespace

Done.

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_helpers.cc:420: // intention
was to modify it to different values and consider this a
On 2011/11/11 22:05:08, Matt Perry wrote:
> this seems like a strange assumption to me

Do you have a better proposal?

Say you have a response header "Foo: 1". Extension A changes it to "Foo: 2" and
extension B to "Foo: 3". If you allow deleting twice and use "Add" and "Delete"
as the only operations considered, you would end up with "Foo: 2" and "Foo: 3"
in the conflict resolved output.

In the case of outgoing requests we look at only the header key (if extension A
and B fiddled with "Foo", there is a conflict). This does not work for incoming
responses. E.g. the "Set-Cookie" key occurs several times in a request.

I hope I made my point clear. Otherwise, I can construct a set of examples where
1) We cannot sensibly define a "Edit" operation as long as multiple response
headers can share the same key.
2) We cannot sensibly conclude what the extension tried to do (delete a header
or modify a header)

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_unittest.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_unittest.cc:50: bool
SearchKey(const Collection& collection, const Key& key) {
On 2011/11/11 22:05:08, Matt Perry wrote:
> rename to Contains?

I have renamed it to "Contains". Note that we already have ContainsKey in
stl_util.h, but at least we are in an anonymous namespace here.

I did not see any traits in the STL container classes that I could use to
implement two different versions of ContainsKey, one for set and friends (that
have find method), and one for vector and friends (that rely on std::find(...)).
So in case you have any good ideas in that direction...

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers.h
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:68: void RemoveHeader(const std::string& name,
const std::string& value);
On 2011/11/11 22:05:08, Matt Perry wrote:
> google style forbids overloading function names. maybe RemoveHeaderWithValue

Done.

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:310: // |header_to_remove_name| are compared
case-insensitively.
On 2011/11/11 22:46:28, willchan wrote:
> s/are/is/?

Done.

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers....
net/http/http_response_headers.h:313: void MergeWithHeaders(const std::string&
raw_headers,
On 2011/11/11 22:05:08, Matt Perry wrote:
> ditto about function name

Done.

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers_...
File net/http/http_response_headers_unittest.cc (right):

http://codereview.chromium.org/8511063/diff/1/net/http/http_response_headers_...
net/http/http_response_headers_unittest.cc:1660: };
On 2011/11/11 22:05:08, Matt Perry wrote:
> how about a test case where the name matches but the value doesn't, and ensure
> that the header is not removed

Done.

[Matt Perry, 2011-11-17 20:04:09]

lgtm

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_helpers.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_helpers.cc:420: // intention
was to modify it to different values and consider this a
On 2011/11/17 12:01:04, battre wrote:
> On 2011/11/11 22:05:08, Matt Perry wrote:
> > this seems like a strange assumption to me
> 
> Do you have a better proposal?
> 
> Say you have a response header "Foo: 1". Extension A changes it to "Foo: 2"
and
> extension B to "Foo: 3". If you allow deleting twice and use "Add" and
"Delete"
> as the only operations considered, you would end up with "Foo: 2" and "Foo: 3"
> in the conflict resolved output.
> 
> In the case of outgoing requests we look at only the header key (if extension
A
> and B fiddled with "Foo", there is a conflict). This does not work for
incoming
> responses. E.g. the "Set-Cookie" key occurs several times in a request.
> 
> I hope I made my point clear. Otherwise, I can construct a set of examples
where
> 1) We cannot sensibly define a "Edit" operation as long as multiple response
> headers can share the same key.
> 2) We cannot sensibly conclude what the extension tried to do (delete a header
> or modify a header)

It still seems possible to me to distinguish between "2 extensions modify header
Foo", and "2 extensions delete header Foo". In your example, if both extensions
delete header "Foo" and do not add another one, there is no conflict. You'd have
to examine both the deleted and added headers to decide.

That said, I don't think it's that important an edge case to worry about right
now, so I'm OK with the code as is.

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_webrequest_api_unittest.cc (right):

http://codereview.chromium.org/8511063/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_webrequest_api_unittest.cc:50: bool
SearchKey(const Collection& collection, const Key& key) {
On 2011/11/17 12:01:04, battre wrote:
> On 2011/11/11 22:05:08, Matt Perry wrote:
> > rename to Contains?
> 
> I have renamed it to "Contains". Note that we already have ContainsKey in
> stl_util.h, but at least we are in an anonymous namespace here.
> 
> I did not see any traits in the STL container classes that I could use to
> implement two different versions of ContainsKey, one for set and friends (that
> have find method), and one for vector and friends (that rely on
std::find(...)).
> So in case you have any good ideas in that direction...

I think this 1-off is fine.

[commit-bot: I haz the power, 2011-11-21 08:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/8511063/11001

[commit-bot: I haz the power, 2011-11-21 11:07:25]

Change committed as 110900