Fix removal of headers

Fix removal of headers

HttpResponseHeaders::RemoveHeaderWithValue did not correctly remove headers if the passed value contained a comma. For a header "Foo: bar, baz", RemoveHeaderWithValue("Foo", "bar, baz") would be be a non-op. This CL fixes that. As the web request API is the only consumer of this function, there will be no side effects on other code due to the changed behavior.

BUG=137396
TEST=no


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

[battre, 2012-07-19 09:56:21]

Hi Ricardo,

could you please review this CL?

Thanks,
Dominic

[rvargas (doing something else), 2012-07-19 19:11:02]

Note that the description is wrong in that the current code eliminates the line
when "foo: bar" is used.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.cc:314: void
HttpResponseHeaders::MergeWithHeadersWithValue(
Please remove this method.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.cc:373: HttpUtil::HeadersIterator headers(
Shouldn't this code use EnumerateHeaderLines?

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers.h (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.h:70: void RemoveHeaderLine(const std::string&
name, const std::string& values);
nit: I'm not sure about using plural for value... it gives the impression that a
list of values is going to be evaluated or considered independently of other
values in that line.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers_unittest.cc (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers_unittest.cc:1645: const char* to_remove_values;
nit: I would still use singular here.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers_unittest.cc:1690: "Foo: bar, baz\n"
How about adding another "Foo: bar\n" line here (to cover multiple lines with
the same name).

[rvargas (doing something else), 2012-07-19 19:12:42]

On 2012/07/19 19:11:02, rvargas wrote:
> Note that the description is wrong in that the current code eliminates the
line
> when "foo: bar" is used.

Of course, I meant RemoveHeaderWithValue("Foo", "bar")


> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> File net/http/http_response_headers.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> net/http/http_response_headers.cc:314: void
> HttpResponseHeaders::MergeWithHeadersWithValue(
> Please remove this method.
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> net/http/http_response_headers.cc:373: HttpUtil::HeadersIterator headers(
> Shouldn't this code use EnumerateHeaderLines?
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> File net/http/http_response_headers.h (right):
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> net/http/http_response_headers.h:70: void RemoveHeaderLine(const std::string&
> name, const std::string& values);
> nit: I'm not sure about using plural for value... it gives the impression that
a
> list of values is going to be evaluated or considered independently of other
> values in that line.
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> File net/http/http_response_headers_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> net/http/http_response_headers_unittest.cc:1645: const char* to_remove_values;
> nit: I would still use singular here.
> 
>
https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
> net/http/http_response_headers_unittest.cc:1690: "Foo: bar, baz\n"
> How about adding another "Foo: bar\n" line here (to cover multiple lines with
> the same name).

[battre, 2012-07-23 13:31:11]

Thanks for the review. PTAL.

Best regards,
Dominic

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers.cc (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.cc:314: void
HttpResponseHeaders::MergeWithHeadersWithValue(
On 2012/07/19 19:11:02, rvargas wrote:
> Please remove this method.

Good catch. Thanks.

Done.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.cc:373: HttpUtil::HeadersIterator headers(
On 2012/07/19 19:11:02, rvargas wrote:
> Shouldn't this code use EnumerateHeaderLines?

Done.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers.h (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers.h:70: void RemoveHeaderLine(const std::string&
name, const std::string& values);
On 2012/07/19 19:11:02, rvargas wrote:
> nit: I'm not sure about using plural for value... it gives the impression that
a
> list of values is going to be evaluated or considered independently of other
> values in that line.

Done.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
File net/http/http_response_headers_unittest.cc (right):

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers_unittest.cc:1645: const char* to_remove_values;
On 2012/07/19 19:11:02, rvargas wrote:
> nit: I would still use singular here.

Done.

https://chromiumcodereview.appspot.com/10809011/diff/1/net/http/http_response...
net/http/http_response_headers_unittest.cc:1690: "Foo: bar, baz\n"
On 2012/07/19 19:11:02, rvargas wrote:
> How about adding another "Foo: bar\n" line here (to cover multiple lines with
> the same name).

Done.

[rvargas (doing something else), 2012-07-23 18:14:12]

LGTM

[commit-bot: I haz the power, 2012-07-24 08:51:19]

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

[commit-bot: I haz the power, 2012-07-24 11:15:03]

Change committed as 148078