Create WebSocketTransportClientSocketPool

net/socket/transport_client_socket_pool.h

Index: net/socket/transport_client_socket_pool.h
diff --git a/net/socket/transport_client_socket_pool.h b/net/socket/transport_client_socket_pool.h
index 1c22bf29ec3776886ddb0e3a5d71084819f34477..003008a372ddfa1a20f4b566a583ed545b3e5f5d 100644
--- a/net/socket/transport_client_socket_pool.h
+++ b/net/socket/transport_client_socket_pool.h
@@ -55,6 +55,75 @@ class NET_EXPORT_PRIVATE TransportSocketParams
   DISALLOW_COPY_AND_ASSIGN(TransportSocketParams);
 };
 
+// Common data and logic shared between TransportConnectJob and
+// WebSocketTransportConnectJob.
+class NET_EXPORT_PRIVATE TransportConnectJobHelper {
+ public:
+  enum State {
+    STATE_RESOLVE_HOST,
+    STATE_RESOLVE_HOST_COMPLETE,
+    STATE_TRANSPORT_CONNECT,
+    STATE_TRANSPORT_CONNECT_COMPLETE,
+    STATE_NONE,
+  };
+
+  // For recording the connection time in the appropriate bucket.
+  enum ConnectionLatencyHistogram {
+    CONNECTION_LATENCY_UNKNOWN,
+    CONNECTION_LATENCY_IPV4_WINS_RACE,
+    CONNECTION_LATENCY_IPV4_NO_RACE,
+    CONNECTION_LATENCY_IPV6_RACEABLE,
+    CONNECTION_LATENCY_IPV6_SOLO,
+  };
+
+  TransportConnectJobHelper(const scoped_refptr<TransportSocketParams>& params,
+                            ClientSocketFactory* client_socket_factory,
+                            HostResolver* host_resolver,
+                            LoadTimingInfo::ConnectTiming* connect_timing);
+  ~TransportConnectJobHelper();
+
+  ClientSocketFactory* client_socket_factory() {
+    return client_socket_factory_;
+  }
+

[Ryan Sleevi, 2014/07/08 18:50:02]

This is a huge set of methods to leave public, for any #include'r of
TransportClientSocketPool.

This seems to break multiple layers of encapsulation/isolation - e.g. - these
details are not exposed to holders of TransportConnectJobs normally.

[tyoshino (SeeGerritForStatus), 2014/07/09 00:17:40]

Sorry about this. I saw use of ClientSocketPoolBaseHelper and was taking things
easy too much.

Basically they're for reducing code duplication. We should be able to put them
into each pool implementation as a quick fix while we should also try to come up
with better design with less code duplication.

Also, shall we put this into an internal namespace as well as
ClientSocketPoolBaseHelper?

[Ryan Sleevi, 2014/07/09 00:20:46]

Right, CSPBH is to be a base class for a set of templated classes, but avoid the
overhead of instantiating all of the common methods on the templates.

That is, it's a performance optimization, more than a code/dependency
optimization.
Right, I think we're on the same page that we want to minimize code duplication
(through template instantiation) and code duplication (through
implementation/copy paste). We just want to make sure none of that
de-duplication leaks out into the interfaces, nor does it violate the layering.
Yes, this is something we'd minimally mark internal

+  const AddressList& addresses() const { return addresses_; }
+  State next_state() const { return next_state_; }
+  void set_next_state(State next_state) { next_state_ = next_state; }
+  CompletionCallback on_io_complete() const { return on_io_complete_; }
+
+  int DoResolveHost(RequestPriority priority, const BoundNetLog& net_log);
+  int DoResolveHostComplete(int result, const BoundNetLog& net_log);
+
+  template <class T>
+  int DoConnectInternal(T* job);

[Ryan Sleevi, 2014/07/08 18:50:02]

These methods are public.

Anyone with a helper can use it to fully mutate the state of an Job. That's
definitely a layering violation.

[Adam Rice, 2014/07/09 08:33:02]

Sorry, yes, when I wrote this I was just focussed on factoring out the common
code in the simplest way possible. I was expecting to get some push-back that
would ultimately lead to a better solution, so I wasn't paying attention to
encapsulation.

But it's easy to miss things in a big CL, especially one with parts that are
ugly-as-designed, so this ended-up getting checked in as-is.

+
+  template <class T>
+  void SetOnIOComplete(T* job);
+
+  template <class T>
+  void OnIOComplete(T* job, int result);

[Ryan Sleevi, 2014/07/08 18:50:02]

Template methods like this are almost always a bad thing - they accept any type
and can be used to mutate the internal state.

+
+  // Record the histograms Net.DNS_Resolution_And_TCP_Connection_Latency2 and
+  // Net.TCP_Connection_Latency and return the connect duration.
+  base::TimeDelta HistogramDuration(ConnectionLatencyHistogram race_result);
+
+  static const int kIPv6FallbackTimerInMs;
+
+ private:
+  template <class T>
+  int DoLoop(T* job, int result);
+
+  scoped_refptr<TransportSocketParams> params_;
+  ClientSocketFactory* const client_socket_factory_;
+  SingleRequestHostResolver resolver_;
+  AddressList addresses_;
+  State next_state_;
+  CompletionCallback on_io_complete_;
+  LoadTimingInfo::ConnectTiming* connect_timing_;
+
+  DISALLOW_COPY_AND_ASSIGN(TransportConnectJobHelper);
+};
+
 // TransportConnectJob handles the host resolution necessary for socket creation
 // and the transport (likely TCP) connect. TransportConnectJob also has fallback
 // logic for IPv6 connect() timeouts (which may happen due to networks / routers
