Add SocketWatcherFactory as a helper class to NQE

net/base/network_quality_estimator.h

Index: net/base/network_quality_estimator.h
diff --git a/net/base/network_quality_estimator.h b/net/base/network_quality_estimator.h
index 32e1a3ef8a8b41672fed1eba59c03f43d94dec1a..857348da9aee3314e53a2c93a0204ae6e8a1fbd1 100644
--- a/net/base/network_quality_estimator.h
+++ b/net/base/network_quality_estimator.h
@@ -17,6 +17,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
@@ -26,6 +27,10 @@
 #include "net/base/socket_performance_watcher.h"
 #include "net/base/socket_performance_watcher_factory.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
 namespace net {
 
 class URLRequest;
@@ -40,8 +45,7 @@ class URLRequest;
 // observed traffic characteristics.
 class NET_EXPORT_PRIVATE NetworkQualityEstimator
     : public NetworkChangeNotifier::ConnectionTypeObserver,
-      public ExternalEstimateProvider::UpdatedEstimateDelegate,
-      public SocketPerformanceWatcherFactory {
+      public ExternalEstimateProvider::UpdatedEstimateDelegate {
  public:
   // On Android, a Java counterpart will be generated for this enum.
   // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.net
@@ -168,12 +172,6 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
       const base::TimeTicks& begin_timestamp,
       int32_t* kbps) const;
 
-  // SocketPerformanceWatcherFactory implementation:
-  scoped_ptr<SocketPerformanceWatcher> CreateSocketPerformanceWatcher(
-      const Protocol protocol) override;
-  void OnUpdatedRTTAvailable(const Protocol protocol,
-                             const base::TimeDelta& rtt) override;
-
   // Adds |rtt_observer| to the list of round trip time observers. Must be
   // called on the IO thread.
   void AddRTTObserver(RTTObserver* rtt_observer);
@@ -190,6 +188,8 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // is on the list of observers. Must be called on the IO thread.
   void RemoveThroughputObserver(ThroughputObserver* throughput_observer);
 
+  SocketPerformanceWatcherFactory* GetSocketPerformanceWatcherFactory();
+
  protected:
   // NetworkID is used to uniquely identify a network.
   // For the purpose of network quality estimation and caching, a network is
@@ -443,6 +443,61 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
     DISALLOW_COPY_AND_ASSIGN(ObservationBuffer);
   };
 
+  // SocketWatcher implements SocketPerformanceWatcher, and notifies
+  // NetworkQualityEstimator of various socket performance events on the
+  // provided |task_runner|. SocketWatcher is not thread-safe.
+  class SocketWatcher : public SocketPerformanceWatcher {

[Ryan Sleevi, 2016/04/02 00:48:20]

Exposing this much implementation logic in your header is generally a design
smell, and the testing justification doesn't seem justified.

In general, you should prefer to keep as much as possible in the .cc, ideally in
the unnamed namespace.

This principle is captured in a variety of texts, but Lakos' "Large Scale C++
Software Design" is perhaps one of the seminal texts on the topic.

In this model, every time your header changes, everyone that depends on this
class must also change (well, in this case, be recompiled). The goal of good
software design is to minimize the number of dependencies that should be
recompiled when you make a change. Certainly, if you change an API contract,
there will be some dependent changes, but if you change an implementation
detail, your ideal result is that it would be isolated to your code.

With this principle in mind, the behaviours of both SocketWatcher and
SocketWatcherFactory are implementation details of the NQE. For testing, you
should not test implementation details - you test to the interface (in this
case, NQE), and the observable effects. You test the thing above NQE (aka the
NQE API contract), and to do that, you can mock the thing below NQE (e.g. the
socket classes)

[tbansal1, 2016/04/04 16:55:44]

Done. I could not move it to the anonymous namespace because SocketWatcher calls
NQE's private function but I removed most of implementation detail from the
header file.

+   public:
+    SocketWatcher(
+        SocketPerformanceWatcherFactory::Protocol protocol,
+        const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,

[Ryan Sleevi, 2016/04/02 00:48:20]

You should not be passing scoped_refptr<>'s as const.

Pass them by-val, and use std::move() to avoid the churn. 

If you wish to understand more,
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86...

[tbansal1, 2016/04/04 16:55:44]

Done.

+        const base::WeakPtr<NetworkQualityEstimator>&
+            network_quality_estimator);
+    ~SocketWatcher();
+
+    // SocketPerformanceWatcher implementation:
+    void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) override;
+    bool ShouldNotifyUpdatedRTT() const override;
+    void Reset() override;
+
+   private:
+    // Transport layer protocol used by the socket that |this| is watching.
+    const SocketPerformanceWatcherFactory::Protocol protocol_;
+
+    // TaskRunner on which |network_quality_estimator_| is notified.
+    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 NET_EXPORT_PRIVATE SocketWatcherFactory
+      : public SocketPerformanceWatcherFactory {
+   public:
+    SocketWatcherFactory(
+        const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,

[Ryan Sleevi, 2016/04/02 00:48:20]

pass & move.

[tbansal1, 2016/04/04 16:55:44]

Done.

+        const base::WeakPtr<NetworkQualityEstimator>&
+            network_quality_estimator);
+    ~SocketWatcherFactory() override;
+
+    // SocketPerformanceWatcherFactory implementation:
+    scoped_ptr<SocketPerformanceWatcher> CreateSocketPerformanceWatcher(
+        const Protocol protocol) override;
+
+   private:
+    // TaskRunner on which the |network_quality_estimator_| is notified.
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+    base::WeakPtr<NetworkQualityEstimator> network_quality_estimator_;
+
+    DISALLOW_COPY_AND_ASSIGN(SocketWatcherFactory);
+  };
+
   // Value of round trip time observations is in base::TimeDelta.
   typedef net::NetworkQualityEstimator::Observation<base::TimeDelta>
       RttObservation;
@@ -498,6 +553,10 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // should discard RTT if it is set to the value returned by |InvalidRTT()|.
   static const base::TimeDelta InvalidRTT();
 
+  // Notifies |this| of a new transport layer RTT.
+  void OnUpdatedRTTAvailable(SocketPerformanceWatcherFactory::Protocol protocol,
+                             const base::TimeDelta& rtt);
+
   // Queries the external estimate provider for the latest network quality
   // estimates, and adds those estimates to the current observation buffer.
   void QueryExternalEstimateProvider();
@@ -535,6 +594,8 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
 
   void NotifyObserversOfThroughput(const ThroughputObservation& observation);
 
+  base::WeakPtr<NetworkQualityEstimator> GetWeakPtr();

[Ryan Sleevi, 2016/04/02 00:48:20]

Don't expose this as a method; you have a weak_ptr_factory_, any time you need a
WeakPtr, you should be running in the context of NQE, and can give it out.

[tbansal1, 2016/04/04 16:55:44]

Done. I also realized that WatcherFactory was making copies of NQE's weak
pointer, which is probably not the best approach. So, I also added
SocketWatcherDelegate class. This helps me avoid making copies of NQE's weak
pointer outside NQE.

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

I apologize that this wasn't clear, although I tried to capture it repeatedly -
that's perfectly fine IF the WatcherFactory is an internal implementation detail
of NQE.

That is, it is perfectly fine to think of a member class as merely an
implementation detail of the main thing. So if NQE can have access to a WeakPtr,
its (*private*) member classes can as well - it only becomes an issue if
"someone else" (code outside the header / .cc) gets access to the WeakPtr
directly.

So now it sounds like you may have too much indirection?

+
   // Records the UMA related to RTT.
   void RecordRTTUMA(int32_t estimated_value_msec,
                     int32_t actual_value_msec) const;
@@ -610,8 +671,12 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   base::ObserverList<RTTObserver> rtt_observer_list_;
   base::ObserverList<ThroughputObserver> throughput_observer_list_;
 
+  scoped_ptr<SocketPerformanceWatcherFactory> watcher_factory_;
+
   base::ThreadChecker thread_checker_;
 
+  base::WeakPtrFactory<NetworkQualityEstimator> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkQualityEstimator);
 };