Collect all ConnectionAttempts from both sockets in TransportConnectJob.

net/socket/transport_client_socket_pool.cc

Index: net/socket/transport_client_socket_pool.cc
diff --git a/net/socket/transport_client_socket_pool.cc b/net/socket/transport_client_socket_pool.cc
index f2d124788345989d0abcdd41a83bc0e6df43b4a9..1b8982fbe0bcfc4eb1dcb488466b1e61ffbcad60 100644
--- a/net/socket/transport_client_socket_pool.cc
+++ b/net/socket/transport_client_socket_pool.cc
@@ -243,9 +243,10 @@ void TransportConnectJob::GetAdditionalErrorState(ClientSocketHandle* handle) {
     DCHECK_EQ(0u, helper_.addresses().size());
     attempts.push_back(ConnectionAttempt(IPEndPoint(), resolve_result_));
   } else if (connect_result_ != OK) {
-    DCHECK_LT(0u, helper_.addresses().size());
-    attempts.push_back(
-        ConnectionAttempt(helper_.addresses().back(), connect_result_));

[Randy Smith (Not in Mondays), 2015/05/01 17:39:50]

Doesn't this mean we can nuke connect_result_ from the member variable list?

[Deprecated (see juliatuttle), 2015/05/04 19:44:50]

Why? We're still using it in the new version here.

[Randy Smith (Not in Mondays), 2015/05/06 15:30:11]

Ah, sorry, missed that.

I do wonder a bit about whether or not it's the right choice to not save errors
when we eventually get to success, but that's a high level question that doesn't
seem that important.

+    attempts.insert(attempts.begin(), connection_attempts_.begin(),
+                    connection_attempts_.end());
+    attempts.insert(attempts.begin(), fallback_connection_attempts_.begin(),
+                    fallback_connection_attempts_.end());

[Randy Smith (Not in Mondays), 2015/05/01 17:39:49]

Is ordering relevant for the ConnectionAttempts vector?  If so, probably worth a
comment in the header file as to what it means, and possibly here as to why you
chose this ordering.  If not, might be worthwhile making it a Set rather than a
Vector, or including a comment in the header file if you want to keep it a
vector, just so that consumers don't rely on the ordering in some weird way.

[Deprecated (see juliatuttle), 2015/05/04 19:44:50]

Ordering is approximately "the order the connections were made", except main and
fallback aren't interleaved.

What I should really be doing is recording start and end times for each attempt,
so Domain Reliability can use those specifically.

[Randy Smith (Not in Mondays), 2015/05/06 15:30:11]

I still feel like this means we should make it clear at the definition location
that there's no ordering guarantees (or what the ordering guarantees are),
either with a comment or hitting it over the head by using a set<>.  If you
later add start/end times that won't change the lack of ordering guarantees.

   }
   handle->set_connection_attempts(attempts);
 }
@@ -329,6 +330,8 @@ int TransportConnectJob::DoTransportConnect() {
 }
 
 int TransportConnectJob::DoTransportConnectComplete(int result) {
+  transport_socket_->GetConnectionAttempts(&connection_attempts_);
+
   if (result == OK) {
     bool is_ipv4 =
         helper_.addresses().front().GetFamily() == ADDRESS_FAMILY_IPV4;
@@ -426,6 +429,9 @@ void TransportConnectJob::DoIPv6FallbackTransportConnectComplete(int result) {
   DCHECK(fallback_transport_socket_.get());
   DCHECK(fallback_addresses_.get());
 
+  fallback_transport_socket_->GetConnectionAttempts(
+      &fallback_connection_attempts_);
+
   if (result == OK) {
     DCHECK(!fallback_connect_start_time_.is_null());
     connect_timing_.connect_start = fallback_connect_start_time_;