HttpRequestHeaders refactor.

HttpRequestHeaders refactor.
* Create HttpRequestHeaders.
* Switch HttpNetworkTransaction to build request headers.
TODO: Change extra_headers to use HttpRequestHeaders.
BUG=22588
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42300

[eroman, 2010-03-16 09:11:54]

Overall looks good.
... please upload the unittests which seem to be missing.

**Design question**

Have you considered doing a hash-map based implementation?

Instead of a vector, I could imagine having a map from 
   lower_case_key => (insertion_order, key, value)

Since we do quite a few lookups throughout the code (especially in say
HttpCache), I think this could be a worthwhile tradeoff. Of course, it would
mean that in ToString() we would sort values based on their |insertion_order|,
but that seems OK since ToString() is called less frequently then Lookup/Remove.

http://codereview.chromium.org/1039001/diff/1/5
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1039001/diff/1/5#newcode9
net/http/http_network_transaction.cc:9: #include "base/string_util.h"
order this lower.

http://codereview.chromium.org/1039001/diff/1/5#newcode63
net/http/http_network_transaction.cc:63:
GetHostAndOptionalPort(request_info->url).c_str());
drop the c_str().

http://codereview.chromium.org/1039001/diff/1/5#newcode89
net/http/http_network_transaction.cc:89: StringPrintf("%" PRIu64,
upload_data_stream->size()));
Uint64ToString(upload_data_stream->size())

http://codereview.chromium.org/1039001/diff/1/5#newcode165
net/http/http_network_transaction.cc:165: "CONNECT",
GetHostAndPort(request_info->url).c_str(), "1.1");
drop .c_str().

http://codereview.chromium.org/1039001/diff/1/5#newcode167
net/http/http_network_transaction.cc:167:
GetHostAndOptionalPort(request_info->url).c_str());
drop .c_str().

http://codereview.chromium.org/1039001/diff/1/5#newcode173
net/http/http_network_transaction.cc:173: request_info->user_agent.c_str());
drop .c_str().

http://codereview.chromium.org/1039001/diff/1/5#newcode987
net/http/http_network_transaction.cc:987: HttpRequestHeaders request_headers,
authorization_headers;
can you declare these 1 per line?

http://codereview.chromium.org/1039001/diff/1/7
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1039001/diff/1/7#newcode38
net/http/http_request_headers.cc:38: void HttpRequestHeaders::SetHeader(const
std::string& key,
So the expected behavior on replacement is that it overwrites in the location of
the old header. I wander if it wouldn't make more sense to have it show up at
the end instead.

http://codereview.chromium.org/1039001/diff/1/7#newcode57
net/http/http_request_headers.cc:57: const std::string::size_type key_end_index
= header_line.find(": ");
Shouldn't you be allowed to have LWS around the colon? (we allow it in response
headers).

http://codereview.chromium.org/1039001/diff/1/7#newcode70
net/http/http_request_headers.cc:70: LOG(DFATAL) << "\"" << header_line << "\"
is missing header value.";
Isn't it OK to have a blank value? (We allow it in response headers).

http://codereview.chromium.org/1039001/diff/1/7#newcode96
net/http/http_request_headers.cc:96: "%s %s HTTP/%s\r\n", method_.c_str(),
path_.c_str(), version_.c_str());
indentation

http://codereview.chromium.org/1039001/diff/1/7#newcode111
net/http/http_request_headers.cc:111: if (LowerCaseEqualsASCII(it->key,
lowercase_key.c_str()))
I wander if you can't use StartsWithASCII() to avoid calling .c_str() here (it
has a case insensitive bool).

http://codereview.chromium.org/1039001/diff/1/8
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1039001/diff/1/8#newcode14
net/http/http_request_headers.h:14: class HttpRequestHeaders {
Please add a header documentation, and maybe explain the approach this uses to
maintain ordering, handling of duplicates, and insertion/lookup cost,
case-insensitive + case preserving, etc...

http://codereview.chromium.org/1039001/diff/1/8#newcode35
net/http/http_request_headers.h:35: void RemoveHeader(const std::string& key);
Please describe what happens if |key| is not present, case insensitivity, and
what happens if there are multiple values with the same key when RemoveHeader is
called.

http://codereview.chromium.org/1039001/diff/1/8#newcode37
net/http/http_request_headers.h:37: void AddHeaderFromString(const std::string&
header_line);
Please describe behavior for malformed inputs, and whitespace.

http://codereview.chromium.org/1039001/diff/1/8#newcode38
net/http/http_request_headers.h:38: void MergeFrom(const HttpRequestHeaders&
other);
The MergeFrom() name wasn't very clear to me. Have you considered something like
AddAll / Append ?

http://codereview.chromium.org/1039001/diff/1/8#newcode40
net/http/http_request_headers.h:40: std::string ToString() const;
I think it is worth mentioning that this includes the trailing \r\n as well.

http://codereview.chromium.org/1039001/diff/1/8#newcode55
net/http/http_request_headers.h:55: HeaderVector::const_iterator
FindHeader(const std::string& key) const;
why do we need a const version. Where is this used?

http://codereview.chromium.org/1039001/diff/1/9
File net/net.gyp (right):

http://codereview.chromium.org/1039001/diff/1/9#newcode669
net/net.gyp:669: 'http/http_request_headers_unittest.cc',
It doesn't look like you have uploaded this in the patch !
.. I was about to whine for unit-tests, I should have know better :)

[eroman, 2010-03-16 09:12:48]

I recommend having Brett review the changes to string_util.h

[willchan no longer on Chromium, 2010-03-16 19:54:19]

Brett: please look at the string_util changes.

eroman: I asked for comments on the bug thread about a vector based approach,
and in the absence of input, went ahead with it, for reasons discussed there. 
The observers will want to know the exact order, so unless we want to present
the same data structure you propose to the extension (which will probably want
to serialize it to a string, so it's a bit more javascript work for extension
authors), it will require an extra serialization step from the hash_map to a
vector to present a friendlier interface to extensions.

Also, I'm not sure if we plan to support repeated request headers.  I don't
think the spec disallows them, but if we do that, then a map based data
structure will be more complicated if we are keeping track of insertion orders.

http://codereview.chromium.org/1039001/diff/1/5
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1039001/diff/1/5#newcode9
net/http/http_network_transaction.cc:9: #include "base/string_util.h"
On 2010/03/16 09:11:54, eroman wrote:
> order this lower.

Looks like this whole section needs to be sorted.  I actually included
string_util.h which already existed.  I've sorted it now.

http://codereview.chromium.org/1039001/diff/1/5#newcode63
net/http/http_network_transaction.cc:63:
GetHostAndOptionalPort(request_info->url).c_str());
On 2010/03/16 09:11:54, eroman wrote:
> drop the c_str().

Done.

http://codereview.chromium.org/1039001/diff/1/5#newcode89
net/http/http_network_transaction.cc:89: StringPrintf("%" PRIu64,
upload_data_stream->size()));
On 2010/03/16 09:11:54, eroman wrote:
> Uint64ToString(upload_data_stream->size())

Done.

http://codereview.chromium.org/1039001/diff/1/5#newcode165
net/http/http_network_transaction.cc:165: "CONNECT",
GetHostAndPort(request_info->url).c_str(), "1.1");
On 2010/03/16 09:11:54, eroman wrote:
> drop .c_str().

Done.

http://codereview.chromium.org/1039001/diff/1/5#newcode167
net/http/http_network_transaction.cc:167:
GetHostAndOptionalPort(request_info->url).c_str());
On 2010/03/16 09:11:54, eroman wrote:
> drop .c_str().

Done.

http://codereview.chromium.org/1039001/diff/1/5#newcode173
net/http/http_network_transaction.cc:173: request_info->user_agent.c_str());
On 2010/03/16 09:11:54, eroman wrote:
> drop .c_str().

Done.

http://codereview.chromium.org/1039001/diff/1/5#newcode987
net/http/http_network_transaction.cc:987: HttpRequestHeaders request_headers,
authorization_headers;
On 2010/03/16 09:11:54, eroman wrote:
> can you declare these 1 per line?

Done.

http://codereview.chromium.org/1039001/diff/1/7
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1039001/diff/1/7#newcode38
net/http/http_request_headers.cc:38: void HttpRequestHeaders::SetHeader(const
std::string& key,
On 2010/03/16 09:11:54, eroman wrote:
> So the expected behavior on replacement is that it overwrites in the location
of
> the old header. I wander if it wouldn't make more sense to have it show up at
> the end instead.

Can you give a good reason to do that?  I believe the Firefox ModifyHeaders
extension modifies headers in-place.  This is why I've chosen to do this
in-place.

http://codereview.chromium.org/1039001/diff/1/7#newcode57
net/http/http_request_headers.cc:57: const std::string::size_type key_end_index
= header_line.find(": ");
On 2010/03/16 09:11:54, eroman wrote:
> Shouldn't you be allowed to have LWS around the colon? (we allow it in
response
> headers).

