Integrate the SpdyProxyClientSocket into the HttpStreamRequest...

Integrate the SpdyProxyClientSocket into the HttpStreamRequest
to support fetching HTTPS URLS over a SPDY Proxy.

BUG=29625
TEST=HttpNetworkTransactionTest.HttpsProxySpdyConnect


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

[Ryan Hamilton, 2010-10-06 03:20:29]

Hi Guys,

Here's a first pass at hooking the SpdyProxyClientSocket into the normal request
flow (HttpStreamRequest).  wtc, as part of this, I "fixed" the behavior of
SpdyProxyClientSocket's Read() method.  Can you take a look at that?  (I've
added new unit tests, so I feel reasonable confident).  vandebo, would you mind
reviewing the rest?

Thanks,

Ryan

[wtc, 2010-10-06 20:53:36]

http://codereview.chromium.org/3417010/diff/53002/59015
File net/spdy/spdy_proxy_client_socket.cc (left):

http://codereview.chromium.org/3417010/diff/53002/59015#oldcode94
net/spdy/spdy_proxy_client_socket.cc:94: return next_state_ == STATE_DONE &&
spdy_stream_ != NULL &&
Can you explain this change?

http://codereview.chromium.org/3417010/diff/53002/59015
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/53002/59015#newcode427
net/spdy/spdy_proxy_client_socket.cc:427: LOG(INFO) << "Closing!";
Please delete this LOG(INFO) statement.  If it is useful,
use VLOG(1) instead.

http://codereview.chromium.org/3417010/diff/53002/59015#newcode438
net/spdy/spdy_proxy_client_socket.cc:438: read_callback_ = NULL;
Should we also leave read_callback_ (and perhaps user_buffer_)
unchanged?  (I am not familiar with this code.)

[Ryan Hamilton, 2010-10-06 21:24:38]

http://codereview.chromium.org/3417010/diff/53002/59015
File net/spdy/spdy_proxy_client_socket.cc (left):

http://codereview.chromium.org/3417010/diff/53002/59015#oldcode94
net/spdy/spdy_proxy_client_socket.cc:94: return next_state_ == STATE_DONE &&
spdy_stream_ != NULL &&
On 2010/10/06 20:53:36, wtc wrote:
> Can you explain this change?

I hope so :> 

As I understand it, there are 2 pairs of states Connected/Disconnected and
Open/Closed.  If a socket is Connected, it may be Closed, depending on the
actions of the peer.

By this logic, we should return true from IsConnected if we have successfully
connected to the peer and have not been Disconnected.  When the
SpdyProxyClientSocket finishes the connection it enters STATE_DONE, and does not
leave STATE_DONE until Disconnect is called.

The old code was more strict.  It was returning true only when the socket was
"open" (is open the opposite of Closed?).

I believe the new semantics are correct.

http://codereview.chromium.org/3417010/diff/53002/59015
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/53002/59015#newcode427
net/spdy/spdy_proxy_client_socket.cc:427: LOG(INFO) << "Closing!";
On 2010/10/06 20:53:36, wtc wrote:
> Please delete this LOG(INFO) statement.  If it is useful,
> use VLOG(1) instead.

Done.

http://codereview.chromium.org/3417010/diff/53002/59015#newcode438
net/spdy/spdy_proxy_client_socket.cc:438: read_callback_ = NULL;
On 2010/10/06 20:53:36, wtc wrote:
> Should we also leave read_callback_ (and perhaps user_buffer_)
> unchanged?  (I am not familiar with this code.)

Oh, you're right.  When I change the semantics of Close vs Disconnect, I
verified that a read following a close would succeed, but I did not add a test
for a read *pending* when on close.  I've add a test for this case, and made the
change you suggests

[vandebo (ex-Chrome), 2010-10-07 21:05:52]

http://codereview.chromium.org/3417010/diff/53002/59010
File net/http/http_proxy_client_socket_pool.cc (left):

http://codereview.chromium.org/3417010/diff/53002/59010#oldcode21
net/http/http_proxy_client_socket_pool.cc:21: #include
"net/socket/tcp_client_socket_pool.h"
Why did you get rid of this include?

