Add SocketWatcherFactory as a helper class to NQE

net/base/network_quality_estimator.h

 // Copyright 2015 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.
 
 #ifndef NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_
 #define NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <deque>
 #include <map>
 #include <string>
 #include <tuple>
 
 #include "base/gtest_prod_util.h"
 #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"
 #include "net/base/external_estimate_provider.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 #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;
 
 // NetworkQualityEstimator provides network quality estimates (quality of the
 // full paths to all origins that have been connected to).
 // The estimates are based on the observed organic traffic.
 // A NetworkQualityEstimator instance is attached to URLRequestContexts and
 // observes the traffic of URLRequests spawned from the URLRequestContexts.
 // A single instance of NQE can be attached to multiple URLRequestContexts,
 // thereby increasing the single NQE instance's accuracy by providing more
 // 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
   // GENERATED_JAVA_CLASS_NAME_OVERRIDE: NetworkQualityObservationSource
   // GENERATED_JAVA_PREFIX_TO_STRIP:
   enum ObservationSource {
     // The observation was taken at the request layer, e.g., a round trip time
     // is recorded as the time between the request being sent and the first byte
     // being received.
     URL_REQUEST,
@@ skipping 106 matching lines @@
       base::TimeDelta* rtt) const;
 
   // Returns true if median downstream throughput is available and sets |kbps|
   // to the median of downstream throughput (in kilobits per second)
   // observations since |begin_timestamp|. Virtualized for testing. |kbps|
   // should not be null.
   virtual bool GetRecentMedianDownlinkThroughputKbps(
       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);
 
   // Removes |rtt_observer| from the list of round trip time observers if it
   // is on the list of observers. Must be called on the IO thread.
   void RemoveRTTObserver(RTTObserver* rtt_observer);
 
   // Adds |throughput_observer| to the list of throughput observers. Must be
   // called on the IO thread.
   void AddThroughputObserver(ThroughputObserver* throughput_observer);
 
   // Removes |throughput_observer| from the list of throughput observers if it
   // 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
   // uniquely identified by a combination of |type| and
   // |id|. This approach is unable to distinguish networks with
   // same name (e.g., different Wi-Fi networks with same SSID).
   // This is a protected member to expose it to tests.
   struct NET_EXPORT_PRIVATE NetworkID {
     NetworkID(NetworkChangeNotifier::ConnectionType type, const std::string& id)
         : type(type), id(id) {}
@@ skipping 233 matching lines @@
     // The factor by which the weight of an observation reduces every second.
     // For example, if an observation is 6 seconds old, its weight would be:
     //     weight_multiplier_per_second_ ^ 6
     // Calculated from |kHalfLifeSeconds| by solving the following equation:
     //     weight_multiplier_per_second_ ^ kHalfLifeSeconds = 0.5
     const double weight_multiplier_per_second_;
 
     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;
   typedef net::NetworkQualityEstimator::ObservationBuffer<base::TimeDelta>
       RttObservationBuffer;
 
   // Value of throughput observations is in kilobits per second.
   typedef net::NetworkQualityEstimator::Observation<int32_t>
       ThroughputObservation;
   typedef net::NetworkQualityEstimator::ObservationBuffer<int32_t>
@@ skipping 35 matching lines @@
 
   // Time duration (in milliseconds) after which the estimate provided by
   // external estimate provider is considered stale.
   static const int kExternalEstimateProviderFreshnessDurationMsec =
       5 * 60 * 1000;
 
   // Returns the RTT value to be used when the valid RTT is unavailable. Readers
   // 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();
 
   // Obtains operating parameters from the field trial parameters.
   void ObtainOperatingParams(
       const std::map<std::string, std::string>& variation_params);
 
   // Adds the default median RTT and downstream throughput estimate for the
   // current connection type to the observation buffer.
@@ skipping 17 matching lines @@
   // Virtualized for testing.
   virtual NetworkID GetCurrentNetworkID() const;
 
   // Writes the estimated quality of the current network to the cache.
   void CacheNetworkQualityEstimate();
 
   void NotifyObserversOfRTT(const RttObservation& observation);
 
   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;
 
   // Returns true only if |request| can be used for network quality estimation.
   // Only the requests that go over network are considered to provide useful
   // observations.
   bool RequestProvidesUsefulObservations(const URLRequest& request) const;
 
   // Values of external estimate provider status. This enum must remain
@@ skipping 55 matching lines @@
   NetworkQuality estimated_median_network_quality_;
 
   // ExternalEstimateProvider that provides network quality using operating
   // system APIs. May be NULL.
   const scoped_ptr<ExternalEstimateProvider> external_estimate_provider_;
 
   // Observer lists for round trip times and throughput measurements.
   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);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_