Notify NQE of TCP RTT values

net/socket/tcp_client_socket.cc

Index: net/socket/tcp_client_socket.cc
diff --git a/net/socket/tcp_client_socket.cc b/net/socket/tcp_client_socket.cc
index 56238ba0d12cb5412df6fc317770ea9aa561fe24..4a43d56e37f954c97626b4432b3399731498883e 100644
--- a/net/socket/tcp_client_socket.cc
+++ b/net/socket/tcp_client_socket.cc
@@ -14,27 +14,36 @@
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
+#include "net/base/socket_performance_watcher.h"
 
 namespace net {
 
-TCPClientSocket::TCPClientSocket(const AddressList& addresses,
-                                 net::NetLog* net_log,
-                                 const net::NetLog::Source& source)
-    : socket_(new TCPSocket(net_log, source)),
+TCPClientSocket::TCPClientSocket(
+    const AddressList& addresses,
+    scoped_ptr<SocketPerformanceWatcher> socket_performance_watcher,
+    net::NetLog* net_log,
+    const net::NetLog::Source& source)
+    : socket_performance_watcher_(socket_performance_watcher.get()),
+      socket_(new TCPSocket(std::move(socket_performance_watcher),

[Ryan Sleevi, 2016/04/11 22:06:55]

This is an intrinsically dangerous pattern (the .get() plus the move()), because
it can complicate reasoning about lifetimes.

Let's discuss this separately, to see if we can't work out an approach. I'm fine
with this for now, however, in this limited case.

[tbansal1, 2016/04/12 16:14:33]

I agree. I am not sure what's the best approach here but another possible
solution is to have the |socket_| expose the watcher (this will possibly need
friend'ing TCPClientSocket).

[Ryan Sleevi, 2016/04/12 21:08:31]

Friending is almost always the wrong decision. Certainly, if it isn't test code,
and it isn't an implementation detail, they shouldn't be friends. Anyways, we
should follow-up separately.

[tbansal1, 2016/04/12 22:39:19]

Acknowledged.

+                            net_log,
+                            source)),
       addresses_(addresses),
       current_address_index_(-1),
       next_connect_state_(CONNECT_STATE_NONE),
       previously_disconnected_(false),
-      total_received_bytes_(0) {}
+      total_received_bytes_(0),
+      notify_reset_socket_performance_watcher_(false) {}
 
 TCPClientSocket::TCPClientSocket(scoped_ptr<TCPSocket> connected_socket,
                                  const IPEndPoint& peer_address)
-    : socket_(std::move(connected_socket)),
+    : socket_performance_watcher_(nullptr),
+      socket_(std::move(connected_socket)),
       addresses_(AddressList(peer_address)),
       current_address_index_(0),
       next_connect_state_(CONNECT_STATE_NONE),
       previously_disconnected_(false),
-      total_received_bytes_(0) {
+      total_received_bytes_(0),
+      notify_reset_socket_performance_watcher_(false) {

[Ryan Sleevi, 2016/04/11 22:06:56]

This is really just

is_first_connection_ isn't it?

Is there any other time or reasons we wouldn't notify? The way it's named now
hides that relationship, so perhaps this would make it clearer.

[tbansal1, 2016/04/12 16:14:33]

Removed the variable.

   DCHECK(socket_);
 
   socket_->SetDefaultOptionsForClient();
@@ -152,6 +161,9 @@ int TCPClientSocket::DoConnect() {
     }
   }
 
+  if (socket_performance_watcher_ && notify_reset_socket_performance_watcher_)
+    socket_performance_watcher_->OnConnectionChanged();
+
   // |socket_| is owned by this class and the callback won't be run once
   // |socket_| is gone. Therefore, it is safe to use base::Unretained() here.
   return socket_->Connect(endpoint,
@@ -195,6 +207,7 @@ void TCPClientSocket::DoDisconnect() {
   // disconnected.
   previously_disconnected_ = socket_->IsValid() && current_address_index_ >= 0;
   socket_->Close();
+  notify_reset_socket_performance_watcher_ = true;
 }
 
 bool TCPClientSocket::IsConnected() const {