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..065ba81b71e3da04f364d9861507b234d0b76bae 100644
--- a/net/base/socket_performance_watcher.h
+++ b/net/base/socket_performance_watcher.h
@@ -5,9 +5,7 @@
 #ifndef NET_BASE_SOCKET_PERFORMANCE_WATCHER_H_
 #define NET_BASE_SOCKET_PERFORMANCE_WATCHER_H_
 
-#include "base/macros.h"
 #include "net/base/net_export.h"
-#include "net/base/socket_performance_watcher_factory.h"
 
 namespace base {
 class TimeDelta;
@@ -16,32 +14,28 @@ class TimeDelta;
 namespace net {
 
 // SocketPerformanceWatcher is the base class for recording and aggregating
-// socket statistics.
+// per-socket statistics. SocketPerformanceWatcher must be used on a single
+// thread.
 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);
-
-  virtual ~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;
-
- 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_;
-
-  DISALLOW_COPY_AND_ASSIGN(SocketPerformanceWatcher);
+  virtual ~SocketPerformanceWatcher() {}
+
+  // Notifies |this| SocketPerformanceWatcher of updated 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.
+  // There is no guarantee that OnUpdatedRTTAvailable will be called every time
+  // an updated RTT is available as the socket may throttle

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

throttle -> throttle the

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

Done.

+  // OnUpdatedRTTAvailable call for various reasons (including performance).

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

reasons (including performance). -> reasons, including performance.

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

Done.

+  virtual void OnUpdatedRTTAvailable(const base::TimeDelta& rtt) = 0;
+
+  // Returns true if |this| SocketPerformanceWatcher is interested in receiving

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

Should this be the first method, logically, I mean?

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

Done.

+  // an updated RTT estimate (via OnUpdatedRTTAvailable).
+  virtual bool ShouldNotifyUpdatedRTT() const = 0;

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

This is an optimization. The question being asked is should I bother to
construct and RTT value for you right now? This seems useful for sockets where
there's some effort to construct the RTT, e.g., a syscall for TCP. Might we want
to have a default implementation that says "yes!"

Also, since NQE says yes every time, do you think it is premature to add this
method?

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

No. A pure interface should be pure. Then it's no longer an interface, it's a
base-class, and you introduce the admonitions against multiple inheritance.
I do

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

I think I want to keep ShouldNotifyUpdatedRTT. NQE would soon not return "true"
in all cases. crbug.com/590300

+
+  // Notifies that |this| watcher will be reused to watch a different socket.
+  // Note: The new socket shares the same protocol as the previously watched
+  // socket.
+  virtual void Reset() = 0;

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

Should this be called OnEndpointChanged()?

Say that there is no way to throttle this. I.e., there is no
ShouldNotifyEnpointChanged(). Say also that this indicates that the source or
destination changed, and give examples of when that happens.

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

That seems a premature optimization, and only relevant to the QUIC multipath
case. From discussion with rch@, we should defer such optimizations unless and
until such support is introduced into Chrome/Cronet (there's some questions
surrounding that and how that'll work)

So it'll be a refactoring question for then, the answer is "maybe", but not
enough information is available now to inform that.

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

Renamed (although to a different name than what you suggested).
Hopefully, with this new name, examples are not required.
In the very near future, this is going to be used for TCP sockets where a
TcpClientSocket uses the same POSIX socket to connect to different IP addresses.
In that case, TcpClientSocket will call OnConnectionChanged() before it uses the
same socket to connect to a different IP address (although I believe there is
open net bug to fix that crbug.com/499289).

[Ryan Sleevi, 2016/04/07 01:51:27]

Correct; we really should be removing that (patches welcome for cleanup
assistance! Makes your life easier! :P)

 };
 
 }  // namespace net