Refactor BlimpConnection to TCPConnection

blimp/engine/session/blimp_engine_session.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "blimp/engine/session/blimp_engine_session.h"
 
 #include <string>
 #include <utility>
 
 #include "base/command_line.h"
@@ skipping 81 matching lines @@
               const base::Closure& closure) {
   task_runner->PostTask(FROM_HERE, closure);
 }
 
 // Returns a closure that quits the current (bind-time) MessageLoop.
 base::Closure QuitCurrentMessageLoopClosure() {
   return base::Bind(&PostTask, base::ThreadTaskRunnerHandle::Get(),
                     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();
   }
   return net::IPAddress::IPv4Localhost();
 }
 
 uint16_t GetListeningPort() {
   unsigned port_parsed = 0;
   if (!base::StringToUint(
@@ skipping 20 matching lines @@
   explicit EngineNetworkComponents(net::NetLog* net_log);
   ~EngineNetworkComponents() override;
 
   // Sets up network components and starts listening for incoming connection.
   // This should be called after all features have been registered so that
   // received messages can be properly handled.
   void Initialize(scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
                   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_;
   }
 
   BlobChannelService* blob_channel_service() {
     return blob_channel_service_.get();
   }
 
  private:
   // ConnectionHandler implementation.
   void HandleConnection(std::unique_ptr<BlimpConnection> connection) override;
 
   // ConnectionErrorObserver implementation.
   // Signals the engine session that an authenticated connection was
   // terminated.
   void OnConnectionError(int error) override;
 
   net::NetLog* net_log_;
-  uint16_t port_ = 0;
 
   BrowserConnectionHandler connection_handler_;
   std::unique_ptr<EngineAuthenticationHandler> authentication_handler_;
   std::unique_ptr<EngineConnectionManager> connection_manager_;
   std::unique_ptr<BlobChannelService> blob_channel_service_;
   base::Closure quit_closure_;
 
   DISALLOW_COPY_AND_ASSIGN(EngineNetworkComponents);
 };
 
 EngineNetworkComponents::EngineNetworkComponents(net::NetLog* net_log)
     : net_log_(net_log), quit_closure_(QuitCurrentMessageLoopClosure()) {}
 
 EngineNetworkComponents::~EngineNetworkComponents() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 }
 
 void EngineNetworkComponents::Initialize(
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
     base::WeakPtr<BlobChannelSender> blob_channel_sender,
     const std::string& client_token) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(!connection_manager_);
   // Plumb authenticated connections from the authentication handler
   // to |this| (which will then pass it to |connection_handler_|.
   authentication_handler_ =
       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(
     std::unique_ptr<BlimpConnection> connection) {
   // Observe |connection| for disconnection events.
   connection->AddConnectionErrorObserver(this);
   connection_handler_.HandleConnection(std::move(connection));
 }
 
 void EngineNetworkComponents::OnConnectionError(int error) {
@@ skipping 350 matching lines @@
     parent->AddChild(content);
   content->Show();
 
   tab_ = base::MakeUnique<Tab>(std::move(new_contents), target_tab_id,
                                &render_widget_feature_,
                                navigation_message_sender_.get());
 }
 
 }  // namespace engine
 }  // namespace blimp