[cast_channel] Implement CastSocketService::OpenSocket()

extensions/browser/api/cast_channel/cast_channel_api.cc

Index: extensions/browser/api/cast_channel/cast_channel_api.cc
diff --git a/extensions/browser/api/cast_channel/cast_channel_api.cc b/extensions/browser/api/cast_channel/cast_channel_api.cc
index b769c3954ecbd169076a81e85e4f2ff9447ec5a6..16bf1264840d3c066f7f928327216c2a492822b1 100644
--- a/extensions/browser/api/cast_channel/cast_channel_api.cc
+++ b/extensions/browser/api/cast_channel/cast_channel_api.cc
@@ -224,9 +224,7 @@ void CastChannelAsyncApiFunction::SetResultFromChannelInfo(
   SetResult(channel_info.ToValue());
 }
 
-CastChannelOpenFunction::CastChannelOpenFunction()
-    : new_channel_id_(0) {
-}
+CastChannelOpenFunction::CastChannelOpenFunction() {}
 
 CastChannelOpenFunction::~CastChannelOpenFunction() { }
 
@@ -287,62 +285,50 @@ bool CastChannelOpenFunction::Prepare() {
 void CastChannelOpenFunction::AsyncWorkStart() {
   DCHECK(api_);
   DCHECK(ip_endpoint_.get());
-  const ConnectInfo& connect_info = params_->connect_info;
-  CastSocket* socket;
+
   std::unique_ptr<CastSocket> test_socket = api_->GetSocketForTest();
-  if (test_socket.get()) {
-    socket = test_socket.release();
-  } else {
-    socket = new CastSocketImpl(
-        extension_->id(), *ip_endpoint_, channel_auth_,
-        ExtensionsBrowserClient::Get()->GetNetLog(),
-        base::TimeDelta::FromMilliseconds(connect_info.timeout.get()
-                                              ? *connect_info.timeout
-                                              : kDefaultConnectTimeoutMillis),
-        liveness_timeout_ > base::TimeDelta(), api_->GetLogger(),
-        connect_info.capabilities.get() ? *connect_info.capabilities
-                                        : CastDeviceCapability::NONE);
-  }
-  new_channel_id_ = cast_socket_service_->AddSocket(base::WrapUnique(socket));
-
-  // Construct read delegates.
-  std::unique_ptr<CastTransport::Delegate> delegate(
-      base::MakeUnique<CastMessageHandler>(
-          base::Bind(&CastChannelAPI::SendEvent, api_->AsWeakPtr(),
-                     extension_->id()),
-          socket, api_->GetLogger()));
-  if (socket->keep_alive()) {
-    // Wrap read delegate in a KeepAliveDelegate for timeout handling.
-    KeepAliveDelegate* keep_alive =
-        new KeepAliveDelegate(socket, api_->GetLogger(), std::move(delegate),
-                              ping_interval_, liveness_timeout_);
-    std::unique_ptr<base::Timer> injected_timer =
-        api_->GetInjectedTimeoutTimerForTest();
-    if (injected_timer) {
-      keep_alive->SetTimersForTest(base::MakeUnique<base::Timer>(false, false),
-                                   std::move(injected_timer));
-    }
-    delegate.reset(keep_alive);
-  }
+  if (test_socket)
+    cast_socket_service_->SetSocketForTest(std::move(test_socket));
 
-  socket->Connect(std::move(delegate),
-                  base::Bind(&CastChannelOpenFunction::OnOpen, this));
-}
+  std::unique_ptr<base::Timer> injected_timer =
+      api_->GetInjectedTimeoutTimerForTest();
+  if (injected_timer)
+    cast_socket_service_->SetPingTimeoutTimerForTest(std::move(injected_timer));
 
-void CastChannelOpenFunction::OnOpen(ChannelError result) {
+  const ConnectInfo& connect_info = params_->connect_info;
+  base::TimeDelta connect_timeout = base::TimeDelta::FromMilliseconds(
+      connect_info.timeout.get() ? *connect_info.timeout
+                                 : kDefaultConnectTimeoutMillis);
+  int capabilities = connect_info.capabilities.get()
+                         ? *connect_info.capabilities
+                         : CastDeviceCapability::NONE;
+  cast_socket_service_->OpenSocket(
+      *ip_endpoint_, channel_auth_, ExtensionsBrowserClient::Get()->GetNetLog(),
+      connect_timeout, ping_interval_, liveness_timeout_, api_->GetLogger(),
+      capabilities, base::Bind(&CastChannelOpenFunction::OnOpen, this));
+}
+
+void CastChannelOpenFunction::OnOpen(int channel_id, ChannelError result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   VLOG(1) << "Connect finished, OnOpen invoked.";
   // TODO: If we failed to open the CastSocket, we may want to clean up here,
   // rather than relying on the extension to call close(). This can be done by
   // calling RemoveSocket() and api_->GetLogger()->ClearLastErrors(channel_id).
   if (result != ChannelError::UNKNOWN) {
-    CastSocket* socket = cast_socket_service_->GetSocket(new_channel_id_);
+    CastSocket* socket = cast_socket_service_->GetSocket(channel_id);
     CHECK(socket);
+
+    std::unique_ptr<CastTransport::Delegate> delegate(
+        base::MakeUnique<CastMessageHandler>(
+            base::Bind(&CastChannelAPI::SendEvent, api_->AsWeakPtr(),
+                       extension_->id()),
+            socket, api_->GetLogger()));

[mark a. foltz, 2017/06/12 21:14:09]

You might want to pass the logger as an argument to onOpen, unless you can
guarantee that cast_socket_service_ (which owns the callback that calls OnOpen)
is destroyed before |api_|.

IIUC, both are keyed services on the UI thread, so you could also make |api_|
depend on CastSocketService, which should guarantee destruction order.

https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

Unfortunately I can't find any authoritative documentation on KeyedService :-(

[zhaobin, 2017/06/20 01:37:42]

Code removed. api_->GetLogger() is moved back to
CastChannelOpenFunction::AsyncWorkStart().

+    cast_socket_service_->RegisterDelegate(channel_id, std::move(delegate));
+
     SetResultFromSocket(*socket);
   } else {
     // The socket is being destroyed.
-    SetResultFromError(new_channel_id_,
-                       api::cast_channel::CHANNEL_ERROR_UNKNOWN);
+    SetResultFromError(channel_id, api::cast_channel::CHANNEL_ERROR_UNKNOWN);
   }
 
   AsyncWorkCompleted();