Plumb connection attempts from (non-proxy) ConnectJobs to HttpNetworkTransaction.

Plumb connection attempts from (non-proxy) ConnectJobs to HttpNetworkTransaction.

Log connection attempts in {Transport,SSL}ConnectJob and copy them into the ClientSocketHandle. Form there, copy them up the stack through the HttpStreamRequest (on job completion) to the HttpNetworkTransaction (on stream creation or failure).

SPDY is covered by this change -- sockets for new SPDY sessions are connected on the same path as sockets for HTTP connections, and then passed off to the SPDY code.

QUIC is not covered by this change -- sockets for new QUIC sessions are created in the QUIC code, and Chrome's current QUIC behavior does not let us see those results easily. My current plan is to log those *in* the QUIC code and then grab them on the next request that would use that QUIC session. I still need to see if this is actually practical.

BUG=480565
Committed: https://crrev.com/1f2d7e9e15aee462a5e80c703f6a7d4886e23c25
Cr-Commit-Position: refs/heads/master@{#327298}

[Ryan Hamilton, 2015-03-12 20:06:03]

rch@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2015-03-12 20:06:04]

Much of this seems good. Am I correct to understand that this will only report
TCP connection errors, not TLS handshake errors?  Seems like if the TCP
connection succeeded, but the TLS handshake failed we would want to know that.

https://codereview.chromium.org/1006643002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:253:
handle->set_connection_attempts(attempts);
Can you simply do this:

