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..a2ae89e302e7759e84ab564f17835846e85bbc91 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,103 @@ base::HistogramBase* GetHistogram(
 
 namespace net {
 
+// SocketWatcherDelegate receives socket performance events from different
+// SocketWatchers, and passes them to the NetworkQualityEstimator on the
+// provided task runner. SocketWatcherDelegate is thread safe.
+class NetworkQualityEstimator::SocketWatcherDelegate {

[Ryan Sleevi, 2016/04/04 17:30:09]

Yeah, I don't think you should need this abstraction (for the reason explained
in the header). It should be fine to have the NQE::SW directly use/call into NQE
via WeakPtr.

[tbansal1, 2016/04/04 18:35:46]

Is it okay if I keep this? The delegate interface looks cleaner.

[Ryan Sleevi, 2016/04/04 18:53:37]

Can you explain why you feel that way? I'd like to understand to make sure I'm
not missing something.

The downside is you're adding an additional level of indirection, except that
indirection is only relevant to your implementation (this .cc), so it doesn't by
you any abstractions or decoupling.

It's sort of like having a

void A() {
  PostTask(B())
}

void B() {
  // stuff
}


void DoSomething() {
  A();
}


That seems like it makes it harder to read (now you have to read A to know that
it'll call B)

Alternatively, it could be rewritten as

void B() {
  // Do stuff;
}

void DoSomething(
  PostTask(B());
}

Which seems slightly clearer.

if there was more logic between the two, I agree, the abstraction might be
helpful - but it just seems to make it harder to know "what will happen" in the
NQE::SocketWatcher here

[tbansal1, 2016/04/04 22:07:03]

I have removed the SocketDelegate. I just thought it was probably incorrect if
SocketFactory was creating copies of NQE's weak pointer. 

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

As long as the copying of NQE's WeakPtr only happens in
network_quality_estimator.cc, that's generally OK.

That's generally how/where you should use a WeakPtr - that is, it should never
'escape' the implementation details. Having it passed/copied between multiple
classes is fine, so long as those classes all live in the same .cc file, and
there's no "public" way (e.g. in the headers via public/protected methods) to
get access to those weak ptrs.

Does that help clarify?

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

Yes. Thanks for the detailed reply.

+ public:
+  SocketWatcherDelegate(
+      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) {
+    DCHECK(task_runner_);
+  }
+
+  ~SocketWatcherDelegate() {}
+
+  void OnUpdatedRTTAvailable(SocketPerformanceWatcherFactory::Protocol protocol,
+                             const base::TimeDelta& rtt) {
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&NetworkQualityEstimator::OnUpdatedRTTAvailable,
+                              network_quality_estimator_, protocol, rtt));
+  }
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  base::WeakPtr<NetworkQualityEstimator> network_quality_estimator_;
+
+  DISALLOW_COPY_AND_ASSIGN(SocketWatcherDelegate);
+};
+
+// SocketWatcher implements SocketPerformanceWatcher, and notifies
+// SocketWatcherDelegate of various socket performance events. SocketWatcher is
+// not thread-safe.
+class NetworkQualityEstimator::SocketWatcher : public SocketPerformanceWatcher {
+ public:
+  //  |delegate| is guaranteed to be non-null during SocketWatcher's lifetime.
+  SocketWatcher(SocketPerformanceWatcherFactory::Protocol protocol,
+                SocketWatcherDelegate* delegate)
+      : protocol_(protocol), delegate_(delegate) {
+    DCHECK(delegate_);
+  }
+
+  ~SocketWatcher() {}
+
+  // SocketPerformanceWatcher implementation:
+  void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+
+    delegate_->OnUpdatedRTTAvailable(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_;
+
+  SocketWatcherDelegate* delegate_;
+
+  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:
+  //  |delegate| is guaranteed to be non-null during SocketWatcherFactory's
+  //  lifetime.
+  explicit SocketWatcherFactory(SocketWatcherDelegate* delegate)
+      : delegate_(delegate) {
+    DCHECK(delegate_);
+  }
+
+  ~SocketWatcherFactory() override {}
+
+  // SocketPerformanceWatcherFactory implementation:
+  scoped_ptr<SocketPerformanceWatcher> CreateSocketPerformanceWatcher(
+      const Protocol protocol) override {
+    return scoped_ptr<SocketPerformanceWatcher>(
+        new SocketWatcher(protocol, delegate_));
+  }
+
+ private:
+  SocketWatcherDelegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(SocketWatcherFactory);
+};
+
 const int32_t NetworkQualityEstimator::kInvalidThroughput = 0;
 
 NetworkQualityEstimator::NetworkQualityEstimator(
@@ -141,7 +240,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 +266,11 @@ NetworkQualityEstimator::NetworkQualityEstimator(
   }
   current_network_id_ = GetCurrentNetworkID();
   AddDefaultEstimates();
+
+  delegate_.reset(new SocketWatcherDelegate(base::ThreadTaskRunnerHandle::Get(),
+                                            weak_ptr_factory_.GetWeakPtr()));
+
+  watcher_factory_.reset(new SocketWatcherFactory(delegate_.get()));
 }
 
 // static
@@ -376,6 +481,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 +1016,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: