arc-bridge: Implement IPC message for app launcher

components/arc/arc_bridge_service_impl.cc

Index: components/arc/arc_bridge_service_impl.cc
diff --git a/components/arc/arc_bridge_service.cc b/components/arc/arc_bridge_service_impl.cc
similarity index 55%
copy from components/arc/arc_bridge_service.cc
copy to components/arc/arc_bridge_service_impl.cc
index c4890afe66aca3bc0b4867d7cbfb421462e4946a..a3081cc6ca29d128819d6a9052cadaa3d1e4ba24 100644
--- a/components/arc/arc_bridge_service.cc
+++ b/components/arc/arc_bridge_service_impl.cc
@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/arc/arc_bridge_service.h"
+#include "components/arc/arc_bridge_service_impl.h"
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
+#include "base/sequenced_task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
@@ -24,91 +25,62 @@ namespace {
 const base::FilePath::CharType kArcBridgeSocketPath[] =
     FILE_PATH_LITERAL("/home/chronos/ArcBridge/bridge.sock");
 
-// Weak pointer.  This class is owned by ChromeBrowserMainPartsChromeos.
-ArcBridgeService* g_arc_bridge_service = nullptr;
-
 }  // namespace
 
-ArcBridgeService::ArcBridgeService(
+ArcBridgeServiceImpl::ArcBridgeServiceImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
-    : origin_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      ipc_task_runner_(ipc_task_runner),
+    : ipc_task_runner_(ipc_task_runner),
       file_task_runner_(file_task_runner),
       session_started_(false),
-      available_(false),
-      state_(State::STOPPED),
       weak_factory_(this) {
-  DCHECK(!g_arc_bridge_service);
-  g_arc_bridge_service = this;
-}
-
-ArcBridgeService::~ArcBridgeService() {
-  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  DCHECK(state_ == State::STOPPING || state_ == State::STOPPED);
-  DCHECK(g_arc_bridge_service);
-  g_arc_bridge_service = nullptr;
 }
 
-// static
-ArcBridgeService* ArcBridgeService::Get() {
-  DCHECK(g_arc_bridge_service);
-  DCHECK(g_arc_bridge_service->origin_task_runner_->RunsTasksOnCurrentThread());
-  return g_arc_bridge_service;
+ArcBridgeServiceImpl::~ArcBridgeServiceImpl() {
 }
 
-void ArcBridgeService::DetectAvailability() {
+void ArcBridgeServiceImpl::DetectAvailability() {
   chromeos::SessionManagerClient* session_manager_client =
       chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
   session_manager_client->CheckArcAvailability(base::Bind(
-      &ArcBridgeService::OnArcAvailable, weak_factory_.GetWeakPtr()));
+      &ArcBridgeServiceImpl::OnArcAvailable, weak_factory_.GetWeakPtr()));
 }
 
-void ArcBridgeService::HandleStartup() {
+void ArcBridgeServiceImpl::HandleStartup() {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
   session_started_ = true;
   PrerequisitesChanged();
 }
 
-void ArcBridgeService::Shutdown() {
+void ArcBridgeServiceImpl::Shutdown() {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
   session_started_ = false;
   PrerequisitesChanged();
 }
 
-void ArcBridgeService::AddObserver(Observer* observer) {
-  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  observer_list_.AddObserver(observer);
-}
-
-void ArcBridgeService::RemoveObserver(Observer* observer) {
-  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  observer_list_.RemoveObserver(observer);
-}
-
-void ArcBridgeService::PrerequisitesChanged() {
+void ArcBridgeServiceImpl::PrerequisitesChanged() {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ == State::STOPPED) {
-    if (!available_ || !session_started_)
+  if (state() == State::STOPPED) {
+    if (!available() || !session_started_)
       return;
     SocketConnect(base::FilePath(kArcBridgeSocketPath));
   } else {
-    if (available_ && session_started_)
+    if (available() && session_started_)
       return;
     StopInstance();
   }
 }
 
-void ArcBridgeService::StopInstance() {
+void ArcBridgeServiceImpl::StopInstance() {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ == State::STOPPED || state_ == State::STOPPING) {
+  if (state() == State::STOPPED || state() == State::STOPPING) {
     VLOG(1) << "StopInstance() called when ARC is not running";
     return;
   }
-  if (state_ == State::CONNECTED || state_ == State::CONNECTING) {
+  if (state() == State::CONNECTED || state() == State::CONNECTING) {
     // This was stopped before the D-Bus command to start the instance. Just
     // close the socket (if it was opened).
-    if (state_ == State::CONNECTED) {
+    if (state() == State::CONNECTED) {
       ipc_channel_.reset();
     }
     SetState(State::STOPPED);
@@ -119,14 +91,14 @@ void ArcBridgeService::StopInstance() {
   chromeos::SessionManagerClient* session_manager_client =
       chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
   session_manager_client->StopArcInstance(base::Bind(
-      &ArcBridgeService::OnInstanceStopped, weak_factory_.GetWeakPtr()));
+      &ArcBridgeServiceImpl::OnInstanceStopped, weak_factory_.GetWeakPtr()));
 }
 
-bool ArcBridgeService::RegisterInputDevice(const std::string& name,
-                                           const std::string& device_type,
-                                           base::ScopedFD fd) {
+bool ArcBridgeServiceImpl::RegisterInputDevice(const std::string& name,
+                                               const std::string& device_type,
+                                               base::ScopedFD fd) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::READY) {
+  if (state() != State::READY) {
     LOG(ERROR) << "Called RegisterInputDevice when the service is not ready";
     return false;
   }
@@ -134,9 +106,42 @@ bool ArcBridgeService::RegisterInputDevice(const std::string& name,
       name, device_type, base::FileDescriptor(fd.Pass())));
 }
 
-void ArcBridgeService::SocketConnect(const base::FilePath& socket_path) {
+bool ArcBridgeServiceImpl::RefreshApps() {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::STOPPED) {
+  if (state() != State::READY) {
+    LOG(ERROR) << "Called RefreshApps when the service is not ready";
+    return false;
+  }
+  return ipc_channel_->Send(new ArcInstanceMsg_RefreshApps());
+}
+
+bool ArcBridgeServiceImpl::LaunchApp(const std::string& package,
+                                     const std::string& activity) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state() != State::READY) {
+    LOG(ERROR) << "Called LaunchApp when the service is not ready";
+    return false;
+  }
+
+  return ipc_channel_->Send(new ArcInstanceMsg_LaunchApp(package, activity));
+}
+
+bool ArcBridgeServiceImpl::RequestIcon(const std::string& package,
+                                       const std::string& activity,
+                                       int scale_factor) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state() != State::READY) {
+    LOG(ERROR) << "Called RequestIcon when the service is not ready";
+    return false;
+  }
+  return ipc_channel_->Send(new ArcInstanceMsg_RequestIcon(package,
+                                                           activity,
+                                                           scale_factor));
+}
+
+void ArcBridgeServiceImpl::SocketConnect(const base::FilePath& socket_path) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state() != State::STOPPED) {
     VLOG(1) << "SocketConnect() called when instance is not stopped";
     return;
   }
@@ -144,15 +149,15 @@ void ArcBridgeService::SocketConnect(const base::FilePath& socket_path) {
   base::PostTaskAndReplyWithResult(
       file_task_runner_.get(), FROM_HERE,
       base::Bind(&base::CreateDirectory, socket_path.DirName()),
-      base::Bind(&ArcBridgeService::SocketConnectAfterEnsureParentDirectory,
+      base::Bind(&ArcBridgeServiceImpl::SocketConnectAfterEnsureParentDirectory,
                  weak_factory_.GetWeakPtr(), socket_path));
 }
 
-void ArcBridgeService::SocketConnectAfterEnsureParentDirectory(
+void ArcBridgeServiceImpl::SocketConnectAfterEnsureParentDirectory(
     const base::FilePath& socket_path,
     bool directory_present) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::CONNECTING) {
+  if (state() != State::CONNECTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
@@ -174,14 +179,14 @@ void ArcBridgeService::SocketConnectAfterEnsureParentDirectory(
       // TODO(lhchavez): Tighten the security around the socket by tying it to
       // the user the instance will run as.
       base::Bind(&base::SetPosixFilePermissions, socket_path, 0777),
-      base::Bind(&ArcBridgeService::SocketConnectAfterSetSocketPermissions,
+      base::Bind(&ArcBridgeServiceImpl::SocketConnectAfterSetSocketPermissions,
                  weak_factory_.GetWeakPtr(), socket_path));
 }
 
-bool ArcBridgeService::Connect(const IPC::ChannelHandle& handle,
-                               IPC::Channel::Mode mode) {
+bool ArcBridgeServiceImpl::Connect(const IPC::ChannelHandle& handle,
+                                   IPC::Channel::Mode mode) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::CONNECTING) {
+  if (state() != State::CONNECTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return false;
   }
@@ -194,11 +199,11 @@ bool ArcBridgeService::Connect(const IPC::ChannelHandle& handle,
   return true;
 }
 
-void ArcBridgeService::SocketConnectAfterSetSocketPermissions(
+void ArcBridgeServiceImpl::SocketConnectAfterSetSocketPermissions(
     const base::FilePath& socket_path,
     bool socket_permissions_success) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::CONNECTED) {
+  if (state() != State::CONNECTED) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
@@ -214,13 +219,13 @@ void ArcBridgeService::SocketConnectAfterSetSocketPermissions(
   chromeos::SessionManagerClient* session_manager_client =
       chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
   session_manager_client->StartArcInstance(
-      socket_path.value(), base::Bind(&ArcBridgeService::OnInstanceStarted,
+      socket_path.value(), base::Bind(&ArcBridgeServiceImpl::OnInstanceStarted,
                                       weak_factory_.GetWeakPtr()));
 }
 
-void ArcBridgeService::OnInstanceStarted(bool success) {
+void ArcBridgeServiceImpl::OnInstanceStarted(bool success) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::STARTING) {
+  if (state() != State::STARTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
@@ -231,9 +236,9 @@ void ArcBridgeService::OnInstanceStarted(bool success) {
   }
 }
 
-void ArcBridgeService::OnInstanceBootPhase(InstanceBootPhase phase) {
+void ArcBridgeServiceImpl::OnInstanceBootPhase(InstanceBootPhase phase) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (state_ != State::STARTING) {
+  if (state() != State::STARTING) {
     VLOG(1) << "StopInstance() called while connecting";
     return;
   }
@@ -243,21 +248,15 @@ void ArcBridgeService::OnInstanceBootPhase(InstanceBootPhase phase) {
   FOR_EACH_OBSERVER(Observer, observer_list_, OnInstanceBootPhase(phase));
 }
 
-void ArcBridgeService::SetState(State state) {
-  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  // DCHECK on enum classes not supported.
-  DCHECK(state_ != state);
-  state_ = state;
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnStateChanged(state_));
-}
-
-bool ArcBridgeService::OnMessageReceived(const IPC::Message& message) {
+bool ArcBridgeServiceImpl::OnMessageReceived(const IPC::Message& message) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
   bool handled = true;
 
-  IPC_BEGIN_MESSAGE_MAP(ArcBridgeService, message)
+  IPC_BEGIN_MESSAGE_MAP(ArcBridgeServiceImpl, message)
     IPC_MESSAGE_HANDLER(ArcInstanceHostMsg_InstanceBootPhase,
                         OnInstanceBootPhase)
+    IPC_MESSAGE_HANDLER(ArcInstanceHostMsg_AppsRefreshed, OnAppsRefreshed)
+    IPC_MESSAGE_HANDLER(ArcInstanceHostMsg_AppIcon, OnAppIcon)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
@@ -266,22 +265,50 @@ bool ArcBridgeService::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void ArcBridgeService::OnArcAvailable(bool available) {
+void ArcBridgeServiceImpl::OnAppsRefreshed(
+    const std::vector<std::string>& names,
+    const std::vector<std::string>& packages,
+    const std::vector<std::string>& activities) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
-  if (available_ == available)
+  DCHECK(names.size() == packages.size() && names.size() == activities.size());

[hidehiko, 2015/11/25 07:47:37]

Can we have

std::vector<AppInfo>

where

struct AppInfo {
  std::string name;
  std::string package;
  std::string activity;
};

?

[khmel1, 2015/11/25 15:28:31]

Yes, this simplifies code, however adds arc_message_traits.h/cc 

+  FOR_EACH_OBSERVER(Observer, observer_list_, OnAppsRefreshed(names,
+                                                              packages,
+                                                              activities));
+}
+
+void ArcBridgeServiceImpl::OnAppIcon(const std::string& package,
+                                 const std::string& activity,
+                                 int scale_factor,
+                                 const std::vector<uint8_t>& icon_png_data) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnAppIcon(package,
+                                                        activity,
+                                                        scale_factor,
+                                                        icon_png_data));
+}
+
+void ArcBridgeServiceImpl::OnArcAvailable(bool arc_available) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (available() == arc_available)
     return;
-  available_ = available;
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnAvailableChanged(available_));
+  SetAvailable(arc_available);
   PrerequisitesChanged();
 }
 
-void ArcBridgeService::OnInstanceStopped(bool success) {
+void ArcBridgeServiceImpl::OnInstanceStopped(bool success) {
   DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
   // STOPPING is the only valid state for this function.
   // DCHECK on enum classes not supported.
-  DCHECK(state_ == State::STOPPING);
+  DCHECK(state() == State::STOPPING);
   ipc_channel_.reset();
   SetState(State::STOPPED);
 }
 
+// static
+ArcBridgeService* ArcBridgeService::Create(

[satorux1, 2015/11/25 06:35:08]

shouldn't this be defined in arc_bridge_service.cc? 

I wanted to tale a look at this function but got confused because it's not in
arc_bridge_service.cc

[khmel1, 2015/11/25 06:51:08]

It was not clear for me how to do better. But I found template in legacy code:
https://code.google.com/p/chromium/codesearch#chromium/src/components/wifi/wi...
If I move this function to arc_bridge_service.cc then I need to include
arc_bridge_service_impl.h also, which my be not acceptable.

[satorux1, 2015/11/25 07:08:50]

I think including arc_bridge_service_impl.h from arc_bridge_service.cc is
alright.

[satorux1, 2015/11/25 07:18:56]

FWIW, I played with a one liner to find foo.cc that includes Create() with
corresponding foo_impl.cc

% for i in chrome/browser/**/*_impl.cc; do grep -H '^[^ ].*::Create('
${i:s/_impl.cc/.cc/}; done 2> /dev/null      
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.cc:scoped_ptr<EnterpriseEnrollmentHelper>
EnterpriseEnrollmentHelper::Create(
chrome/browser/local_discovery/gcd_api_flow.cc:scoped_ptr<GCDApiFlow>
GCDApiFlow::Create(
chrome/browser/ui/autofill/autofill_dialog_controller.cc:AutofillDialogController::Create(


Returning scoped_ptr<> looks neat. Can we do this here?

[hidehiko, 2015/11/25 07:47:37]

+1 for both scoped_ptr<> returning and moving this to arc_bridge_service.cc

[khmel1, 2015/11/25 15:28:31]

Thank for your help! Done

+    const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner,
+    const scoped_refptr<base::SequencedTaskRunner>& file_task_runner) {
+  return new ArcBridgeServiceImpl(ipc_task_runner, file_task_runner);
+}
+
 }  // namespace arc