Notify NQE of TCP RTT values

net/socket/tcp_socket_posix.cc

Index: net/socket/tcp_socket_posix.cc
diff --git a/net/socket/tcp_socket_posix.cc b/net/socket/tcp_socket_posix.cc
index 68229ff68435ba7e077f80122bb95dbf9e0264e2..5557cbe5f410701f88c6ee9c795690d7edd37efb 100644
--- a/net/socket/tcp_socket_posix.cc
+++ b/net/socket/tcp_socket_posix.cc
@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/profiler/scoped_tracker.h"
 #include "base/task_runner_util.h"
 #include "base/threading/worker_pool.h"
 #include "net/base/address_list.h"
@@ -136,8 +137,12 @@ void CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
 #endif
 }
 
-TCPSocketPosix::TCPSocketPosix(NetLog* net_log, const NetLog::Source& source)
-    : use_tcp_fastopen_(false),
+TCPSocketPosix::TCPSocketPosix(
+    scoped_ptr<SocketPerformanceWatcher> socket_performance_watcher,
+    NetLog* net_log,
+    const NetLog::Source& source)
+    : socket_performance_watcher_(std::move(socket_performance_watcher)),
+      use_tcp_fastopen_(false),
       tcp_fastopen_write_attempted_(false),
       tcp_fastopen_connected_(false),
       tcp_fastopen_status_(TCP_FASTOPEN_STATUS_UNKNOWN),
@@ -514,14 +519,16 @@ int TCPSocketPosix::BuildTcpSocketPosix(scoped_ptr<TCPSocketPosix>* tcp_socket,
     return ERR_ADDRESS_INVALID;
   }
 
-  tcp_socket->reset(new TCPSocketPosix(net_log_.net_log(), net_log_.source()));
+  tcp_socket->reset(
+      new TCPSocketPosix(nullptr, net_log_.net_log(), net_log_.source()));

[Wez, 2016/03/04 18:50:26]

IIUC this means that you can't associated a socket-performance-watcher with a
server socket (one received via accept())?  If you passed the factory all the
way down to the TCPSocket ctor then you could allow that to work, since
BuildTcpSocketPosix could just create a new watcher instance.

[tbansal1, 2016/03/09 00:20:03]

SocketPerformanceWatcher only expects RTT values from connections that are
initiated by Chrome since these connections are more likely to be from Chrome to
a web server (that's what Socket Performance Watcher is interested in).

   (*tcp_socket)->socket_.reset(accept_socket_.release());
   return OK;
 }
 
 void TCPSocketPosix::ConnectCompleted(const CompletionCallback& callback,
-                                      int rv) const {
+                                      int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleConnectCompleted(rv));
 }
 
@@ -575,6 +582,7 @@ void TCPSocketPosix::ReadCompleted(const scoped_refptr<IOBuffer>& buf,
                                    const CompletionCallback& callback,
                                    int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleReadCompleted(buf.get(), rv));
 }
 
@@ -612,6 +620,7 @@ void TCPSocketPosix::WriteCompleted(const scoped_refptr<IOBuffer>& buf,
                                     const CompletionCallback& callback,
                                     int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleWriteCompleted(buf.get(), rv));
 }
 
@@ -699,6 +708,33 @@ int TCPSocketPosix::TcpFastOpenWrite(IOBuffer* buf,
   return socket_->WaitForWrite(buf, buf_len, callback);
 }
 
+void TCPSocketPosix::NotifySocketPerformanceWatcher() {
+#if defined(TCP_INFO)
+  // TODO(tbansal): Remove ScopedTracker once crbug.com/590254 is fixed.
+  tracked_objects::ScopedTracker tracking_profile(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "590254 TCPSocketPosix::NotifySocketPerformanceWatcher"));
+
+  // Check if |socket_performance_watcher_| can be notified of a RTT value.

[Wez, 2016/03/04 18:50:26]

nit: "can" -> "must" or reword to "...watcher_| is interested in..."

[tbansal1, 2016/03/09 00:20:03]

Done.

+  if (!socket_performance_watcher_ ||
+      !socket_performance_watcher_->ShouldNotifyUpdatedRTT()) {
+    return;
+  }
+
+  tcp_info info;
+  if (!GetTcpInfo(socket_->socket_fd(), &info))
+    return;
+
+  // Only notify the SocketPerformanceWatcher if the RTT in |tcp_info| struct
+  // was populated. A value of 0 may be valid, but it is not reported to the
+  // SocketPerformanceWatcher in order to maintain high accuracy.

[Wez, 2016/03/04 18:50:26]

nit: Not clear from the comment why omitting valid data improves accuracy.

[tbansal1, 2016/03/09 00:20:03]

I expanded the comment to make it clearer.

+  if (info.tcpi_rtt > 0) {
+    socket_performance_watcher_->OnUpdatedRTTAvailable(
+        base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
+  }
+#endif  // defined(TCP_INFO)
+}
+
 void TCPSocketPosix::UpdateTCPFastOpenStatusAfterRead() {
   DCHECK(tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ||
          tcp_fastopen_status_ == TCP_FASTOPEN_SLOW_CONNECT_RETURN);