Er, what do you mean by around?  I looked up the RFC and it looks like the spec
allows any amount of LWS between the : and the header value and after the header
value.  I've adjusted the code to only look for the : and to trim the whitespace
surrounding the value.

http://codereview.chromium.org/1039001/diff/1/7#newcode70
net/http/http_request_headers.cc:70: LOG(DFATAL) << "\"" << header_line << "\"
is missing header value.";
On 2010/03/16 09:11:54, eroman wrote:
> Isn't it OK to have a blank value? (We allow it in response headers).

Oops, looks like it is.  I've added this now.

http://codereview.chromium.org/1039001/diff/1/7#newcode96
net/http/http_request_headers.cc:96: "%s %s HTTP/%s\r\n", method_.c_str(),
path_.c_str(), version_.c_str());
On 2010/03/16 09:11:54, eroman wrote:
> indentation

Done.

http://codereview.chromium.org/1039001/diff/1/7#newcode111
net/http/http_request_headers.cc:111: if (LowerCaseEqualsASCII(it->key,
lowercase_key.c_str()))
On 2010/03/16 09:11:54, eroman wrote:
> I wander if you can't use StartsWithASCII() to avoid calling .c_str() here (it
> has a case insensitive bool).

But then I'd have to do a length() equality too.  If you dislike the .c_str(),
perhaps I should just add std::string and friends overloads in string_util.h?

http://codereview.chromium.org/1039001/diff/1/8
File net/http/http_request_headers.h (right):

http://codereview.chromium.org/1039001/diff/1/8#newcode14
net/http/http_request_headers.h:14: class HttpRequestHeaders {
On 2010/03/16 09:11:54, eroman wrote:
> Please add a header documentation, and maybe explain the approach this uses to
> maintain ordering, handling of duplicates, and insertion/lookup cost,
> case-insensitive + case preserving, etc...

Done.

http://codereview.chromium.org/1039001/diff/1/8#newcode37
net/http/http_request_headers.h:37: void AddHeaderFromString(const std::string&
header_line);
On 2010/03/16 09:11:54, eroman wrote:
> Please describe behavior for malformed inputs, and whitespace.

Done.

http://codereview.chromium.org/1039001/diff/1/8#newcode38
net/http/http_request_headers.h:38: void MergeFrom(const HttpRequestHeaders&
other);
On 2010/03/16 09:11:54, eroman wrote:
> The MergeFrom() name wasn't very clear to me. Have you considered something
like
> AddAll / Append ?

I can rename.  I chose is because protocol buffers use this semantic, but I
guess we don't do it much in chromium code.  Well, Append() isn't accurate
because I call SetHeader() which may modify in place.  That's one reason
MergeFrom seems clearer.  If I ditch that semantic, then something like
AppendFrom() would make more sense.

http://codereview.chromium.org/1039001/diff/1/8#newcode40
net/http/http_request_headers.h:40: std::string ToString() const;
On 2010/03/16 09:11:54, eroman wrote:
> I think it is worth mentioning that this includes the trailing \r\n as well.

Done.

http://codereview.chromium.org/1039001/diff/1/8#newcode55
net/http/http_request_headers.h:55: HeaderVector::const_iterator
FindHeader(const std::string& key) const;
On 2010/03/16 09:11:54, eroman wrote:
> why do we need a const version. Where is this used?

It indeed is not used.  I wrote them both at the same time because I needed one
of them.  Just habit from writing libraries where I don't necessarily know all
the consumers.

[eroman, 2010-03-16 23:53:43]

http://codereview.chromium.org/1039001/diff/9001/10006
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1039001/diff/9001/10006#newcode72
net/http/http_request_headers.cc:72: const std::string::size_type value_index =
key_end_index + 2;
The constant of 2 seems dangerous, since you don't enforce a minimum string
length.

I believe you can cause memory errors with the input:

'a:'

Also seems like it will truncate a character for stuff like:

'foobar:baz'

http://codereview.chromium.org/1039001/diff/9001/10006#newcode75
net/http/http_request_headers.cc:75: TrimWhitespaceASCII(header_value, TRIM_ALL,
&header_value);
How about HttpUtil::TrimLWS() ?

stringutil's concept of whitespace is different from HTTP  (where it is just HT
+ SP).

