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..8f12eeb08557c87227155f45479595313d6aac8b 100644
--- a/blimp/engine/session/blimp_engine_session.cc
+++ b/blimp/engine/session/blimp_engine_session.cc
@@ -72,6 +72,9 @@ const int kDefaultDisplayWidth = 800;
 const int kDefaultDisplayHeight = 600;
 const uint16_t kDefaultPort = 25467;
 
+const char kTcpTransport[] = "tcp";
+const char kGrpcTransport[] = "grpc";
+
 // Focus rules that support activating an child window.
 class FocusRulesImpl : public wm::BaseFocusRules {
  public:
@@ -99,6 +102,20 @@ base::Closure QuitCurrentMessageLoopClosure() {
                     base::MessageLoop::QuitWhenIdleClosure());
 }
 
+EngineConnectionManager::EngineTransportType GetTransportType() {
+  const std::string transport_parsed =
+      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 "
+             << kGrpcTransport << ", " << kTcpTransport;
+  return EngineConnectionManager::EngineTransportType::TCP;
+}
+
 net::IPAddress GetListeningAddress() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(kAllowNonLocalhost)) {
     return net::IPAddress::IPv4AllZeros();
@@ -139,6 +156,8 @@ class EngineNetworkComponents : public ConnectionHandler,
                   base::WeakPtr<BlobChannelSender> blob_channel_sender,
                   const std::string& client_token);
 
+  // TODO(perumaal): Remove this once gRPC support is ready.
+  //                 See crbug.com/659279.
   uint16_t GetPortForTesting() { return port_; }
 
   BrowserConnectionHandler* connection_handler() {
@@ -189,20 +208,16 @@ 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);
+  connection_manager_->ConnectTransport(&address, GetTransportType());

[Garrett Casto, 2016/10/26 23:43:48]

I think that it's a little cleaner to not have ConnectTransport() call
GetLocalAddress(), and just do it here as it was before. It's just a little more
obvious.

[perumaal, 2016/10/27 00:16:04]

The problem is that I explicitly avoid leaking the BlimpTransport out of the
ConnectionManager.
i.e. EngineConnectionManager is the sole owner/proprietor of the Transport. This
also prevents the caller from incorrectly accessing the transport as many of the
methods are based on callbacks from the transport class itself so it's not a
good API to expose out.

[Garrett Casto, 2016/10/27 16:45:45]

Right, I wasn't very clear about this. What I actually meant to say was that
exposing something like GetLocalAddress() from EngineConnectionManager() (and
plumbing through to BlimpEngineTranport) might be a little clearer. That being
said, I'm not hung up on this, just a slight readability preference.

   port_ = address.port();
-  DVLOG(1) << "Engine port #: " << port_;
 }
 
 void EngineNetworkComponents::HandleConnection(