Add SocketWatcherFactory as a helper class to NQE

net/base/socket_performance_watcher.h

Index: net/base/socket_performance_watcher.h
diff --git a/net/base/socket_performance_watcher.h b/net/base/socket_performance_watcher.h
index e0e76327ce63541e8d0dc2ba23215d34f953df41..892f2c02ff0e0cc55cfc7f9f76ac7c3816b3922f 100644
--- a/net/base/socket_performance_watcher.h
+++ b/net/base/socket_performance_watcher.h
@@ -5,7 +5,12 @@
 #ifndef NET_BASE_SOCKET_PERFORMANCE_WATCHER_H_
 #define NET_BASE_SOCKET_PERFORMANCE_WATCHER_H_
 
+#include <stddef.h>
+
+#include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/threading/thread_checker.h"
 #include "net/base/net_export.h"
 #include "net/base/socket_performance_watcher_factory.h"
 
@@ -19,27 +24,51 @@ namespace net {
 // socket statistics.
 class NET_EXPORT_PRIVATE SocketPerformanceWatcher {
  public:
-  // |socket_performance_watcher_factory| is the factory that constructed
-  // |this| watcher.
-  SocketPerformanceWatcher(
-      const SocketPerformanceWatcherFactory::Protocol protocol,
-      SocketPerformanceWatcherFactory* socket_performance_watcher_factory);
+  typedef base::Callback<void(
+      SocketPerformanceWatcherFactory::Protocol protocol,
+      const base::TimeDelta& rtt)>
+      RTTCallback;
+  typedef base::Callback<void()> ResetCallback;
 
-  virtual ~SocketPerformanceWatcher();
+  // |protocol| is the transport layer protocol of the socket that |this|
+  // SocketPerformanceWatcher is going to watch. |rtt_callback| is invoked when
+  // an updated RTT value is available. |reset_callback| is invoked when the
+  // socket being watched resets.
+  SocketPerformanceWatcher(SocketPerformanceWatcherFactory::Protocol protocol,
+                           const RTTCallback& rtt_callback,
+                           const ResetCallback& reset_callback);

[Ryan Sleevi, 2016/04/01 01:38:55]

When determining between delegates and callbacks, it's important to consider the
lifetime and relationship between things, as well as things like optionality.

For example, is it ever valid to specify an rtt_callback, but an empty callback
for reset? (since it's, effectively, a base::Closure()) ? Why or why not?

Also think about use - anyone who is interested in one is interested in the
other, right?

What about in deployment - should it be valid to 'give up' recording midway?

There are pros and cons to when to use Callbacks vs a Delegate, and really they
relate to how someone will use it, hold it, what's the necessary functionality
that an implementor should implement, and what are the sort of guarantees you
expect the SPW to have.

There's a more subtle 'problem' with callbacks, which is not necessarily an
intrinsic problem, but can hide codesmells. That's because callbacks are, under
the hood, ref-counted, and so it can easily become tempting to use callbacks to
hide the need to address and explain lifetime issues.

Further, callbacks can be used to hide cyclic dependencies, in coupling
interfaces such that any change in base class A requires that you change
consumer B. Since we're largely talking about SOLID design patterns, one of the
goals of the dependency inversion at work here is to isolate A from B, by using
a mechanism glue layer. B only has to change when the mechanism glue changes,
and largely, A can change w/o needing to change the mechanism glue. Callbacks
can break that very easily.

Finally, one last thing to consider when deciding on callbacks is whether this
is a one-off message or a repeated message. A callback can be totally
appropriate for an asynchronous completion, but if you're talking about ongoing
monitoring and interaction, a delegate is generally the right approach.


For these reasons, and perhaps more importantly, for overall consistency with
//net (which largely uses delegates), I'd like to steer you in that direction to
solve this problem. It also encourages a more explicitly encapsulated layering
of interfaces.

My concrete suggestion is

class SocketPerformanceWatcher {
 public:
  class Delegate {
   public:
    virtual ~Delegate() {};

    // Alternatively, these could be dummy methods of a base
    // (aka {} vs =0), rather than pure virtual, but that requires
    // thinking carefully about how these events related.
    virtual void OnRTTUpdate(protocol, rtt) = 0;
    virtual void OnReset() = 0;
  };

  SocketPerformanceWatcher(protocol,
                           scoped_ptr<Delegate> delegate);
};

Now, when writing out that sketch of an interface, it's easier to question now -
what logic is the Delegate providing vs the SocketPerformanceWatcher? That is,
if SPW just forwards all events to a Delegate, what's the value being added here
- what's the single responsibility (in those SOLID patterns) of SPW?

From just the header interface, it's not clear. The only thing 'interesting'
here is the ShouldNotifyUpdatedRTT(), it would seem. If that's the case, it
prompts a second level design question of - is the SPW (conceptually) simply the
Delegate interface for the socket?

That is, do we want our layering to be

Pool -> SPWF -> Socket -> SPW[pure interface]

Sorry that code review is hard to draw out this as a graph, but I find it often
helps when thinking about this design flow - what class Depends-On this
behaviour?

Certainly, the Socket needs a way to either push RTT (via the SPW) or provide a
means to pull RTT (which does not seem desirable at all, for perf/power). So
that means that Socket Depends-On SPW (also evidenced by taking an SPW)

So should we instead make SPW a pure-virtual interface (conceptually, it's just
Socket::PerformanceDelegate then), which Socket can safely depend on, and then,
up at the SPWF/SocketPool layer, create a thing which creates *instances of* the
SPW - for example, NQE::SocketPerformanceDelegate, that actually holds the meat
of the logic.


I realize this is a very LONG comment, but hopefully it explains some of the
design concerns as we work to break this dependency.

+  ~SocketPerformanceWatcher();
 
   // Called when updated transport layer RTT information is available. This
   // must be the transport layer RTT from this device to the remote transport
   // layer endpoint. This method is called immediately after the observation is
   // made, hence no timestamp.
-  void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) const;
+  void OnUpdatedRTTAvailable(const base::TimeDelta& rtt);
+
+  // Returns |true| if the current SocketPerformanceWatcher is interested
+  // in receiving a new RTT estimate (via OnUpdatedRTTAvailable).
+  // Callers may use this to avoid doing expensive work computing the
+  // RTT when the SocketPerformanceWatcher is not interested in such
+  // updates.
+  bool ShouldNotifyUpdatedRTT() const WARN_UNUSED_RESULT;
+
+  // Resets the internal state of this SocketPerformanceWatcher in preparation
+  // for observing a new socket.
+  // Note: The new socket must share the same protocol as the previously
+  // observed socket.
+  void Reset();
 
  private:
   // Transport layer protocol used by the socket that |this| is watching.
   const SocketPerformanceWatcherFactory::Protocol protocol_;
 
-  // |socket_performance_watcher_factory_| is the factory that created
-  // |this| watcher.
-  SocketPerformanceWatcherFactory* socket_performance_watcher_factory_;
+  // Number of RTT notifications received.
+  size_t rtt_notification_received_count_;
+
+  RTTCallback rtt_callback_;
+  ResetCallback reset_callback_;
+
+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(SocketPerformanceWatcher);
 };