http://codereview.chromium.org/1039001/diff/9001/10008
File net/http/http_request_headers_unittest.cc (right):

http://codereview.chromium.org/1039001/diff/9001/10008#newcode29
net/http/http_request_headers_unittest.cc:29: headers.SetHeader("Foo", "bar");
I think this test would be stronger if the value were different.

http://codereview.chromium.org/1039001/diff/9001/10008#newcode37
net/http/http_request_headers_unittest.cc:37: EXPECT_EQ("Foo: Bar\r\n\r\n",
headers.ToString());
Why wouldn't we want to keep the case from the second one?

http://codereview.chromium.org/1039001/diff/9001/10008#newcode44
net/http/http_request_headers_unittest.cc:44: EXPECT_EQ("\r\n",
headers.ToString());
would be useful to have a case of RemoveHeader where the end result is not empty
string. Also removal of header that doesn't exist, and case insensitivity.

http://codereview.chromium.org/1039001/diff/9001/10008#newcode49
net/http/http_request_headers_unittest.cc:49: headers.AddHeaderFromString("Foo:
bar");
Please add the test:

"a:"

(I think it crashes right now).

[brettw, 2010-03-17 00:21:19]

http://codereview.chromium.org/1039001/diff/9001/10002
File base/string_util.h (right):

http://codereview.chromium.org/1039001/diff/9001/10002#newcode579
base/string_util.h:579: void SplitStringUsingSubstr(const std::wstring& str,
I think we should pick a mode (output arg or return value) and stick with it
rather than having so many different variants. Since the other functions use
output args, I think that makes the most sense here, so I would delete the
"return the vector" versions.

I wouldn't add a wstring variant of this function since you don't need it here,
and since we're trying to kill wstring, we hopefully won't need it in the future
either.

[willchan no longer on Chromium, 2010-03-17 17:03:51]

On 2010/03/17 00:21:19, brettw wrote:
> http://codereview.chromium.org/1039001/diff/9001/10002
> File base/string_util.h (right):
> 
> http://codereview.chromium.org/1039001/diff/9001/10002#newcode579
> base/string_util.h:579: void SplitStringUsingSubstr(const std::wstring& str,
> I think we should pick a mode (output arg or return value) and stick with it
> rather than having so many different variants. Since the other functions use
> output args, I think that makes the most sense here, so I would delete the
> "return the vector" versions.

Do you feel strongly about this?  If so, I'll make the change as requested.  I
just want to push back a little bit, because I personally am fond of "return the
vector", since then I can initialize a const vector (without a copy, due to
NRVO).  That said, the output arg is more versatile since you can reuse
variables that have already been initialized (like member variables), so we
definitely should have that.  I'd like to have both, but will delete the "return
a vector" version if you still would prefer only one variant.

> 
> I wouldn't add a wstring variant of this function since you don't need it
here,
> and since we're trying to kill wstring, we hopefully won't need it in the
future
> either.

Ok, I'll kill the wstring variant.

[willchan no longer on Chromium, 2010-03-17 17:10:21]

http://codereview.chromium.org/1039001/diff/9001/10002
File base/string_util.h (right):

http://codereview.chromium.org/1039001/diff/9001/10002#newcode579
base/string_util.h:579: void SplitStringUsingSubstr(const std::wstring& str,
On 2010/03/17 00:21:19, brettw wrote:
> I think we should pick a mode (output arg or return value) and stick with it
> rather than having so many different variants. Since the other functions use
> output args, I think that makes the most sense here, so I would delete the
> "return the vector" versions.
> 
> I wouldn't add a wstring variant of this function since you don't need it
here,
> and since we're trying to kill wstring, we hopefully won't need it in the
future
> either.

I'm removing the wstring variant right now.  I missed all the string16/wstring
brouhaha from before, so I don't know what the WCHAR_T_IS_UTF16 define is.  Does
that mean that string16 == wstring if that define is set?  So the #if
!defined(WCHAR_T_IS_UTF16) guards should be removed if I don't define a wstring
variant?  I'm going to try that, lemme know if I misunderstood.

[brettw, 2010-03-17 17:17:02]

On 2010/03/17 17:03:51, willchan wrote:
> On 2010/03/17 00:21:19, brettw wrote:
> > http://codereview.chromium.org/1039001/diff/9001/10002
> > File base/string_util.h (right):
> > 
> > http://codereview.chromium.org/1039001/diff/9001/10002#newcode579
> > base/string_util.h:579: void SplitStringUsingSubstr(const std::wstring& str,
> > I think we should pick a mode (output arg or return value) and stick with it
> > rather than having so many different variants. Since the other functions use
> > output args, I think that makes the most sense here, so I would delete the
> > "return the vector" versions.
> 
> Do you feel strongly about this?  If so, I'll make the change as requested.  I
> just want to push back a little bit, because I personally am fond of "return
the
> vector", since then I can initialize a const vector (without a copy, due to
> NRVO).  That said, the output arg is more versatile since you can reuse
> variables that have already been initialized (like member variables), so we
> definitely should have that.  I'd like to have both, but will delete the
"return
> a vector" version if you still would prefer only one variant.

I feel reasonably strongly about this. I think we have too many "string utils"
that makes it harder for people to find what they need. I try to push back on
every addition to this file to make sure we need it or it would be 3x longer
than it is now. I think there is also a strong consistency argument to be made
here. Either they should all support returning the value (which I think would be
too much), or none of them should.

[willchan no longer on Chromium, 2010-03-17 18:33:28]

On 2010/03/17 17:17:02, brettw wrote:
> On 2010/03/17 17:03:51, willchan wrote:
> > On 2010/03/17 00:21:19, brettw wrote:
> > > http://codereview.chromium.org/1039001/diff/9001/10002
> > > File base/string_util.h (right):
> > > 
> > > http://codereview.chromium.org/1039001/diff/9001/10002#newcode579
> > > base/string_util.h:579: void SplitStringUsingSubstr(const std::wstring&
str,
> > > I think we should pick a mode (output arg or return value) and stick with
it
> > > rather than having so many different variants. Since the other functions
use
> > > output args, I think that makes the most sense here, so I would delete the
> > > "return the vector" versions.
> > 
> > Do you feel strongly about this?  If so, I'll make the change as requested. 
I
> > just want to push back a little bit, because I personally am fond of "return
> the
> > vector", since then I can initialize a const vector (without a copy, due to
> > NRVO).  That said, the output arg is more versatile since you can reuse
> > variables that have already been initialized (like member variables), so we
> > definitely should have that.  I'd like to have both, but will delete the
> "return
> > a vector" version if you still would prefer only one variant.
> 
> I feel reasonably strongly about this. I think we have too many "string utils"
> that makes it harder for people to find what they need. I try to push back on
> every addition to this file to make sure we need it or it would be 3x longer
> than it is now. I think there is also a strong consistency argument to be made
> here. Either they should all support returning the value (which I think would
be
> too much), or none of them should.

Ok, I've removed it as requested.

[willchan no longer on Chromium, 2010-03-17 18:33:40]

http://codereview.chromium.org/1039001/diff/9001/10006
File net/http/http_request_headers.cc (right):

http://codereview.chromium.org/1039001/diff/9001/10006#newcode72
net/http/http_request_headers.cc:72: const std::string::size_type value_index =
key_end_index + 2;
On 2010/03/16 23:53:43, eroman wrote:
> The constant of 2 seems dangerous, since you don't enforce a minimum string
> length.
> 
> I believe you can cause memory errors with the input:
> 
> 'a:'
> 
> Also seems like it will truncate a character for stuff like:
> 
> 'foobar:baz'

Agreed.  I missed this when I was trimming LWS.  I think I've fixed it (the new
tests pass).  PTAL.

http://codereview.chromium.org/1039001/diff/9001/10006#newcode75
net/http/http_request_headers.cc:75: TrimWhitespaceASCII(header_value, TRIM_ALL,
&header_value);
On 2010/03/16 23:53:43, eroman wrote:
> How about HttpUtil::TrimLWS() ?
> 
> stringutil's concept of whitespace is different from HTTP  (where it is just
HT
> + SP).

Thanks for pointing this out.  Fixed.

http://codereview.chromium.org/1039001/diff/9001/10008
File net/http/http_request_headers_unittest.cc (right):

http://codereview.chromium.org/1039001/diff/9001/10008#newcode29
net/http/http_request_headers_unittest.cc:29: headers.SetHeader("Foo", "bar");
On 2010/03/16 23:53:43, eroman wrote:
> I think this test would be stronger if the value were different.

The following test covered it.  I just wanted to make sure that if we set the
exact same thing twice, it wasn't getting duplicated or anything.  I've added
more test cases for this stuff as well.

http://codereview.chromium.org/1039001/diff/9001/10008#newcode37
net/http/http_request_headers_unittest.cc:37: EXPECT_EQ("Foo: Bar\r\n\r\n",
headers.ToString());
On 2010/03/16 23:53:43, eroman wrote:
> Why wouldn't we want to keep the case from the second one?

It's all supposed to be the same header key anyway, so I don't know that it
matters.  Also, if we really do a map based approach, then changing the key will
require re-hashing.  Seems unnecessary.  Is there a good reason to update the
header key to the actual casing from the second invocation?

http://codereview.chromium.org/1039001/diff/9001/10008#newcode44
net/http/http_request_headers_unittest.cc:44: EXPECT_EQ("\r\n",
headers.ToString());
On 2010/03/16 23:53:43, eroman wrote:
> would be useful to have a case of RemoveHeader where the end result is not
empty
> string. Also removal of header that doesn't exist, and case insensitivity.

Done.

http://codereview.chromium.org/1039001/diff/9001/10008#newcode49
net/http/http_request_headers_unittest.cc:49: headers.AddHeaderFromString("Foo:
bar");
On 2010/03/16 23:53:43, eroman wrote:
> Please add the test:
> 
> "a:"
> 
> (I think it crashes right now).

Done.

[eroman, 2010-03-18 02:58:29]

http://codereview.chromium.org/1039001/diff/24001/25004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1039001/diff/24001/25004#newcode120
net/http/http_network_transaction.cc:120: std::vector<std::string>
extra_headers_vector;
Please move the code which builds the stripped headers to a separate function,
and add unittests for it.

I don't think your implementation here is correct. You should only be testing
the header name against the blacklist (not the full line). Moreover, the
comparison needs to be case-insensitive.

Having a separate function will also avoid getting a loss of test coverage,
since HttpUtil::StripHeaders() has unittest, which clearly isn't being exercised
by other tests :)

http://codereview.chromium.org/1039001/diff/24001/25004#newcode122
net/http/http_network_transaction.cc:122: request_info->extra_headers, "\r\n",
&extra_headers_vector);
nit: For response headers, we tokenize on "\r" OR "\n"... You can do this using
StringTokenizer(LWS). Probably won't matter since we build most of the request
headers ourselves, but may be good to keep in mind in case XHRs can set a string
with headers...

