Refactor HttpNetworkTransaction to remove side effects in some member functions.

Refactor HttpNetworkTransaction to remove side effects in some member functions.
I'm preparing to move some of this functionality out to a HttpStream object or something.  I'm hindered here by the mutation of state in functions that seemingly should be const.  I've refactored some code into non-member functions to make the dependencies more explicit.  This will make it easier for me to pull some of this code out.
Also dropped the net:: qualifiers in the unittest.
TESTED=Ran net_unittests

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

[darin (slow to review), 2009-04-25 05:59:53]

Looks good... just a couple minor comments:

http://codereview.chromium.org/100001/diff/2001/3001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/100001/diff/2001/3001#newcode719
Line 719: request_->upload_data->GetContentLength()) {
GetContentLength can be kind of expensive since it may have to stat files to
determine their length.  it isn't clear to me why you are making this change.

http://codereview.chromium.org/100001/diff/2001/3003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/100001/diff/2001/3003#newcode3106
Line 3106: TEST_F(BuildRequestTest, UserAgent) {
nit: we usually try to have only one testsuite per file and have the filename
match the testsuite name.  otherwise, it is easy to lose site of which
testsuites are in which files.

how about this instead:

TEST_F(HttpNetworkTransactionTest, BuildRequestTest_UserAgent)
TEST_F(HttpNetworkTransactionTest, BuildRequestTest_Foo)
TEST_F(HttpNetworkTransactionTest, BuildRequestTest_Bar)
...

[willchan no longer on Chromium, 2009-04-25 07:09:21]

http://codereview.chromium.org/100001/diff/2001/3001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/100001/diff/2001/3001#newcode719
Line 719: request_->upload_data->GetContentLength()) {
On 2009/04/25 05:59:54, darin wrote:
> GetContentLength can be kind of expensive since it may have to stat files to
> determine their length.  it isn't clear to me why you are making this change.

It's because of another change I made above.  request_body_stream_ used to be
set in HttpNetworkTransaction::BuildRequestHeaders(), but now that
BuildRequestHeaders() is a non-member function, it can't be set there (unless I
use an output param).  I was hoping that GetContentLength() was cheap, but since
it's not, I've moved request_body_stream_'s initialization to DoWriteHeaders().

http://codereview.chromium.org/100001/diff/2001/3003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/100001/diff/2001/3003#newcode3106
Line 3106: TEST_F(BuildRequestTest, UserAgent) {
On 2009/04/25 05:59:54, darin wrote:
> nit: we usually try to have only one testsuite per file and have the filename
> match the testsuite name.  otherwise, it is easy to lose site of which
> testsuites are in which files.
> 
> how about this instead:
> 
> TEST_F(HttpNetworkTransactionTest, BuildRequestTest_UserAgent)
> TEST_F(HttpNetworkTransactionTest, BuildRequestTest_Foo)
> TEST_F(HttpNetworkTransactionTest, BuildRequestTest_Bar)
> ...
> 

Done.

[wtc, 2009-04-27 23:56:20]

LGTM.

You may want to mention the removal of the net:: qualifiers
in the CL's description, because that's the bulk of the CL
by line count.

Some nits below.  I am a little worried about the extra
string copying resulting from this refactoring.  Hopefully
the strings involved are short, and the compiler can optimize
away some of the string copying.

I'm also not sure if some of the refactoring is necessary.
I trust that you know what you're doing.

Thank you for adding unit tests for the BuildRequestHeaders
function.

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

http://codereview.chromium.org/100001/diff/5/1005#newcode33
Line 33: namespace {
Nit: add a blank line above "namespace {".

http://codereview.chromium.org/100001/diff/5/1005#newcode672
Line 672: (HaveAuth(HttpAuth::AUTH_PROXY) ||
Nit: should we create a small function for
  HaveAuth(target) || SelectPreemptiveAuth(target)
?

http://codereview.chromium.org/100001/diff/5/1005#newcode689
Line 689: request_headers_ = BuildTunnelRequest(request_,
authorization_headers);
Just checking -- the compiler can optimize away the copying
of the function's return value to request_headers_, right?
If not, we can use a std:string* output parameter.

http://codereview.chromium.org/100001/diff/5/1005#newcode1297
Line 1297: if (ShouldResendRequest()) {
Nit: I don't see the need for splitting ShouldResendRequest
into two functions if these two functions are always called
together.

http://codereview.chromium.org/100001/diff/5/1005#newcode1412
Line 1412: return HttpAuth::GetAuthorizationHeaderName(target) +
Nit: does this cause more string copying?  Just curious.

http://codereview.chromium.org/100001/diff/5/1006
File net/http/http_network_transaction.h (left):

http://codereview.chromium.org/100001/diff/5/1006#oldcode58
Line 58: void BuildRequestHeaders();
I don't understand why you need to make these two methods
non-member methods.

http://codereview.chromium.org/100001/diff/5/1006
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/100001/diff/5/1006#newcode126
Line 126: void ResetConnectionAndRequestForResend();
Nit: how about renaming this method ResetStateForResend?
It also sets next_state_, so it's cumbersome to enumerate
everything it sets in its name.  Also, we already have a
method named ResetStateForRestart.

http://codereview.chromium.org/100001/diff/5/1006#newcode160
Line 160: bool ShouldApplyProxyAuth() const;
Nit: these two ShouldApplyXXXAuth methods are very simple.
Should they be inline?

http://codereview.chromium.org/100001/diff/5/1007
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/100001/diff/5/1007#newcode3299
Line 3299: TEST_F(HttpNetworkTransactionTest,
BuildRequest_ControlControlNoCache) {
Just curious: what does "ControlControl" mean?  Did you
intend to say "CacheControl"?

http://codereview.chromium.org/100001/diff/5/1007#newcode3341
Line 3341: BuildRequest_ControlControlValidateCache) {
Same here: did you mean to say "CacheControl"?

[willchan no longer on Chromium, 2009-04-28 05:42:26]

We chatted about this offline and I updated wtc on my plan wrt HttpStream.  I've
addressed all the comments.  I also used some StringAppendF() calls to get rid
of extra string temporaries and reduce code size.

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

http://codereview.chromium.org/100001/diff/5/1005#newcode33
Line 33: namespace {
On 2009/04/27 23:56:20, wtc wrote:
> Nit: add a blank line above "namespace {".

Done.

http://codereview.chromium.org/100001/diff/5/1005#newcode672
Line 672: (HaveAuth(HttpAuth::AUTH_PROXY) ||
On 2009/04/27 23:56:20, wtc wrote:
> Nit: should we create a small function for
>   HaveAuth(target) || SelectPreemptiveAuth(target)
> ?

I'm not sure if it'd save us any lines, and I'm not sure if it improves
readability.  HaveAuth() is const and SelectPreemptiveAuth() has side effects,
so I don't know how to name it clearly so people know what it does.  Using
SelectPreemptiveAuth() makes it clearer that something is changing.

http://codereview.chromium.org/100001/diff/5/1005#newcode689
Line 689: request_headers_ = BuildTunnelRequest(request_,
authorization_headers);
On 2009/04/27 23:56:20, wtc wrote:
> Just checking -- the compiler can optimize away the copying
> of the function's return value to request_headers_, right?
> If not, we can use a std:string* output parameter.

thanks for pointing this out.  i was intending to use nrvo, but it only works
for local variables.  i've added an output parameter.

http://codereview.chromium.org/100001/diff/5/1005#newcode1297
Line 1297: if (ShouldResendRequest()) {
On 2009/04/27 23:56:20, wtc wrote:
> Nit: I don't see the need for splitting ShouldResendRequest
> into two functions if these two functions are always called
> together.

As we discussed offline, it was so that ShouldSendRequest() can be const.  It
confused me when reading it that something was mutating under the hood.

http://codereview.chromium.org/100001/diff/5/1005#newcode1412
Line 1412: return HttpAuth::GetAuthorizationHeaderName(target) +
On 2009/04/27 23:56:20, wtc wrote:
> Nit: does this cause more string copying?  Just curious.

Yep, one extra copy.  As discussed earlier, this is fairly small so it shouldn't
be a big deal.

http://codereview.chromium.org/100001/diff/5/1007
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/100001/diff/5/1007#newcode3299
Line 3299: TEST_F(HttpNetworkTransactionTest,
BuildRequest_ControlControlNoCache) {
On 2009/04/27 23:56:20, wtc wrote:
> Just curious: what does "ControlControl" mean?  Did you
> intend to say "CacheControl"?

Woops, yeah I meant CacheControl.  Thanks!

http://codereview.chromium.org/100001/diff/5/1007#newcode3341
Line 3341: BuildRequest_ControlControlValidateCache) {
On 2009/04/27 23:56:20, wtc wrote:
> Same here: did you mean to say "CacheControl"?

Yep, thanks for pointing this out.

[wtc, 2009-04-28 17:48:34]

LGTM.  Please note my questions about the use of 'swap'
below, and please remove the HaveTargetAuth method from
the .h file.

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

http://codereview.chromium.org/100001/diff/5/1005#newcode689
Line 689: request_headers_ = BuildTunnelRequest(request_,
authorization_headers);
On 2009/04/28 05:42:26, willchan wrote:
>
> thanks for pointing this out.  i was intending to use nrvo, but it only works
> for local variables.  i've added an output parameter.

I guess you meant the return value must be assigned to
a local variable (as opposed to a class member such as
request_headers_ here) for nrvo to work?  That's too bad.

http://codereview.chromium.org/100001/diff/8/1014#newcode97
Line 97: std::string BuildTunnelRequest(const HttpRequestInfo* request_info,
Nit: This function doesn't have any early returns, so
I don't understand why we need to use a swap.

http://codereview.chromium.org/100001/diff/8/1014#newcode124
Line 124: ClientSocketFactory* csf)
Same question about the use of swap.  Why don't we just name
the output parameter 'request_headers'?

http://codereview.chromium.org/100001/diff/8/1015
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/100001/diff/8/1015#newcode212
Line 212: bool HaveTargetAuth(HttpAuth::Target target) const;
I didn't find the definition of this method in the
.cc file.  Please remove this declaration.

[willchan no longer on Chromium, 2009-04-28 17:59:16]

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

http://codereview.chromium.org/100001/diff/5/1005#newcode689
Line 689: request_headers_ = BuildTunnelRequest(request_,
authorization_headers);
On 2009/04/28 17:48:34, wtc wrote:
> On 2009/04/28 05:42:26, willchan wrote:
> >
> > thanks for pointing this out.  i was intending to use nrvo, but it only
works
> > for local variables.  i've added an output parameter.
> 
> I guess you meant the return value must be assigned to
> a local variable (as opposed to a class member such as
> request_headers_ here) for nrvo to work?  That's too bad.

Correct.

http://codereview.chromium.org/100001/diff/8/1014#newcode97
Line 97: std::string BuildTunnelRequest(const HttpRequestInfo* request_info,
On 2009/04/28 17:48:34, wtc wrote:
> Nit: This function doesn't have any early returns, so
> I don't understand why we need to use a swap.

It wasn't necessary.  It just (1) took fewer code changes and (2) didn't require
changing everything to *request_headers or request_headers.append(), neither of
which look as "pretty" to me.  I don't care much though, so I've gone ahead
renamed out to request_headers and got rid of the local variable.

http://codereview.chromium.org/100001/diff/8/1014#newcode124
Line 124: ClientSocketFactory* csf)
On 2009/04/28 17:48:34, wtc wrote:
> Same question about the use of swap.  Why don't we just name
> the output parameter 'request_headers'?

Explained above, but changed as requested.

http://codereview.chromium.org/100001/diff/8/1015
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/100001/diff/8/1015#newcode212
Line 212: bool HaveTargetAuth(HttpAuth::Target target) const;
On 2009/04/28 17:48:34, wtc wrote:
> I didn't find the definition of this method in the
> .cc file.  Please remove this declaration.

Done.

[wtc, 2009-04-30 18:57:20]

http://codereview.chromium.org/100001/diff/1019/15
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/100001/diff/1019/15#newcode70
Line 70: StringAppendF(request_headers, "Content-Length: %llu\r\n",
I just learned that we're supposed to use the PRIu64 macro
instead of llu to print a uint64_t variable:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

[willchan no longer on Chromium, 2009-04-30 19:32:39]

http://codereview.chromium.org/100001/diff/1019/15
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/100001/diff/1019/15#newcode70
Line 70: StringAppendF(request_headers, "Content-Length: %llu\r\n",
On 2009/04/30 18:57:20, wtc wrote:
> I just learned that we're supposed to use the PRIu64 macro
> instead of llu to print a uint64_t variable:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit...

That's for uint64_t.  We're using uint64 here.  uint64!=uint64_t.