Refactor BlimpConnection to TCPConnection

blimp/net/engine_connection_manager.h

Index: blimp/net/engine_connection_manager.h
diff --git a/blimp/net/engine_connection_manager.h b/blimp/net/engine_connection_manager.h
index 9fe04168233d9e7cb27d40f56086ce0eefe44e84..c75fbca7c68d84d8b7bbd433bbd90313fe3cf83f 100644
--- a/blimp/net/engine_connection_manager.h
+++ b/blimp/net/engine_connection_manager.h
@@ -12,11 +12,14 @@
 #include "base/macros.h"
 #include "blimp/net/blimp_net_export.h"
 #include "blimp/net/connection_handler.h"
+#include "net/base/ip_endpoint.h"
+#include "net/log/net_log.h"
 
 namespace blimp {
 
 class BlimpConnection;
 class BlimpTransport;
+class BlimpEngineTransport;
 
 // Coordinates the channel creation and authentication workflows for
 // incoming (Engine) network connections.
@@ -24,26 +27,29 @@ class BlimpTransport;
 // TODO(kmarshall): Add rate limiting and abuse handling logic.
 class BLIMP_NET_EXPORT EngineConnectionManager {

[Wez, 2016/11/09 22:47:17]

This class, as per the comment, was intended to be Transport-agnostic, purely
handling the task of accepting connections, rate limiting etc, and handling them
off to the ConnectionHandler.

Rather than folding the Transport-creation logic into it, I'd prefer a separate
EngineTransport : public Transport implementation that creates the relevant (TCP
or GRPC) Transport under the hood, and which can be passed to
EngineConnectionManager to work with.

(Though depending on how the gRPC code looks, EngineConnectionmanager may of
course go away)

[perumaal, 2016/11/10 02:05:05]

Kevin and I talked about this specifically. One could imagine a factory class to
provide the specific implementation here.
We agreed that this class is going away in the HeliumStream world and it was not
worth it. It also mimics the existing code (and in client) to contain the
creation in one place.

Thoughts? I can add an explicit factory class if you'd prefer and want this
EngineConnectionManager to stay.

  public:
+  enum class EngineTransportType { TCP, GRPC };
+
   // Caller is responsible for ensuring that |connection_handler| outlives
   // |this|.
-  explicit EngineConnectionManager(ConnectionHandler* connection_handler);
+  explicit EngineConnectionManager(ConnectionHandler* connection_handler,
+                                   net::NetLog* net_log);
 
   ~EngineConnectionManager();
 
   // Adds a transport and accepts new BlimpConnections from it as fast as they
-  // arrive.
-  void AddTransport(std::unique_ptr<BlimpTransport> transport);
+  // arrive. The |ip_endpoint| will receive the actual port-number if the
+  // provided one is not available for listening.
+  void ConnectTransport(net::IPEndPoint* ip_endpoint,

[Wez, 2016/11/09 22:47:17]

Let's not have in/out parameters.  We can add a getter that pulls the address
information from |transport_| instead.

[perumaal, 2016/11/10 02:05:05]

I had it that way actually initially. The problem is that the GetLocalAddress
can only be called *after* the connection is made. Which is at an unknown point
in time because it could be after the ConnectTransport or after the
OnConnectResult. So I made it explicit (and also less 'chatty' as you don't need
a tightly coupled sequence of two API calls).

+                        EngineTransportType transport_type);
 
  private:
-  // Invokes transport->Connect to listen for a connection.
-  void Connect(BlimpTransport* transport);
-
   // Callback invoked by |transport| to indicate that it has a connection
   // ready to be authenticated.
   void OnConnectResult(BlimpTransport* transport, int result);
 
   ConnectionHandler* connection_handler_;
-  std::vector<std::unique_ptr<BlimpTransport>> transports_;
+  net::NetLog* net_log_;
+  std::unique_ptr<BlimpEngineTransport> transport_;
 
   DISALLOW_COPY_AND_ASSIGN(EngineConnectionManager);
 };