Perform HTTP authentication over a keep-alive connection....

Perform HTTP authentication over a keep-alive connection.
This is required for NTLM authentication.

R=eroman
BUG=6567, 6824
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9964

[wtc, 2009-02-17 22:51:26]

eroman: please review the CL.
darin: just FYI.

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

http://codereview.chromium.org/21433/diff/1/2#newcode977
Line 977: int rv = HandleAuthChallenge();
I have to move this block of code towards the end of this
method so that we have set the content_length_ member.
The new DoDrainBodyForAuthRestartComplete method uses the
content_length_ member.

[eroman, 2009-02-17 23:10:47]

will review this as soon as I get back.
one quick comment as I was scanning:

http://codereview.chromium.org/21433/diff/1007/6
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/21433/diff/1007/6#newcode855
Line 855: establishing_tunnel_)
So I believe this is going to cause problems when using HTTPS on proxy server
that requires NTLM-auth.

The |establishing_tunnel_| provision will prevent the socket from being
recycled...

[darin (slow to review), 2009-02-18 00:17:18]

so long as we can still do basic/digest auth over non-keep-alive connections,
all should be well.  the duplication of code is unfortunate... i fear
maintenance headaches :)

[wtc, 2009-02-18 01:32:14]

Darin: Note that this CL will cause us to also do
basic/digest auth over a keep-alive connection.  It's not
limited to the NTLM auth scheme.

Re: code duplication: I promise I will eliminate it later.
There are two options: 1. Create a new method for the common
code.  2. Add a new boolean member 'draining_body_'
(similar to the establishing_tunnel_ member) and reuse the
existing DoReadBody and DoReadBodyComplete methods.

Eric: you're right about establishing_tunnel_.  I removed
establishing_tunnel_ from that test and added a unit test
for that scenario.  Please review the new Patch Set.
Thanks.

[eroman, 2009-02-18 02:36:04]

LGTM

(reviewed patchset 4).

Thanks for all the tests.

I wander if we will want to split up http_transaction_unittest.cc at some point,
it is getting quite large.

[wtc, 2009-02-18 02:56:45]

I found that I missed another corner case: a response
content length of 0.  I fixed that and added a unit test.

Please review Patch Set 5.

[eroman, 2009-02-18 03:34:02]

I am not seeing what changed between revision 4 and 5 other than refactoring.

This is the diff I looked at:

http://codereview.chromium.org/21433/diff2/1013:25/26

http://codereview.chromium.org/21433/diff/25/27
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/21433/diff/25/27#newcode132
Line 132: // Resets connection_ unless |keep_alive| is true, then calls
|connection_|

http://codereview.chromium.org/21433/diff/25/27#newcode133
Line 133: // ResetStateForRestart.  Sets next_state_ appropriately.
|next_state_|

[eroman, 2009-02-18 20:14:11]

lgtm