handle->set_connection_attempts(
  transport_socket_handle_->connection_attempts());

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:238: if (connect_result_ != OK) {
nit: early return for the win

if (connect_result_ == OK)
  return;

DCHECK(...);
...

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:242: attempt.result =
connect_result_;
consider adding a 2 arg constructor to ConnectionAttempt to remove boiler-plate
code like this.

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:243:
std::vector<ClientSocketHandle::ConnectionAttempt> attempts;
Consider using a typdef to make this expression something like
ClientSocketHandle::ConnectionAttempts.

[Deprecated (see juliatuttle), 2015-03-16 15:53:54]

PTAL, Ryan.

I made your changes, including capturing SSL connection errors in SSLConnectJob.
I've also added a stub in HttpStreamFactoryImpl::Job that console-logs the
errors it would copy into the request; it appears to log the proper errors the
proper number of times, so the next step is to find a way to copy them the rest
of the way up the stack.

https://codereview.chromium.org/1006643002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool.cc:253:
handle->set_connection_attempts(attempts);
On 2015/03/12 20:06:04, Ryan Hamilton wrote:
> Can you simply do this:
> 
> handle->set_connection_attempts(
>   transport_socket_handle_->connection_attempts());

Done.

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:238: if (connect_result_ != OK) {
On 2015/03/12 20:06:04, Ryan Hamilton wrote:
> nit: early return for the win
> 
> if (connect_result_ == OK)
>   return;
> 
> DCHECK(...);
> ...

Done.

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:242: attempt.result =
connect_result_;
On 2015/03/12 20:06:04, Ryan Hamilton wrote:
> consider adding a 2 arg constructor to ConnectionAttempt to remove
boiler-plate
> code like this.

Done.

https://codereview.chromium.org/1006643002/diff/1/net/socket/transport_client...
net/socket/transport_client_socket_pool.cc:243:
std::vector<ClientSocketHandle::ConnectionAttempt> attempts;
On 2015/03/12 20:06:04, Ryan Hamilton wrote:
> Consider using a typdef to make this expression something like
> ClientSocketHandle::ConnectionAttempts.

Done.

[Ryan Hamilton, 2015-03-17 03:22:29]

Heh, this CL has grown :>

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:759: CopyConnectionAttempts();
Is it clear that these are the only places this method should be called?

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1556:
connection_attempts_.push_back(attempt);
Could you simply do connection_attempts_ =
stream_request_->connection_attempts(); ?

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory.h:176: virtual const
ClientSocketHandle::ConnectionAttempts& connection_attempts()
If this struct is going to be used outside of the ClientSocketHandle class, then
it should probably be defined elsewhere.

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.cc:462: direct_address_ = IPEndPoint();
If this is the only place this member is used, it doesn't seem like you need to
reset it. I assume you can't call GetPeerAddress() here because the transport
socket might not be connected?

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.h:265: IPEndPoint direct_address_;
Looks like this is the peer address. In that case, I think I'd call it
peer_address_

[Deprecated (see juliatuttle), 2015-03-23 17:45:35]

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:759: CopyConnectionAttempts();
On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> Is it clear that these are the only places this method should be called?

It was clear to me when I wrote it; I looked for places that HNT reset
stream_request_ to find places it was "consumed".

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1556:
connection_attempts_.push_back(attempt);
On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> Could you simply do connection_attempts_ =
> stream_request_->connection_attempts(); ?

Can we not restart the request with auth data? I want to make sure we capture
any errors in both the pre- and post-auth request at the URLRequest layer, once
I get there.

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory.h:176: virtual const
ClientSocketHandle::ConnectionAttempts& connection_attempts()
On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> If this struct is going to be used outside of the ClientSocketHandle class,
then
> it should probably be defined elsewhere.

Yeah, I agree, but I'm not sure where. I suppose I could just throw another
small .h in net/base.

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.cc:462: direct_address_ = IPEndPoint();
On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> If this is the only place this member is used, it doesn't seem like you need
to
> reset it. I assume you can't call GetPeerAddress() here because the transport
> socket might not be connected?

I'm resetting it just to be careful, and yeah, I tried calling GetPeerAddress
here but the socket can be closed by here.

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.h:265: IPEndPoint direct_address_;
On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> Looks like this is the peer address. In that case, I think I'd call it
> peer_address_

It's specifically called direct_address_ because it's *only* populated if we
have a direct connection to the host.

[Deprecated (see juliatuttle), 2015-03-24 18:22:15]

Patchset #5 (id:80001) has been deleted

[Ryan Hamilton, 2015-03-24 19:20:19]

Good progress. Sorry for the delay. I'm slammed and am about to be OOO for about
a month. You might want to find another reviewer :(

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:1556:
connection_attempts_.push_back(attempt);
On 2015/03/23 17:45:35, ttuttle wrote:
> On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> > Could you simply do connection_attempts_ =
> > stream_request_->connection_attempts(); ?
> 
> Can we not restart the request with auth data? I want to make sure we capture
> any errors in both the pre- and post-auth request at the URLRequest layer,
once
> I get there.

Oh, are you explicitly attempting to append and not assign?? I see. In that
case, I recommend adding a comment.

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/http/http_stream_fa...
net/http/http_stream_factory.h:176: virtual const
ClientSocketHandle::ConnectionAttempts& connection_attempts()
On 2015/03/23 17:45:35, ttuttle wrote:
> On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> > If this struct is going to be used outside of the ClientSocketHandle class,
> then
> > it should probably be defined elsewhere.
> 
> Yeah, I agree, but I'm not sure where. I suppose I could just throw another
> small .h in net/base.

My vote would go for net/socket/connection_attempts.h

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/1006643002/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_pool.h:265: IPEndPoint direct_address_;
On 2015/03/23 17:45:35, ttuttle wrote:
> On 2015/03/17 03:22:28, Ryan Hamilton wrote:
> > Looks like this is the peer address. In that case, I think I'd call it
> > peer_address_
> 
> It's specifically called direct_address_ because it's *only* populated if we
> have a direct connection to the host.

Hm. I appreciate the distinction you're going for. But I don't think that the
name "direct" help. That sounds like "the host I'm actually talking to via TCP"
which may well be a proxy :/ server_address?

https://codereview.chromium.org/1006643002/diff/100001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1006643002/diff/100001/net/http/http_network_...
net/http/http_network_transaction.h:101: 
Probably should have a comment here. The other methods do not because they're
all overrides.

https://codereview.chromium.org/1006643002/diff/100001/net/http/http_network_...
net/http/http_network_transaction.h:102: const
ClientSocketHandle::ConnectionAttempts& connection_attempts() const;
This makes me wonder if ConnectionAttempts should be defined in some other .h
file since it's getting exposed up the stack. It doesn't seem like consumers
should care about ClientSocketHandles. maybe just
net/socket/connection_attempts.h ?

https://codereview.chromium.org/1006643002/diff/100001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1006643002/diff/100001/net/socket/client_sock...
net/socket/client_socket_handle.h:43: // A record of a TCP connection attempt
made to connect to the host.
Can you update the comment to mention TLS errors?

[Deprecated (see juliatuttle), 2015-04-08 19:17:19]

https://codereview.chromium.org/1006643002/diff/100001/net/http/http_network_...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1006643002/diff/100001/net/http/http_network_...
net/http/http_network_transaction.h:102: const
ClientSocketHandle::ConnectionAttempts& connection_attempts() const;
On 2015/03/24 19:20:19, Ryan Hamilton wrote:
> This makes me wonder if ConnectionAttempts should be defined in some other .h
> file since it's getting exposed up the stack. It doesn't seem like consumers
> should care about ClientSocketHandles. maybe just
> net/socket/connection_attempts.h ?

Done.

https://codereview.chromium.org/1006643002/diff/100001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1006643002/diff/100001/net/socket/client_sock...
net/socket/client_socket_handle.h:43: // A record of a TCP connection attempt
made to connect to the host.
On 2015/03/24 19:20:19, Ryan Hamilton wrote:
> Can you update the comment to mention TLS errors?

Done.

[Randy Smith (Not in Mondays), 2015-04-10 21:15:02]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-04-10 21:15:02]

A couple of high level questions:
* Do none of the other connect jobs need to have the details of their underlying
transport/ssl connect job connections exposed?  Why not?

* I'm a bit uncomfortable about the lack of specificity in naming around the
connection attempt copying machinery.  I find
AddConnectionAttempts(connection_attempts) and
set_connection_attempts(connection_attempts) to be intuitive (the argument goes
into object internals storage), but CopyConnectionAttempts() and
MaybeCopyConnectionAttempts() I find more confusion--CopyConnectionAttempts()
copies from a related object into a data member, and
MaybeCopyConnectinAttempts() copies from one non-self object another.  Maybe
"MaybeAddConnectionAttempts()" -> "MaybeTransferConnectionAttempts()" and
include a comment as to what it's doing in the header, and
CopyConnectionAttempts() -> CopyConnectionAttemptsFromStream?

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:770: if (result !=
ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
Willing to educate me about why a proxy tunnel response is special?  It looks
like you're intercepting it in one place than avoiding intercepting it in the
other (here); why is this code not equivalent to getting rid of the conditional
here and the full call in OnHttpsProxyTunnelResponse?

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
net/http/http_stream_factory.h:19: #include "net/socket/client_socket_handle.h"
nit: Why is this added?  None of the other additions in this file seem to use
it.

https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
net/socket/client_socket_handle.h:141: void
set_connection_attempts(ConnectionAttempts attempts) {
Why note const&?

[Deprecated (see juliatuttle), 2015-04-14 19:14:36]

On 2015/04/10 21:15:02, rdsmith wrote:
> A couple of high level questions:
> * Do none of the other connect jobs need to have the details of their
underlying
> transport/ssl connect job connections exposed?  Why not?

I don't think so. I'm specifically aiming to get data for *direct* connections
-- Domain Reliability specifically does *not* want to report proxy IPs and the
like.

It is entirely plausible that, at some point in the future, someone could expand
ConnectionAttempt to capture proxy and proxy-tunnel connections as well.

> * I'm a bit uncomfortable about the lack of specificity in naming around the
> connection attempt copying machinery.  I find
> AddConnectionAttempts(connection_attempts) and
> set_connection_attempts(connection_attempts) to be intuitive (the argument
goes
> into object internals storage), but CopyConnectionAttempts() and
> MaybeCopyConnectionAttempts() I find more confusion--CopyConnectionAttempts()
> copies from a related object into a data member, and
> MaybeCopyConnectinAttempts() copies from one non-self object another.  Maybe
> "MaybeAddConnectionAttempts()" -> "MaybeTransferConnectionAttempts()" and
> include a comment as to what it's doing in the header, and
> CopyConnectionAttempts() -> CopyConnectionAttemptsFromStream?

Fixed.

>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
> net/http/http_network_transaction.cc:770: if (result !=
> ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
> Willing to educate me about why a proxy tunnel response is special?  It looks
> like you're intercepting it in one place than avoiding intercepting it in the
> other (here); why is this code not equivalent to getting rid of the
conditional
> here and the full call in OnHttpsProxyTunnelResponse?

Commented.

>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
> File net/http/http_stream_factory.h (right):
> 
>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
> net/http/http_stream_factory.h:19: #include
"net/socket/client_socket_handle.h"
> nit: Why is this added?  None of the other additions in this file seem to use
> it.

Whoops, didn't get caught in refactoring. Done.

>
https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
> File net/socket/client_socket_handle.h (right):
> 
>
https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
> net/socket/client_socket_handle.h:141: void
> set_connection_attempts(ConnectionAttempts attempts) {
> Why note const&?

Done.

[Deprecated (see juliatuttle), 2015-04-14 19:14:45]

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:770: if (result !=
ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
On 2015/04/10 21:15:03, rdsmith wrote:
> Willing to educate me about why a proxy tunnel response is special?  It looks
> like you're intercepting it in one place than avoiding intercepting it in the
> other (here); why is this code not equivalent to getting rid of the
conditional
> here and the full call in OnHttpsProxyTunnelResponse?

Yes: we reset stream_request_ in OnHttpsProxyTunnelResponse, so we have to grab
the connection attempts early and then avoid grabbing them again later (when
stream_request_ is NULL).

I can make this a comment if you like.

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
net/http/http_stream_factory.h:19: #include "net/socket/client_socket_handle.h"
On 2015/04/10 21:15:03, rdsmith wrote:
> nit: Why is this added?  None of the other additions in this file seem to use
> it.

Leftover from when ConnectionAttempts was a member of ClientSocketHandle.

https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/1006643002/diff/200001/net/socket/client_sock...
net/socket/client_socket_handle.h:141: void
set_connection_attempts(ConnectionAttempts attempts) {
On 2015/04/10 21:15:03, rdsmith wrote:
> Why note const&?

Done.

[Randy Smith (Not in Mondays), 2015-04-15 20:19:07]

Summary of offline conversation:

-- I'd like a plan or explicit lack of plan for QUIC and SPDY.

-- I'd like a high level summary of the architecture of this CL somewhere.  (I'm
ok with it being in the CL description; that may be the right place.)  Something
like:

   During connection TransportConnectJob and SSLConnectJob will store their
connection attempts on the ClientSocketHandle.  These attempts will be copied
into the HttpStreamRequest (== HttpStreamFactoryImpl::Request) during HttpStream
creation, and from there to HttpNetworkTransaction after the stream or an error
is returned from stream creation.

-- You should think about how you're going to do testing, though I'm happy to
review the rest of the CL in parallel with that.

-- I think you've got an un-uploaded PS somewhere that contains the actions
taken in response to my minor comments so far.  Can I haz PS?  :-}

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
net/http/http_network_transaction.cc:770: if (result !=
ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
On 2015/04/14 19:14:45, ttuttle wrote:
> On 2015/04/10 21:15:03, rdsmith wrote:
> > Willing to educate me about why a proxy tunnel response is special?  It
looks
> > like you're intercepting it in one place than avoiding intercepting it in
the
> > other (here); why is this code not equivalent to getting rid of the
> conditional
> > here and the full call in OnHttpsProxyTunnelResponse?
> 
> Yes: we reset stream_request_ in OnHttpsProxyTunnelResponse, so we have to
grab
> the connection attempts early and then avoid grabbing them again later (when
> stream_request_ is NULL).
> 
> I can make this a comment if you like.

Yes, please.  (If it were arcana about tunneling in the abstract, I wouldn't
request it, since I know I really don't grok proxies & tunnels & proxy tunnels,
but if it's because of an oddity of this code, that seems worth doing.)

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
File net/http/http_stream_factory.h (right):

https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
net/http/http_stream_factory.h:19: #include "net/socket/client_socket_handle.h"
On 2015/04/14 19:14:45, ttuttle wrote:
> On 2015/04/10 21:15:03, rdsmith wrote:
> > nit: Why is this added?  None of the other additions in this file seem to
use
> > it.
> 
> Leftover from when ConnectionAttempts was a member of ClientSocketHandle.

Hmmm.  Why doesn't that mean it should be removed?

[Deprecated (see juliatuttle), 2015-04-22 21:44:12]

Patchset #13 (id:260001) has been deleted

[Deprecated (see juliatuttle), 2015-04-22 21:50:25]

On 2015/04/15 20:19:07, rdsmith wrote:
> Summary of offline conversation:
> 
> -- I'd like a plan or explicit lack of plan for QUIC and SPDY.

Done, in the CL description.

> -- I'd like a high level summary of the architecture of this CL somewhere. 
(I'm
> ok with it being in the CL description; that may be the right place.) 
Something
> like:
> 
>    During connection TransportConnectJob and SSLConnectJob will store their
> connection attempts on the ClientSocketHandle.  These attempts will be copied
> into the HttpStreamRequest (== HttpStreamFactoryImpl::Request) during
HttpStream
> creation, and from there to HttpNetworkTransaction after the stream or an
error
> is returned from stream creation.

Done.

> -- You should think about how you're going to do testing, though I'm happy to
> review the rest of the CL in parallel with that.

Done; I've added some tests.

> -- I think you've got an un-uploaded PS somewhere that contains the actions
> taken in response to my minor comments so far.  Can I haz PS?  :-}

Done, plus several more :)

>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
> File net/http/http_network_transaction.cc (right):
> 
>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_network_...
> net/http/http_network_transaction.cc:770: if (result !=
> ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
> On 2015/04/14 19:14:45, ttuttle wrote:
> > On 2015/04/10 21:15:03, rdsmith wrote:
> > > Willing to educate me about why a proxy tunnel response is special?  It
> looks
> > > like you're intercepting it in one place than avoiding intercepting it in
> the
> > > other (here); why is this code not equivalent to getting rid of the
> > conditional
> > > here and the full call in OnHttpsProxyTunnelResponse?
> > 
> > Yes: we reset stream_request_ in OnHttpsProxyTunnelResponse, so we have to
> grab
> > the connection attempts early and then avoid grabbing them again later (when
> > stream_request_ is NULL).
> > 
> > I can make this a comment if you like.
> 
> Yes, please.  (If it were arcana about tunneling in the abstract, I wouldn't
> request it, since I know I really don't grok proxies & tunnels & proxy
tunnels,
> but if it's because of an oddity of this code, that seems worth doing.)

Done.

>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
> File net/http/http_stream_factory.h (right):
> 
>
https://codereview.chromium.org/1006643002/diff/200001/net/http/http_stream_f...
> net/http/http_stream_factory.h:19: #include
"net/socket/client_socket_handle.h"
> On 2015/04/14 19:14:45, ttuttle wrote:
> > On 2015/04/10 21:15:03, rdsmith wrote:
> > > nit: Why is this added?  None of the other additions in this file seem to
> use
> > > it.
> > 
> > Leftover from when ConnectionAttempts was a member of ClientSocketHandle.
> 
> Hmmm.  Why doesn't that mean it should be removed?

It doesn't; I fixed it.

[Deprecated (see juliatuttle), 2015-04-22 21:50:40]

PTAL, rdsmith.

[Randy Smith (Not in Mondays), 2015-04-23 19:04:09]

High level comments:

* You don't currently have this CL linked to a bug.  Given that I understand it
as being part of a larger, multi-CL effort, I think there should be a tracking
bug?

* You probably don't need to worry about this, but if TCP Fast Open is active,
connection establishment won't happen until first write, and you won't see that
error.  If we ever deploy TCP Fast Open more broadly, we'll need to go back and
fix that.  

I'm also a bit worried about testing all this once you stitch it all together;
there are a lot of moving pieces.  But I suspect you're worried about that too,
so I won't worry about it too much :-}.

https://codereview.chromium.org/1006643002/diff/280001/net/http/failing_http_...
File net/http/failing_http_transaction_factory.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/failing_http_...
net/http/failing_http_transaction_factory.cc:170: NOTIMPLEMENTED();
This seems like it would break any browser level tests using
FailingHttpTransaction, but if all try jobs pass, I'm good with it.

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:698: // instead of just proxying to the
(current?) HttpNetworkTransaction?
Put a little bit more detail into this TODO, maybe your current view of the
arguments for/against doing accumulation?  I'm worried that it's targeted at
someone with your current level of knowledge.  I wouldn't have a lot of clue
about how to go about executing this TODO if I were to take on that task.  I
suspect you'll be in the same position six months from now, and someone who
isn't one of us would be in substantially worse shape.

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_network_...
net/http/http_network_transaction.cc:783: // request, so don't try to do it
again here.
Also mention (here or in OnHttpsProxyTunnelResponse) that you *can't* do it here
because the stream_request_ is reset?  This doesn't really answer the question
"Why not just do it here?"

https://codereview.chromium.org/1006643002/diff/280001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:211: connect_result_(OK) {
Should there be an initialization for resolve_result_?

[Deprecated (see juliatuttle), 2015-04-23 21:04:16]

On 2015/04/23 19:04:09, rdsmith wrote:
> High level comments:
> 
> * You don't currently have this CL linked to a bug.  Given that I understand
it
> as being part of a larger, multi-CL effort, I think there should be a tracking
> bug?

Done.

> * You probably don't need to worry about this, but if TCP Fast Open is active,
> connection establishment won't happen until first write, and you won't see
that
> error.  If we ever deploy TCP Fast Open more broadly, we'll need to go back
and
> fix that.  

Those should be fine; we'll have grabbed the address after the connection
'succeeds', and when the write fails, we'll be able to use that cached address.

> I'm also a bit worried about testing all this once you stitch it all together;
> there are a lot of moving pieces.  But I suspect you're worried about that
too,
> so I won't worry about it too much :-}.

Yeah, I'm adding tests as I go, at various levels.

https://codereview.chromium.org/1006643002/diff/280001/net/http/failing_http_...
File net/http/failing_http_transaction_factory.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/failing_http_...
net/http/failing_http_transaction_factory.cc:170: NOTIMPLEMENTED();
On 2015/04/23 19:04:08, rdsmith wrote:
> This seems like it would break any browser level tests using
> FailingHttpTransaction, but if all try jobs pass, I'm good with it.

Yeah, I'd rather have it fail explicitly if we end up using it, so whoever uses
it can think about what they want it to return. (Presumably a failing
transaction may fail in the connection phase, in which case they'd want to add a
mock value.)

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:698: // instead of just proxying to the
(current?) HttpNetworkTransaction?
On 2015/04/23 19:04:08, rdsmith wrote:
> Put a little bit more detail into this TODO, maybe your current view of the
> arguments for/against doing accumulation?  I'm worried that it's targeted at
> someone with your current level of knowledge.  I wouldn't have a lot of clue
> about how to go about executing this TODO if I were to take on that task.  I
> suspect you'll be in the same position six months from now, and someone who
> isn't one of us would be in substantially worse shape.

Better yet, I'll just do it now.

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_network_...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/http/http_network_...
net/http/http_network_transaction.cc:783: // request, so don't try to do it
again here.
On 2015/04/23 19:04:09, rdsmith wrote:
> Also mention (here or in OnHttpsProxyTunnelResponse) that you *can't* do it
here
> because the stream_request_ is reset?  This doesn't really answer the question
> "Why not just do it here?"

Done.

https://codereview.chromium.org/1006643002/diff/280001/net/socket/transport_c...
File net/socket/transport_client_socket_pool.cc (right):

https://codereview.chromium.org/1006643002/diff/280001/net/socket/transport_c...
net/socket/transport_client_socket_pool.cc:211: connect_result_(OK) {
On 2015/04/23 19:04:09, rdsmith wrote:
> Should there be an initialization for resolve_result_?

Done.

[Randy Smith (Not in Mondays), 2015-04-27 17:53:07]

Could you add a mention of the ClientSocketHandle as an intermediate resting
place for the data in your CL description?  Maybe something like 

"Log connection attempts in {Transport,SSL}ConnectJob and into the
ClientSocketHandle. Copy them up the stack through the HttpStreamRequest (on job
completion) to the HttpNetworkTransaction (on stream creation or failure)."

Beyond that LGTM.

https://codereview.chromium.org/1006643002/diff/300001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1006643002/diff/300001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2928:
old_connection_attempts_.push_back(attempt);
I find myself wondering if it might make sense to just spec
network_trans_->GetConnectionAttempts as appending rather than replacing (might
need to be done elsewhere in the stack as well).  Just a suggestion; up to you.

[Deprecated (see juliatuttle), 2015-04-27 18:01:59]

On 2015/04/27 17:53:07, rdsmith wrote:
> Could you add a mention of the ClientSocketHandle as an intermediate resting
> place for the data in your CL description?  Maybe something like 
> 
> "Log connection attempts in {Transport,SSL}ConnectJob and into the
> ClientSocketHandle. Copy them up the stack through the HttpStreamRequest (on
job
> completion) to the HttpNetworkTransaction (on stream creation or failure)."

Done.

> Beyond that LGTM.
> 
>
https://codereview.chromium.org/1006643002/diff/300001/net/http/http_cache_tr...
> File net/http/http_cache_transaction.cc (right):
> 
>
https://codereview.chromium.org/1006643002/diff/300001/net/http/http_cache_tr...
> net/http/http_cache_transaction.cc:2928:
> old_connection_attempts_.push_back(attempt);
> I find myself wondering if it might make sense to just spec
> network_trans_->GetConnectionAttempts as appending rather than replacing
(might
> need to be done elsewhere in the stack as well).  Just a suggestion; up to
you.

I think I want to keep it this way, for consistency. I might go through and make
them use OutputIterators or something later, if that seems like it would help a
lot.

[Deprecated (see juliatuttle), 2015-04-27 18:02:30]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-04-27 18:02:33]

The CQ bit was unchecked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-04-27 22:25:44]

Patchset #6 (id:120001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:25:53]

Patchset #6 (id:140001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:26:08]

Patchset #13 (id:320001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:26:17]

Patchset #13 (id:340001) has been deleted

[Deprecated (see juliatuttle), 2015-04-27 22:26:27]

Patchset #13 (id:360001) has been deleted

[yurys, 2015-04-28 06:49:17]

yurys@chromium.org changed reviewers:
+ yurys@chromium.org

[yurys, 2015-04-28 06:49:18]

devtools - lgtm

[Deprecated (see juliatuttle), 2015-04-28 16:05:34]

The CQ bit was checked by ttuttle@chromium.org

[Deprecated (see juliatuttle), 2015-04-28 16:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1006643002/#ps380001
(title: "rebase, resolve conflict")

[commit-bot: I haz the power, 2015-04-28 16:05:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006643002/380001

[commit-bot: I haz the power, 2015-04-28 16:18:00]

Committed patchset #13 (id:380001)

[commit-bot: I haz the power, 2015-04-28 16:18:52]

Patchset 13 (id:??) landed as
https://crrev.com/1f2d7e9e15aee462a5e80c703f6a7d4886e23c25
Cr-Commit-Position: refs/heads/master@{#327298}