[willchan no longer on Chromium, 2010-03-20 00:23:34]

http://codereview.chromium.org/1039001/diff/24001/25004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1039001/diff/24001/25004#newcode120
net/http/http_network_transaction.cc:120: std::vector<std::string>
extra_headers_vector;
On 2010/03/18 02:58:29, eroman wrote:
> Please move the code which builds the stripped headers to a separate function,
> and add unittests for it.
> 
> I don't think your implementation here is correct. You should only be testing
> the header name against the blacklist (not the full line). Moreover, the
> comparison needs to be case-insensitive.
> 
> Having a separate function will also avoid getting a loss of test coverage,
> since HttpUtil::StripHeaders() has unittest, which clearly isn't being
exercised
> by other tests :)

I fixed the code and added a test.  I didn't pull it out into a separate
function, since the code isn't invoked anywhere else.  I just added to my
existing set of BuildRequestHeaders tests in HttpNetworkTransactionTest.

http://codereview.chromium.org/1039001/diff/24001/25004#newcode122
net/http/http_network_transaction.cc:122: request_info->extra_headers, "\r\n",
&extra_headers_vector);
On 2010/03/18 02:58:29, eroman wrote:
> nit: For response headers, we tokenize on "\r" OR "\n"... You can do this
using
> StringTokenizer(LWS). Probably won't matter since we build most of the request
> headers ourselves, but may be good to keep in mind in case XHRs can set a
string
> with headers...

Didn't want to bother reading what StringTokenizer did, so I just used
Tokenize().  It's a shame we do this because it weakens the extra_headers check.
 webkit should be constructing with CRLF.  The spec says CRLF is CR LF, not ( CR
| LF ).  I wonder if I should revert back to my old version and just fix
breakages.

[eroman, 2010-03-22 23:29:37]

lgtm