Notify NQE of TCP RTT values

net/socket/tcp_client_socket.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_client_socket.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/time/time.h"
 #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();
   use_history_.set_was_ever_connected();
 }
 
 TCPClientSocket::~TCPClientSocket() {
   Disconnect();
 }
 
@@ skipping 97 matching lines @@
       if (bind_address_) {
         result = socket_->Bind(*bind_address_);
         if (result != OK) {
           socket_->Close();
           return result;
         }
       }
     }
   }
 
+  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,
                           base::Bind(&TCPClientSocket::DidCompleteConnect,
                                      base::Unretained(this)));
 }
 
 int TCPClientSocket::DoConnectComplete(int result) {
   if (result == OK) {
     use_history_.set_was_ever_connected();
@@ skipping 23 matching lines @@
   bind_address_.reset();
 }
 
 void TCPClientSocket::DoDisconnect() {
   total_received_bytes_ = 0;
   EmitTCPMetricsHistogramsOnDisconnect();
   // If connecting or already connected, record that the socket has been
   // disconnected.
   previously_disconnected_ = socket_->IsValid() && current_address_index_ >= 0;
   socket_->Close();
+  notify_reset_socket_performance_watcher_ = true;
 }
 
 bool TCPClientSocket::IsConnected() const {
   return socket_->IsConnected();
 }
 
 bool TCPClientSocket::IsConnectedAndIdle() const {
   return socket_->IsConnectedAndIdle();
 }
 
@@ skipping 167 matching lines @@
 void TCPClientSocket::EmitTCPMetricsHistogramsOnDisconnect() {
   base::TimeDelta rtt;
   if (socket_->GetEstimatedRoundTripTime(&rtt)) {
     UMA_HISTOGRAM_CUSTOM_TIMES("Net.TcpRtt.AtDisconnect", rtt,
                                base::TimeDelta::FromMilliseconds(1),
                                base::TimeDelta::FromMinutes(10), 100);
   }
 }
 
 }  // namespace net