http://codereview.chromium.org/3417010/diff/53002/59010
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/53002/59010#newcode204
net/http/http_proxy_client_socket_pool.cc:204: // TODO(rch): If we ever decide
to implement a "trusted" SPDY proxy
How likely is it that we will do this?  If fairly unlikely, the comment probably
isn't worthwhile - it'll get figured out when it's done.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode211
net/http/http_proxy_client_socket_pool.cc:211: next_state_ =
STATE_SPDY_PROXY_CREATE_SESSION;
won't we always have ssl_params and not tcp_params for the spdy proxy case? i.e.
we'd never hit this code?

http://codereview.chromium.org/3417010/diff/53002/59010#newcode218
net/http/http_proxy_client_socket_pool.cc:218: next_state_ =
STATE_SSL_CONNECT_COMPLETE;
Shouldn't you call spdy_pool->Get() here to see if you already have a session
and can skip directly to CreateStream?

http://codereview.chromium.org/3417010/diff/53002/59010#newcode270
net/http/http_proxy_client_socket_pool.cc:270: return
spdy_pool->GetSpdySessionFromSocket(
This can't return ERR_IO_PENDING so what's the point in making it it's own
state?  SpdyProxyCreateSession/Complete and SpdyProxyCreateStream could all be
in one state.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode293
net/http/http_proxy_client_socket_pool.cc:293: next_state_ =
STATE_SPDY_PROXY_CONNECT;
This state is effectively empty - remove it.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode309
net/http/http_proxy_client_socket_pool.cc:309: // Clear the circular reference
to HttpNetworkSession (|params_| reference
params_ no longer references HttpNetworkSession. It should be safe to remove
this comment and code from here as well as from DoHttpProxyConnect

http://codereview.chromium.org/3417010/diff/56002/44003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1832
net/http/http_network_transaction_unittest.cc:1832: spdy::SpdyFrame*
ConstructConnectRequestFrame() {
Did this get copied from somewhere? Should it go into a utility file?

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1849
net/http/http_network_transaction_unittest.cc:1849: //    "user-agent",
"Chromium Ultra Awesome X Edition",
Remove cruft.

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1857
net/http/http_network_transaction_unittest.cc:1857: // Test a SPDY CONNECT
through an HTTPS Proxy to an HTTP Server.
What about a SPDY CONNECT through an HTTPS Proxy to an *HTTPS* server?

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1936
net/http/http_network_transaction_unittest.cc:1936: spdy::SpdyFrame*
ConstructWrappedSpdyFrame(
Util file, together with other utility methods?

http://codereview.chromium.org/3417010/diff/56002/44006
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44006#newcode5
net/http/http_proxy_client_socket_pool_unittest.cc:5: #include
"net/http/http_proxy_client_socket_pool.h"
Should you add more tests here?

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (left):

http://codereview.chromium.org/3417010/diff/56002/44009#oldcode431
net/spdy/spdy_proxy_client_socket.cc:431: user_buffer_ = NULL;
I think you can still clear the user_buffer_ (after OnDataReceived).

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44009#newcode433
net/spdy/spdy_proxy_client_socket.cc:433: write_callback_ = NULL;
Have write callbacks been cancelled at this point?

[Ryan Hamilton, 2010-10-07 23:38:00]

http://codereview.chromium.org/3417010/diff/53002/59010
File net/http/http_proxy_client_socket_pool.cc (left):

http://codereview.chromium.org/3417010/diff/53002/59010#oldcode21
net/http/http_proxy_client_socket_pool.cc:21: #include
"net/socket/tcp_client_socket_pool.h"
On 2010/10/07 21:05:52, vandebo wrote:
> Why did you get rid of this include?

Whoops.  I think that must have been accidental.

http://codereview.chromium.org/3417010/diff/53002/59010
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/53002/59010#newcode204
net/http/http_proxy_client_socket_pool.cc:204: // TODO(rch): If we ever decide
to implement a "trusted" SPDY proxy
On 2010/10/07 21:05:52, vandebo wrote:
> How likely is it that we will do this?  If fairly unlikely, the comment
probably
> isn't worthwhile - it'll get figured out when it's done.

Well, mbelshe keeps talking about this feature, so I thought it was likely to
happen.  I'm happy to remove it if you would prefer.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode211
net/http/http_proxy_client_socket_pool.cc:211: next_state_ =
STATE_SPDY_PROXY_CREATE_SESSION;
On 2010/10/07 21:05:52, vandebo wrote:
> won't we always have ssl_params and not tcp_params for the spdy proxy case?
i.e.
> we'd never hit this code?

Interesting.  You're right that we'll never hit this path.  It looks like we're
not paying attention to force_spdy_always here so there's no way for using_spdy_
to become true.  I've removed the code.  Nice catch.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode218
net/http/http_proxy_client_socket_pool.cc:218: next_state_ =
STATE_SSL_CONNECT_COMPLETE;
On 2010/10/07 21:05:52, vandebo wrote:
> Shouldn't you call spdy_pool->Get() here to see if you already have a session
> and can skip directly to CreateStream?

Done.  I just noticed this myself.  I think we don't want to call Get() (since
that will create one if it does not already exist), I think we want to call
HasSession().

http://codereview.chromium.org/3417010/diff/53002/59010#newcode270
net/http/http_proxy_client_socket_pool.cc:270: return
spdy_pool->GetSpdySessionFromSocket(
On 2010/10/07 21:05:52, vandebo wrote:
> This can't return ERR_IO_PENDING so what's the point in making it it's own
> state?  SpdyProxyCreateSession/Complete and SpdyProxyCreateStream could all be
> in one state.

Done.  I guess since it does not take a callback it can't very well return
ERR_IO_PENDING :>

http://codereview.chromium.org/3417010/diff/53002/59010#newcode293
net/http/http_proxy_client_socket_pool.cc:293: next_state_ =
STATE_SPDY_PROXY_CONNECT;
On 2010/10/07 21:05:52, vandebo wrote:
> This state is effectively empty - remove it.

Done.

http://codereview.chromium.org/3417010/diff/53002/59010#newcode309
net/http/http_proxy_client_socket_pool.cc:309: // Clear the circular reference
to HttpNetworkSession (|params_| reference
On 2010/10/07 21:05:52, vandebo wrote:
> params_ no longer references HttpNetworkSession. It should be safe to remove
> this comment and code from here as well as from DoHttpProxyConnect

Done.

http://codereview.chromium.org/3417010/diff/56002/44003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1832
net/http/http_network_transaction_unittest.cc:1832: spdy::SpdyFrame*
ConstructConnectRequestFrame() {
On 2010/10/07 21:05:52, vandebo wrote:
> Did this get copied from somewhere? Should it go into a utility file?

Moved to spdy_test_util.cc.

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1849
net/http/http_network_transaction_unittest.cc:1849: //    "user-agent",
"Chromium Ultra Awesome X Edition",
On 2010/10/07 21:05:52, vandebo wrote:
> Remove cruft.

Done.

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1857
net/http/http_network_transaction_unittest.cc:1857: // Test a SPDY CONNECT
through an HTTPS Proxy to an HTTP Server.
On 2010/10/07 21:05:52, vandebo wrote:
> What about a SPDY CONNECT through an HTTPS Proxy to an *HTTPS* server?

Hm.. so that's exactly what this is testing.  I said "HTTP Server" to contrast
with "SPDY Server", but I've changed it to "HTTPS (non-SPDY) Server".

http://codereview.chromium.org/3417010/diff/56002/44003#newcode1936
net/http/http_network_transaction_unittest.cc:1936: spdy::SpdyFrame*
ConstructWrappedSpdyFrame(
On 2010/10/07 21:05:52, vandebo wrote:
> Util file, together with other utility methods?

Done.

http://codereview.chromium.org/3417010/diff/56002/44006
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44006#newcode5
net/http/http_proxy_client_socket_pool_unittest.cc:5: #include
"net/http/http_proxy_client_socket_pool.h"
On 2010/10/07 21:05:52, vandebo wrote:
> Should you add more tests here?

Good idea.  I added my tests to the HttpNetworkTransactionTest, but you're
right, we should have tests here too.  I'll work on it.

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (left):

http://codereview.chromium.org/3417010/diff/56002/44009#oldcode431
net/spdy/spdy_proxy_client_socket.cc:431: user_buffer_ = NULL;
On 2010/10/07 21:05:52, vandebo wrote:
> I think you can still clear the user_buffer_ (after OnDataReceived).

OnDataReceived will take care of clearing it for us (and read_callback_).  Is
that sufficient?

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44009#newcode433
net/spdy/spdy_proxy_client_socket.cc:433: write_callback_ = NULL;
On 2010/10/07 21:05:52, vandebo wrote:
> Have write callbacks been cancelled at this point?

Trying to parse out what you're asking here.  I think you're asking if it's
possible for SpdyStream or SpdyStream to call back to us (since we are a
SpdyStream::Delegate) after OnClose is called.  I believe that SpdySession calls
SpdyStream::OnClose which clears the delegate member field before calling the
delegate's OnClose method.

So said differently, I believe that when SpdyStream::Delegate::OnClose is
called, is it not possible for the delegate to receive and more call backs.

Do you agree?

[vandebo (ex-Chrome), 2010-10-08 19:08:41]

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44009#newcode433
net/spdy/spdy_proxy_client_socket.cc:433: write_callback_ = NULL;
On 2010/10/07 23:38:01, Ryan Hamilton wrote:
> On 2010/10/07 21:05:52, vandebo wrote:
> > Have write callbacks been cancelled at this point?
> 
> Trying to parse out what you're asking here.  I think you're asking if it's
> possible for SpdyStream or SpdyStream to call back to us (since we are a
> SpdyStream::Delegate) after OnClose is called.  I believe that SpdySession
calls
> SpdyStream::OnClose which clears the delegate member field before calling the
> delegate's OnClose method.
> 
> So said differently, I believe that when SpdyStream::Delegate::OnClose is
> called, is it not possible for the delegate to receive and more call backs.

Thinking about this more, I think you need to do something similar to how you
cleanup the read callback.  If write_callback_ != NULL, that means there's an
HttpNetworkTransaction waiting for the write to complete.  If you just NULL it
out, the HttpNetworkTransaction will just hang there waiting for the write to
complete.

http://codereview.chromium.org/3417010/diff/107001/37003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37003#newcode1832
net/http/http_network_transaction_unittest.cc:1832:
TEST_F(HttpNetworkTransactionTest, HttpsProxySpdyConnectHttp) {
Should update the name to reflect the Https endpoint.

http://codereview.chromium.org/3417010/diff/107001/37003#newcode2042
net/http/http_network_transaction_unittest.cc:2042: 
Does this change enable connecting Spdy proxy to plain Http host ?  Do we need a
test for that case?

http://codereview.chromium.org/3417010/diff/107001/37004
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37004#newcode193
net/http/http_proxy_client_socket_pool.cc:193: HostPortProxyPair
pair(params_->destination().host_port_pair(),
Is it worth storing this value, so you only need to create it once? (I'm not
sure)

http://codereview.chromium.org/3417010/diff/107001/37004#newcode255
net/http/http_proxy_client_socket_pool.cc:255:
transport_socket_handle_.get()->Reset();
I'm glad you considered this race.  Should we disconnect the socket here though?
 It is to the proxy, and the proxy evidently supports spdy so we should never
request another ssl connection to the proxy.  Eventually it'll timeout anyway,
right?

http://codereview.chromium.org/3417010/diff/107001/37007
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37007#newcode465
net/http/http_stream_request.cc:465: ProxyServer::Direct());
This makes me realize that you probably should have another reviewer for the
spdy part of this (and other) changes, as I don't know the spdy code.

[Ryan Hamilton, 2010-10-08 23:08:43]

http://codereview.chromium.org/3417010/diff/56002/44009
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/56002/44009#newcode433
net/spdy/spdy_proxy_client_socket.cc:433: write_callback_ = NULL;
On 2010/10/08 19:08:41, vandebo wrote:
> On 2010/10/07 23:38:01, Ryan Hamilton wrote:
> > On 2010/10/07 21:05:52, vandebo wrote:
> > > Have write callbacks been cancelled at this point?
> > 
> > Trying to parse out what you're asking here.  I think you're asking if it's
> > possible for SpdyStream or SpdyStream to call back to us (since we are a
> > SpdyStream::Delegate) after OnClose is called.  I believe that SpdySession
> calls
> > SpdyStream::OnClose which clears the delegate member field before calling
the
> > delegate's OnClose method.
> > 
> > So said differently, I believe that when SpdyStream::Delegate::OnClose is
> > called, is it not possible for the delegate to receive and more call backs.
> 
> Thinking about this more, I think you need to do something similar to how you
> cleanup the read callback.  If write_callback_ != NULL, that means there's an
> HttpNetworkTransaction waiting for the write to complete.  If you just NULL it
> out, the HttpNetworkTransaction will just hang there waiting for the write to
> complete.
> 
> 

It is unclear.  Wan-Teh thinks that the write callback should not be invoked in
this situation, but he's still digging.

http://codereview.chromium.org/3417010/diff/107001/37003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37003#newcode1832
net/http/http_network_transaction_unittest.cc:1832:
TEST_F(HttpNetworkTransactionTest, HttpsProxySpdyConnectHttp) {
On 2010/10/08 19:08:41, vandebo wrote:
> Should update the name to reflect the Https endpoint.

Done.

http://codereview.chromium.org/3417010/diff/107001/37003#newcode2042
net/http/http_network_transaction_unittest.cc:2042: 
On 2010/10/08 19:08:41, vandebo wrote:
> Does this change enable connecting Spdy proxy to plain Http host ?  Do we need
a
> test for that case?

No, this change merely enables SPDY to the proxy, and then HTTPS to the host. 
If we're not doing HTTPS to the host, the *we* don't talk to the host at all, we
only talk to the proxy.  This case is handled in
HttpNetworkTransactionTest.HttpsProxySpdyGet.

http://codereview.chromium.org/3417010/diff/107001/37004
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37004#newcode193
net/http/http_proxy_client_socket_pool.cc:193: HostPortProxyPair
pair(params_->destination().host_port_pair(),
On 2010/10/08 19:08:41, vandebo wrote:
> Is it worth storing this value, so you only need to create it once? (I'm not
> sure)

My sense is that these are pretty tiny objects that are easy to create when
needed.  We only use it in one other place in the code, so I would be inclined
to leave it alone, but I'll change it if you prefer.  C++ performance is *not*
my strong suit :>

http://codereview.chromium.org/3417010/diff/107001/37004#newcode255
net/http/http_proxy_client_socket_pool.cc:255:
transport_socket_handle_.get()->Reset();
On 2010/10/08 19:08:41, vandebo wrote:
> I'm glad you considered this race.  Should we disconnect the socket here
though?
>  It is to the proxy, and the proxy evidently supports spdy so we should never
> request another ssl connection to the proxy.  Eventually it'll timeout anyway,
> right?

Done.

http://codereview.chromium.org/3417010/diff/107001/37007
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37007#newcode465
net/http/http_stream_request.cc:465: ProxyServer::Direct());
On 2010/10/08 19:08:41, vandebo wrote:
> This makes me realize that you probably should have another reviewer for the
> spdy part of this (and other) changes, as I don't know the spdy code.

Sounds good.

[Ryan Hamilton, 2010-10-12 18:34:05]

Addressing a number of issues that mbelshe raised in a verbal code review.

Also adding eroman to this CL as I'm (hopefully :>) fixing one of his TODOs in
http_proxy_client_socket.cc

http://codereview.chromium.org/3417010/diff/136001/137002
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/136001/137002#newcode216
net/http/http_proxy_client_socket_pool.cc:216: else
Added a new ERR_PROXY_CERTIFICATE_INVALID error for this condition.

http://codereview.chromium.org/3417010/diff/136001/137002#newcode223
net/http/http_proxy_client_socket_pool.cc:223:
transport_socket_handle_->socket()->Disconnect();
ping eroman

http://codereview.chromium.org/3417010/diff/136001/137002#newcode239
net/http/http_proxy_client_socket_pool.cc:239: // need to add a predicated to
this if statement so we fall through
"predicate"

http://codereview.chromium.org/3417010/diff/136001/137002#newcode248
net/http/http_proxy_client_socket_pool.cc:248: 
extra line

http://codereview.chromium.org/3417010/diff/136001/137002#newcode275
net/http/http_proxy_client_socket_pool.cc:275:
params_->destination().priority(),
HIGHEST

http://codereview.chromium.org/3417010/diff/136001/137004
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/136001/137004#newcode212
net/http/http_proxy_client_socket_pool_unittest.cc:212: MockWrite(true, 0,
"CONNECT www.google.com:443 HTTP/1.1\r\n"
Simplified flow by passing both spdy and non-spdy Mocks to Initialize which
conditionally uses the correct ones.

http://codereview.chromium.org/3417010/diff/136001/137004#newcode298
net/http/http_proxy_client_socket_pool_unittest.cc:298: MockWrite("CONNECT
www.google.com:443 HTTP/1.1\r\n"
indent is incorrect on all MockRead/MockWrite lines

http://codereview.chromium.org/3417010/diff/136001/137004#newcode379
net/http/http_proxy_client_socket_pool_unittest.cc:379: BoundNetLog());
indent

http://codereview.chromium.org/3417010/diff/136001/137004#newcode416
net/http/http_proxy_client_socket_pool_unittest.cc:416: }
blank line

http://codereview.chromium.org/3417010/diff/136001/137005
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/136001/137005#newcode463
net/http/http_stream_request.cc:463: if (proxy_info()->is_https() &&
!request_info().url.SchemeIs("https")) {
Factor this into a helper method: isHttpsProxyAndHttpUrl.

http://codereview.chromium.org/3417010/diff/136001/137005#newcode757
net/http/http_stream_request.cc:757: 
save results of the following boolean expression to a local var named

useRelativeUrl

http://codereview.chromium.org/3417010/diff/136001/137007
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/136001/137007#newcode443
net/spdy/spdy_proxy_client_socket.cc:443: }
Per mbelshe: Call back the write callback with ERR_CONNECTION_CLOSED.

http://codereview.chromium.org/3417010/diff/136001/137008
File net/spdy/spdy_proxy_client_socket_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/136001/137008#newcode1021
net/spdy/spdy_proxy_client_socket_unittest.cc:1021: ASSERT_EQ(0,
sock_->Read(NULL, 1, NULL));
Add comment that we are testing ERR_CONNECTION_CLOSED N times, explicitly

http://codereview.chromium.org/3417010/diff/136001/137008#newcode1073
net/spdy/spdy_proxy_client_socket_unittest.cc:1073: // ----------- Pending
read/write when Disconnected
remove

[vandebo (ex-Chrome), 2010-10-12 22:19:08]

http://codereview.chromium.org/3417010/diff/107001/37004
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/107001/37004#newcode193
net/http/http_proxy_client_socket_pool.cc:193: HostPortProxyPair
pair(params_->destination().host_port_pair(),
On 2010/10/08 23:08:43, Ryan Hamilton wrote:
> On 2010/10/08 19:08:41, vandebo wrote:
> > Is it worth storing this value, so you only need to create it once? (I'm not
> > sure)
> 
> My sense is that these are pretty tiny objects that are easy to create when
> needed.  We only use it in one other place in the code, so I would be inclined
> to leave it alone, but I'll change it if you prefer.  C++ performance is *not*
> my strong suit :>
> 

I agree on the performance point - probably not a big deal.  However, it is also
code duplication, though I guess it's not enough to worry about. I guess the
best fix, which is beyond the scope of this CL is to push the use of
HostPortProxyPair all the way up the stack.

http://codereview.chromium.org/3417010/diff/166002/147021#newcode224
net/http/http_proxy_client_socket_pool.cc:224: 
This error codes should only be used if
(!transport_socket_handle_->is_ssl_error()).  I think...

http://codereview.chromium.org/3417010/diff/166002/147021#newcode259
net/http/http_proxy_client_socket_pool.cc:259: pair, params_->spdy_settings(),
transport_socket_handle_.release(),
Shouldn't we disconnect the transport_socket_handle here?  You used to have a
Reset(), but dropped even that...

http://codereview.chromium.org/3417010/diff/166002/147023
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147023#newcode245
net/http/http_proxy_client_socket_pool_unittest.cc:245: Initialize(false, reads,
arraysize(reads), writes, arraysize(writes),
I appreciate the effort to unify everything, but it might be easier to read and
see what's going on if you break the startup out by case.. i.e.

if (GetParam() != SPDY) {
  Mock reads[] = ...
  ...
  Initialize(...
} else {
  Spdy reads[] = ...
  ...
  Initialize(...
}

Or something along those lines.

http://codereview.chromium.org/3417010/diff/166002/147024
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147024#newcode806
net/http/http_stream_request.cc:806: return proxy_info()->is_https() &&
!request_info().url.SchemeIs("https");
Seems that this should check for SchemeIs("http") instead of not https.

http://codereview.chromium.org/3417010/diff/166002/147030
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3417010/diff/166002/147030#newcode264
net/spdy/spdy_framer.h:264: public:
You should move these to the public section if it's now public.

http://codereview.chromium.org/3417010/diff/166002/147031
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147031#newcode92
net/spdy/spdy_proxy_client_socket.cc:92: write_callback_ = 0;
NULL

http://codereview.chromium.org/3417010/diff/166002/147031#newcode447
net/spdy/spdy_proxy_client_socket.cc:447:
write_callback_->Run(ERR_CONNECTION_CLOSED);
I vaguely recall that the socket may be destroyed in the user callback, so we
tend to call user callbacks after doing any field manipulation. You should check
to see if it's possible that |this| is invalid after calling a callback.

[Ryan Hamilton, 2010-10-12 23:14:02]

http://codereview.chromium.org/3417010/diff/166002/147021
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147021#newcode224
net/http/http_proxy_client_socket_pool.cc:224: return
ERR_PROXY_CONNECTION_FAILED;
On 2010/10/12 22:19:09, vandebo wrote:
> This error codes should only be used if
> (!transport_socket_handle_->is_ssl_error()).  I think...

How about I add an ERR_PROXY_SSL_CONNECTION_FAILED code, and return that when
transport_socket_handle_->is_ssl_error()?

http://codereview.chromium.org/3417010/diff/166002/147021#newcode259
net/http/http_proxy_client_socket_pool.cc:259: spdy_session =
spdy_pool->Get(pair, params_->spdy_settings(), net_log());
On 2010/10/12 22:19:09, vandebo wrote:
> Shouldn't we disconnect the transport_socket_handle here?  You used to have a
> Reset(), but dropped even that...

Did I misunderstand your comment here:

http://codereview.chromium.org/3417010/diff/107001/37004#newcode255

I removed the Reset in response to your comment.

http://codereview.chromium.org/3417010/diff/166002/147023
File net/http/http_proxy_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147023#newcode245
net/http/http_proxy_client_socket_pool_unittest.cc:245: Initialize(false, reads,
arraysize(reads), writes, arraysize(writes),
On 2010/10/12 22:19:09, vandebo wrote:
> I appreciate the effort to unify everything, but it might be easier to read
and
> see what's going on if you break the startup out by case.. i.e.
> 
> if (GetParam() != SPDY) {
>   Mock reads[] = ...
>   ...
>   Initialize(...
> } else {
>   Spdy reads[] = ...
>   ...
>   Initialize(...
> }
> 
> Or something along those lines.

Ok.  I can't quite do what you propose because of the nature of the various
ConstructSpdyFrame, I need the SpdyFrame objects around for the lifetime of the
method.  If I construct them in an if() block, they'll get destroyed before the
corresponding Mock is used.  

So instead, I've created both sets of Mocks, but then call Initialize
conditionally.  Does that work for you

http://codereview.chromium.org/3417010/diff/166002/147024
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147024#newcode806
net/http/http_stream_request.cc:806: return proxy_info()->is_https() &&
!request_info().url.SchemeIs("https");
On 2010/10/12 22:19:09, vandebo wrote:
> Seems that this should check for SchemeIs("http") instead of not https.

Done.

http://codereview.chromium.org/3417010/diff/166002/147030
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3417010/diff/166002/147030#newcode264
net/spdy/spdy_framer.h:264: public:
On 2010/10/12 22:19:09, vandebo wrote:
> You should move these to the public section if it's now public.

Removed the public modifier.  (Left over from testing)

http://codereview.chromium.org/3417010/diff/166002/147031
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/166002/147031#newcode92
net/spdy/spdy_proxy_client_socket.cc:92: write_callback_ = 0;
On 2010/10/12 22:19:09, vandebo wrote:
> NULL

Done.

http://codereview.chromium.org/3417010/diff/166002/147031#newcode447
net/spdy/spdy_proxy_client_socket.cc:447:
write_callback_->Run(ERR_CONNECTION_CLOSED);
On 2010/10/12 22:19:09, vandebo wrote:
> I vaguely recall that the socket may be destroyed in the user callback, so we
> tend to call user callbacks after doing any field manipulation. You should
check
> to see if it's possible that |this| is invalid after calling a callback.

Wow, this hurts my head :>  I'm not sure how to determine if it's possible that
|this| is invalid after the callback, so I instead modified the code to use the
convention you suggested (perform all the field manipulations before the
callbacks)

[vandebo (ex-Chrome), 2010-10-12 23:45:24]

LGTM with comments.

http://codereview.chromium.org/3417010/diff/166002/147030
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3417010/diff/166002/147030#newcode264
net/spdy/spdy_framer.h:264: public:
On 2010/10/12 23:14:03, Ryan Hamilton wrote:
> On 2010/10/12 22:19:09, vandebo wrote:
> > You should move these to the public section if it's now public.
> 
> Removed the public modifier.  (Left over from testing)
> 

Umm, you just commented it out.

http://codereview.chromium.org/3417010/diff/141005/142014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/141005/142014#newcode458
net/spdy/spdy_proxy_client_socket.cc:458: if (write_callback) {
nit: extra {}'s

http://codereview.chromium.org/3417010/diff/197001/198003
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/197001/198003#newcode261
net/http/http_proxy_client_socket_pool.cc:261:
transport_socket_handle_.get()->Reset();
Shouldn't this be transport_socket_handle_->Reset()?

http://codereview.chromium.org/3417010/diff/197001/198013
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/197001/198013#newcode453
net/spdy/spdy_proxy_client_socket.cc:453: read_callback_->Run(status);
Do you want to clear the read_callback in this case?

[Ryan Hamilton, 2010-10-13 15:48:39]

http://codereview.chromium.org/3417010/diff/166002/147030
File net/spdy/spdy_framer.h (right):

http://codereview.chromium.org/3417010/diff/166002/147030#newcode264
net/spdy/spdy_framer.h:264: public:
On 2010/10/12 23:45:25, vandebo wrote:
> On 2010/10/12 23:14:03, Ryan Hamilton wrote:
> > On 2010/10/12 22:19:09, vandebo wrote:
> > > You should move these to the public section if it's now public.
> > 
> > Removed the public modifier.  (Left over from testing)
> > 
> 
> Umm, you just commented it out.

*sob*  Done.

http://codereview.chromium.org/3417010/diff/141005/142014
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/141005/142014#newcode458
net/spdy/spdy_proxy_client_socket.cc:458: if (write_callback) {
On 2010/10/12 23:45:25, vandebo wrote:
> nit: extra {}'s

Done.

http://codereview.chromium.org/3417010/diff/197001/198003
File net/http/http_proxy_client_socket_pool.cc (right):

http://codereview.chromium.org/3417010/diff/197001/198003#newcode261
net/http/http_proxy_client_socket_pool.cc:261:
transport_socket_handle_.get()->Reset();
On 2010/10/12 23:45:25, vandebo wrote:
> Shouldn't this be transport_socket_handle_->Reset()?

Done.

http://codereview.chromium.org/3417010/diff/197001/198013
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/3417010/diff/197001/198013#newcode453
net/spdy/spdy_proxy_client_socket.cc:453: read_callback_->Run(status);
On 2010/10/12 23:45:25, vandebo wrote:
> Do you want to clear the read_callback in this case?

Done.

[willchan no longer on Chromium, 2010-12-02 22:44:31]

http://codereview.chromium.org/3417010/diff/201003/net/http/http_stream_reque...
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/3417010/diff/201003/net/http/http_stream_reque...
net/http/http_stream_request.cc:757: bool useRelativeUrl = direct ||
request_info().url.SchemeIs("https");
is this WebKit style sneaking in here? :P
s/useRelativeUrl/use_relative_url/