Add SocketWatcherFactory as a helper class to NQE

net/base/network_quality_estimator.cc

Index: net/base/network_quality_estimator.cc
diff --git a/net/base/network_quality_estimator.cc b/net/base/network_quality_estimator.cc
index dfc243ef490a0ff7106a9b08df97061f75aa468f..9ab5c6649995583ef9e84d3ad10245b312583be9 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -15,10 +15,12 @@
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_base.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "net/base/load_flags.h"
 #include "net/base/load_timing_info.h"
 #include "net/base/network_interfaces.h"
+#include "net/base/socket_performance_watcher.h"
 #include "net/base/url_util.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
@@ -117,6 +119,79 @@ base::HistogramBase* GetHistogram(
 
 namespace net {
 
+// SocketWatcher implements SocketPerformanceWatcher, and notifies
+// NetworkQualityEstimator of various socket performance events. SocketWatcher
+// is not thread-safe.
+class NetworkQualityEstimator::SocketWatcher : public SocketPerformanceWatcher {

[bengr, 2016/04/07 00:20:05]

Any chance I could convince you to move this to another file?

[Ryan Sleevi, 2016/04/07 00:27:53]

I specifically asked for this. I don't feel you can reasonably move to another
file w/o adding another (unnecessary) layer of indirection, and I'm not sure I
see the value.

[tbansal1, 2016/04/07 01:43:36]

Acknowledged.

+ public:
+  SocketWatcher(
+      SocketPerformanceWatcherFactory::Protocol protocol,

[Ryan Sleevi, 2016/04/04 22:34:04]

DESIGN NOTE (Future): As/if you change SocketPerformanceWatcher to be
TCPSocket::PerformanceWatcher (or delegate, whatever), I don't think it'll make
sense to need to track the |protocol| here, because
TCPSocket::PerformanceWatchers will always be Protocol::TCP. And if you have a
QuicConnection::PerformanceWatcher (which again, is really about whether or not
the TCPSocket and QUIC abstraction share the same pre/post conditions/api shape,
or whether they need distinct API shapes, such as to handle things like QUIC
connection migrations, so probably so?), the factory creating those wouldn't
need to track protocol either, because they'd always be QUIC watchers.

But you could still have both factories call NQE::OnUpdatedRTTAvailable - since
NQE only cares about the aggregated, and not per-destination IPs.

[tbansal1, 2016/04/04 23:46:32]

OK, I will write down a quick design note to see which approach makes sense.
Should we move the SPWF and SPW or not to TcpClientSocket. Or should we only
move SPW.  If we move both, then we need to pass 2 factories down the stack with
HttpNetworkSession::Params which might be wasteful given that the two factories
are not doing anything really different.

+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      const base::WeakPtr<NetworkQualityEstimator>& network_quality_estimator)
+      : protocol_(protocol),
+        task_runner_(std::move(task_runner)),
+        network_quality_estimator_(network_quality_estimator) {}
+
+  ~SocketWatcher() override {}
+
+  // SocketPerformanceWatcher implementation:
+  void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&NetworkQualityEstimator::OnUpdatedRTTAvailable,
+                              network_quality_estimator_, protocol_, rtt));
+  }
+
+  bool ShouldNotifyUpdatedRTT() const override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+
+    return true;
+  }
+
+  void Reset() override { DCHECK(thread_checker_.CalledOnValidThread()); }
+
+ private:
+  // Transport layer protocol used by the socket that |this| is watching.
+  const SocketPerformanceWatcherFactory::Protocol protocol_;

[bengr, 2016/04/07 00:20:05]

I agree with Ryan's design note above. 

[tbansal1, 2016/04/07 01:43:36]

I believe the eventual decision was to keep 1 factory interface, and 1 watcher
interface (see the design note that I shared on April 4 titled "SPW and SPWF").

+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::WeakPtr<NetworkQualityEstimator> network_quality_estimator_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(SocketWatcher);
+};
+
+// SocketWatcherFactory implements SocketPerformanceWatcherFactory, and is
+// owned by NetworkQualityEstimator. SocketWatcherFactory is thread safe.
+class NetworkQualityEstimator::SocketWatcherFactory
+    : public SocketPerformanceWatcherFactory {
+ public:
+  SocketWatcherFactory(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      const base::WeakPtr<NetworkQualityEstimator>& network_quality_estimator)
+      : task_runner_(std::move(task_runner)),
+        network_quality_estimator_(network_quality_estimator) {}
+
+  ~SocketWatcherFactory() override {}
+
+  // SocketPerformanceWatcherFactory implementation:
+  scoped_ptr<SocketPerformanceWatcher> CreateSocketPerformanceWatcher(
+      const Protocol protocol) override {
+    return scoped_ptr<SocketPerformanceWatcher>(
+        new SocketWatcher(protocol, task_runner_, network_quality_estimator_));
+  }
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::WeakPtr<NetworkQualityEstimator> network_quality_estimator_;
+
+  DISALLOW_COPY_AND_ASSIGN(SocketWatcherFactory);
+};
+
 const int32_t NetworkQualityEstimator::kInvalidThroughput = 0;
 
 NetworkQualityEstimator::NetworkQualityEstimator(
@@ -141,7 +216,8 @@ NetworkQualityEstimator::NetworkQualityEstimator(
       downstream_throughput_kbps_observations_(
           GetWeightMultiplierPerSecond(variation_params)),
       rtt_observations_(GetWeightMultiplierPerSecond(variation_params)),
-      external_estimate_provider_(std::move(external_estimates_provider)) {
+      external_estimate_provider_(std::move(external_estimates_provider)),
+      weak_ptr_factory_(this) {
   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
   static_assert(kDefaultHalfLifeSeconds > 0,
@@ -166,6 +242,9 @@ NetworkQualityEstimator::NetworkQualityEstimator(
   }
   current_network_id_ = GetCurrentNetworkID();
   AddDefaultEstimates();
+
+  watcher_factory_.reset(new SocketWatcherFactory(
+      base::ThreadTaskRunnerHandle::Get(), weak_ptr_factory_.GetWeakPtr()));
 }
 
 // static
@@ -376,6 +455,13 @@ void NetworkQualityEstimator::RemoveThroughputObserver(
   throughput_observer_list_.RemoveObserver(throughput_observer);
 }
 
+SocketPerformanceWatcherFactory*
+NetworkQualityEstimator::GetSocketPerformanceWatcherFactory() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  return watcher_factory_.get();
+}
+
 void NetworkQualityEstimator::RecordRTTUMA(int32_t estimated_value_msec,
                                            int32_t actual_value_msec) const {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -904,25 +990,16 @@ void NetworkQualityEstimator::CacheNetworkQualityEstimate() {
             static_cast<size_t>(kMaximumNetworkQualityCacheSize));
 }
 
-scoped_ptr<SocketPerformanceWatcher>
-NetworkQualityEstimator::CreateSocketPerformanceWatcher(
-    const Protocol protocol) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  return scoped_ptr<SocketPerformanceWatcher>(
-      new SocketPerformanceWatcher(protocol, this));
-}
-
 void NetworkQualityEstimator::OnUpdatedRTTAvailable(
-    const Protocol protocol,
+    SocketPerformanceWatcherFactory::Protocol protocol,
     const base::TimeDelta& rtt) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   switch (protocol) {
-    case PROTOCOL_TCP:
+    case SocketPerformanceWatcherFactory::PROTOCOL_TCP:
       NotifyObserversOfRTT(RttObservation(rtt, base::TimeTicks::Now(), TCP));
       return;
-    case PROTOCOL_QUIC:
+    case SocketPerformanceWatcherFactory::PROTOCOL_QUIC:
       NotifyObserversOfRTT(RttObservation(rtt, base::TimeTicks::Now(), QUIC));
       return;
     default: