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..60327543e37a3e39d7df4d26665c34ec3f8f4afa 100644
--- a/blimp/net/engine_connection_manager.h
+++ b/blimp/net/engine_connection_manager.h
@@ -12,6 +12,10 @@
 #include "base/macros.h"
 #include "blimp/net/blimp_net_export.h"
 #include "blimp/net/connection_handler.h"
+#include "net/base/ip_address.h"

[Garrett Casto, 2016/10/25 18:33:50]

Doesn't look like this is used.

[perumaal, 2016/10/25 23:02:39]

How do I check for unnecessary includes using git cl?

+#include "net/base/ip_endpoint.h"
+#include "net/base/net_errors.h"

[Garrett Casto, 2016/10/25 18:33:50]

Or this.

[perumaal, 2016/10/25 23:02:39]

Done.

+#include "net/log/net_log.h"
 
 namespace blimp {
 
@@ -24,26 +28,28 @@ class BlimpTransport;
 // TODO(kmarshall): Add rate limiting and abuse handling logic.
 class BLIMP_NET_EXPORT EngineConnectionManager {
  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);
+  void ConnectTransport(const net::IPEndPoint& ip_endpoint,

[Kevin M, 2016/10/25 18:49:23]

Suggestion: just take a std::unique_ptr<BlimpTransport> supplied by the caller
rather than using an enum. The lookup is unnecessary and it's a natural
interface for introducing loose coupling.

[perumaal, 2016/10/25 23:02:39]

I would really like the EngineConnectionManager to be the factory. This
decouples the callers from having to worry about the specifics of tcp vs grpc or
duplicate this logic.

Also, please note that this is going away once gRPC is settled (and the sole
connection facility).

+                        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<BlimpTransport> transport_;
 
   DISALLOW_COPY_AND_ASSIGN(EngineConnectionManager);
 };