@@ -82,27 +151,14 @@ class NET_EXPORT_PRIVATE TransportConnectJob : public ConnectJob {
   // WARNING: this method should only be used to implement the prefer-IPv4 hack.
   static void MakeAddressListStartWithIPv4(AddressList* addrlist);
 
-  static const int kIPv6FallbackTimerInMs;
-
  private:
-  enum State {
-    STATE_RESOLVE_HOST,
-    STATE_RESOLVE_HOST_COMPLETE,
-    STATE_TRANSPORT_CONNECT,
-    STATE_TRANSPORT_CONNECT_COMPLETE,
-    STATE_NONE,
-  };
-
   enum ConnectInterval {
     CONNECT_INTERVAL_LE_10MS,
     CONNECT_INTERVAL_LE_20MS,
     CONNECT_INTERVAL_GT_20MS,
   };
 
-  void OnIOComplete(int result);
-
-  // Runs the state transition loop.
-  int DoLoop(int result);
+  friend class TransportConnectJobHelper;

[Ryan Sleevi, 2014/07/08 18:50:02]

Every time we have a friend class, we have failed encapsulation, and it should
only be done with great pain/suffering/introspection.

I'd like to work to explore alternate designs that actually maintain a
well-encapsulated boundary.

 
   int DoResolveHost();
   int DoResolveHostComplete(int result);
@@ -118,11 +174,7 @@ class NET_EXPORT_PRIVATE TransportConnectJob : public ConnectJob {
   // Otherwise, it returns a net error code.
   virtual int ConnectInternal() OVERRIDE;
 
-  scoped_refptr<TransportSocketParams> params_;
-  ClientSocketFactory* const client_socket_factory_;
-  SingleRequestHostResolver resolver_;
-  AddressList addresses_;
-  State next_state_;
+  TransportConnectJobHelper helper_;
 
   scoped_ptr<StreamSocket> transport_socket_;
 
@@ -187,6 +239,12 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool : public ClientSocketPool {
   virtual void AddHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
   virtual void RemoveHigherLayeredPool(HigherLayeredPool* higher_pool) OVERRIDE;
 
+ protected:

[Ryan Sleevi, 2014/07/08 18:50:02]

The Chromium style guide strongly discourages protected methods 

+  // Methods shared with WebSocketTransportClientSocketPool
+  void NetLogTcpClientSocketPoolRequestedSocket(
+      const BoundNetLog& net_log,
+      const scoped_refptr<TransportSocketParams>* casted_params);

[Ryan Sleevi, 2014/07/08 18:50:02]

This doesn't look right at all. A pointer to a const scoped-refptr?

[Adam Rice, 2014/07/09 08:33:02]

Whereas the void* argument to RequestSocket() is totally cool?

+
  private:
   typedef ClientSocketPoolBase<TransportSocketParams> PoolBase;
 
@@ -224,6 +282,61 @@ class NET_EXPORT_PRIVATE TransportClientSocketPool : public ClientSocketPool {
   DISALLOW_COPY_AND_ASSIGN(TransportClientSocketPool);
 };
 
+template <class T>
+int TransportConnectJobHelper::DoConnectInternal(T* job) {
+  next_state_ = STATE_RESOLVE_HOST;
+  return this->DoLoop(job, OK);
+}
+
+template <class T>
+void TransportConnectJobHelper::SetOnIOComplete(T* job) {
+  // These usages of base::Unretained() are safe because IO callbacks are
+  // guaranteed not to be called after the object is destroyed.
+  on_io_complete_ = base::Bind(&TransportConnectJobHelper::OnIOComplete<T>,
+                               base::Unretained(this),
+                               base::Unretained(job));
+}
+
+template <class T>
+void TransportConnectJobHelper::OnIOComplete(T* job, int result) {
+  result = this->DoLoop(job, result);
+  if (result != ERR_IO_PENDING)
+    job->NotifyDelegateOfCompletion(result);  // Deletes |job| and |this|
+}
+
+template <class T>
+int TransportConnectJobHelper::DoLoop(T* job, int result) {
+  DCHECK_NE(next_state_, STATE_NONE);
+
+  int rv = result;
+  do {
+    State state = next_state_;
+    next_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_RESOLVE_HOST:
+        DCHECK_EQ(OK, rv);
+        rv = job->DoResolveHost();
+        break;
+      case STATE_RESOLVE_HOST_COMPLETE:
+        rv = job->DoResolveHostComplete(rv);
+        break;
+      case STATE_TRANSPORT_CONNECT:
+        DCHECK_EQ(OK, rv);
+        rv = job->DoTransportConnect();
+        break;
+      case STATE_TRANSPORT_CONNECT_COMPLETE:
+        rv = job->DoTransportConnectComplete(rv);
+        break;
+      default:
+        NOTREACHED();
+        rv = ERR_FAILED;
+        break;
+    }
+  } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
+
+  return rv;
+}
+
 }  // namespace net
 
 #endif  // NET_SOCKET_TRANSPORT_CLIENT_SOCKET_POOL_H_