Refactor BlimpConnection to TCPConnection

blimp/engine/session/blimp_engine_session.cc

Index: blimp/engine/session/blimp_engine_session.cc
diff --git a/blimp/engine/session/blimp_engine_session.cc b/blimp/engine/session/blimp_engine_session.cc
index f4708eaa662112f412ca279c403249b77864a06e..e91e29de90b96c4650c57cbe467eeefbfec746d5 100644
--- a/blimp/engine/session/blimp_engine_session.cc
+++ b/blimp/engine/session/blimp_engine_session.cc
@@ -99,6 +99,21 @@ base::Closure QuitCurrentMessageLoopClosure() {
                     base::MessageLoop::QuitWhenIdleClosure());
 }
 
+EngineConnectionManager::EngineTransportType GetTransportType() {
+  const std::string kTcpTransport = "tcp";

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

Move these into the anonymous namespace and use const char[]

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

Done.

+  const std::string kGrpcTransport = "grpc";
+  const auto transport_parsed =

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

std::string

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

Done.

+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          kEngineTransport);
+  if (transport_parsed == kTcpTransport || transport_parsed.empty()) {
+    return EngineConnectionManager::EngineTransportType::TCP;
+  } else if (transport_parsed == kGrpcTransport) {
+    return EngineConnectionManager::EngineTransportType::GRPC;
+  }
+  LOG(FATAL) << "--engine-transport must either be empty or one of tcp, grpc.";

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

Log the flag names using constants, since we got 'em :)

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

Done.

+  return EngineConnectionManager::EngineTransportType::TCP;
+}
+
 net::IPAddress GetListeningAddress() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(kAllowNonLocalhost)) {
     return net::IPAddress::IPv4AllZeros();
@@ -139,7 +154,7 @@ class EngineNetworkComponents : public ConnectionHandler,
                   base::WeakPtr<BlobChannelSender> blob_channel_sender,
                   const std::string& client_token);
 
-  uint16_t GetPortForTesting() { return port_; }
+  uint16_t GetPortForTesting() { return GetListeningPort(); }

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

I'm surprised that this doesn't break the browser_test. That test passes in "0"
to pick random port for testing, and then it tries to get the actual used port
back through this function. It seems like it should get 0 back, and then fail
when trying to connect the client to the engine. Is that not what happens?

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

Why make this change? For the e2e tests, this method is used to get the
ephemeral port number, which is needed for initiating e2e connections.

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

I wanted to minimize the usage of port numbers as there would not be a port
number in the actual gRPC implementation (as it uses file-descriptors). I will
fix the browser_test separately and reference the gRPC bug instead for now.

Ideally I would like the EngineTransport to provide some 'opaque options' that
the test can directly pass to a suitable client - so that it does not have to
know the implementation details of IP address vs fd etc.

 
   BrowserConnectionHandler* connection_handler() {
     return &connection_handler_;
@@ -159,7 +174,6 @@ class EngineNetworkComponents : public ConnectionHandler,
   void OnConnectionError(int error) override;
 
   net::NetLog* net_log_;
-  uint16_t port_ = 0;
 
   BrowserConnectionHandler connection_handler_;
   std::unique_ptr<EngineAuthenticationHandler> authentication_handler_;
@@ -189,20 +203,15 @@ void EngineNetworkComponents::Initialize(
       base::MakeUnique<EngineAuthenticationHandler>(this, client_token);
 
   // Plumb unauthenticated connections to |authentication_handler_|.
-  connection_manager_ =
-      base::MakeUnique<EngineConnectionManager>(authentication_handler_.get());
+  connection_manager_ = base::MakeUnique<EngineConnectionManager>(
+      authentication_handler_.get(), net_log_);
 
   blob_channel_service_ =
       base::MakeUnique<BlobChannelService>(blob_channel_sender, ui_task_runner);
 
   // Adds BlimpTransports to connection_manager_.
   net::IPEndPoint address(GetListeningAddress(), GetListeningPort());
-  TCPEngineTransport* transport = new TCPEngineTransport(address, net_log_);
-  connection_manager_->AddTransport(base::WrapUnique(transport));
-
-  transport->GetLocalAddress(&address);
-  port_ = address.port();
-  DVLOG(1) << "Engine port #: " << port_;
+  connection_manager_->ConnectTransport(address, GetTransportType());
 }
 
 void EngineNetworkComponents::HandleConnection(