chromeos: Introduce SetClient() on ash::mojom::SystemTray interface

Previously ash would make a direct connection to Chrome (content_browser) to
handle showing system tray menu UI. This is a layering violation and makes
it more difficult to run ash without chrome.

Chrome's SystemTrayClient now explicitly registers itself with ash on startup.
It no longer attempts to reconnect on error; my understanding is that chrome
tolerating an ash crash is not a goal.

BUG=none
TEST=manual, can still open settings via system tray menu

Review-Url: https://codereview.chromium.org/2525813004
Cr-Commit-Position: refs/heads/master@{#434736}

ash/common/system/tray/system_tray_controller.cc

@@ -6,117 +6,112 @@
 
 #include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/wm_shell.h"
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "content/public/common/service_names.mojom.h"
-#include "services/service_manager/public/cpp/connector.h"
 
 namespace ash {
 
-SystemTrayController::SystemTrayController(
-    service_manager::Connector* connector)
-    : connector_(connector), hour_clock_type_(base::GetHourClockType()) {}
+SystemTrayController::SystemTrayController()
+    : hour_clock_type_(base::GetHourClockType()) {}
 
 SystemTrayController::~SystemTrayController() {}
 
 void SystemTrayController::ShowSettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowSettings();
 }
 
 void SystemTrayController::ShowDateSettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowDateSettings();
 }
 
 void SystemTrayController::ShowSetTimeDialog() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowSetTimeDialog();
 }
 
 void SystemTrayController::ShowDisplaySettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowDisplaySettings();
 }
 
 void SystemTrayController::ShowPowerSettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowPowerSettings();
 }
 
 void SystemTrayController::ShowChromeSlow() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowChromeSlow();
 }
 
 void SystemTrayController::ShowIMESettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowIMESettings();
 }
 
 void SystemTrayController::ShowHelp() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowHelp();
 }
 
 void SystemTrayController::ShowAccessibilityHelp() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowAccessibilityHelp();
 }
 
 void SystemTrayController::ShowAccessibilitySettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowAccessibilitySettings();
 }
 
 void SystemTrayController::ShowPaletteHelp() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowPaletteHelp();
 }
 
 void SystemTrayController::ShowPaletteSettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowPaletteSettings();
 }
 
 void SystemTrayController::ShowPublicAccountInfo() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowPublicAccountInfo();
 }
 
 void SystemTrayController::ShowNetworkConfigure(const std::string& network_id) {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowNetworkConfigure(network_id);
 }
 
 void SystemTrayController::ShowNetworkCreate(const std::string& type) {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowNetworkCreate(type);
 }
 
 void SystemTrayController::ShowThirdPartyVpnCreate(
     const std::string& extension_id) {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowThirdPartyVpnCreate(extension_id);
 }
 
 void SystemTrayController::ShowNetworkSettings(const std::string& network_id) {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowNetworkSettings(network_id);
 }
 
 void SystemTrayController::ShowProxySettings() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->ShowProxySettings();
 }
 
 void SystemTrayController::SignOut() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->SignOut();
 }
 
 void SystemTrayController::RequestRestartForUpdate() {
-  if (ConnectToSystemTrayClient())
+  if (system_tray_client_)
     system_tray_client_->RequestRestartForUpdate();
 }
 
@@ -124,33 +119,8 @@ void SystemTrayController::BindRequest(mojom::SystemTrayRequest request) {
   bindings_.AddBinding(this, std::move(request));
 }
 
-bool SystemTrayController::ConnectToSystemTrayClient() {
-  // Unit tests may not have a connector.
-  if (!connector_)
-    return false;
-
-#if defined(OS_CHROMEOS)
-  // If already connected, nothing to do.
-  if (system_tray_client_.is_bound())
-    return true;
-
-  // Connect (or reconnect) to the interface.
-  connector_->ConnectToInterface(content::mojom::kBrowserServiceName,
-                                 &system_tray_client_);
-
-  // Handle chrome crashes by forcing a reconnect on the next request.
-  system_tray_client_.set_connection_error_handler(base::Bind(
-      &SystemTrayController::OnClientConnectionError, base::Unretained(this)));
-  return true;
-#else
-  // The SystemTrayClient interface in the browser is only implemented for
-  // Chrome OS, so don't try to connect on other platforms.
-  return false;
-#endif  // defined(OS_CHROMEOS)
-}
-
-void SystemTrayController::OnClientConnectionError() {
-  system_tray_client_.reset();
+void SystemTrayController::SetClient(mojom::SystemTrayClientPtr client) {
+  system_tray_client_ = std::move(client);
 }
 
 void SystemTrayController::SetUse24HourClock(bool use_24_hour) {

ash/common/system/tray/system_tray_controller.h

@@ -12,29 +12,21 @@
 #include "base/macros.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 
-namespace service_manager {
-class Connector;
-}
-
 namespace ash {
 
 // Both implements mojom::SystemTray and wraps the mojom::SystemTrayClient
-// interface. The wrapper makes the initial connection and handles reconnecting
-// on error. Implements both because it caches state pushed down from the
+// interface. Implements both because it caches state pushed down from the
 // browser process via SystemTray so it can be synchronously queried inside ash.
 //
 // Conceptually similar to historical ash-to-chrome interfaces like
 // SystemTrayDelegate. Lives on the main thread.
 //
-// Only connects to the actual mojom::SystemTrayClient interface when running on
-// Chrome OS. In tests and on Windows all operations are no-ops.
-//
 // TODO: Consider renaming this to SystemTrayClient or renaming the current
 // SystemTray to SystemTrayView and making this class SystemTray.
 class ASH_EXPORT SystemTrayController
     : NON_EXPORTED_BASE(public mojom::SystemTray) {
  public:
-  explicit SystemTrayController(service_manager::Connector* connector);
+  SystemTrayController();
   ~SystemTrayController() override;
 
   base::HourClockType hour_clock_type() const { return hour_clock_type_; }
@@ -65,19 +57,10 @@ class ASH_EXPORT SystemTrayController
   void BindRequest(mojom::SystemTrayRequest request);
 
  private:
-  // Connects or reconnects to the mojom::SystemTrayClient interface when
-  // running on Chrome OS. Otherwise does nothing. Returns true if connected.
-  bool ConnectToSystemTrayClient();
-
-  // Handles errors on the |system_tray_client_| interface connection.
-  void OnClientConnectionError();
-
   // mojom::SystemTray:
+  void SetClient(mojom::SystemTrayClientPtr client) override;
   void SetUse24HourClock(bool use_24_hour) override;
 
-  // May be null in unit tests.
-  service_manager::Connector* connector_;
-
   // Client interface in chrome browser. Only bound on Chrome OS.
   mojom::SystemTrayClientPtr system_tray_client_;
 

ash/common/wm_shell.cc

@@ -255,8 +255,7 @@ WmShell::WmShell(std::unique_ptr<ShellDelegate> shell_delegate)
           delegate_->GetShellConnector())),
       shelf_controller_(base::MakeUnique<ShelfController>()),
       shutdown_controller_(base::MakeUnique<ShutdownController>()),
-      system_tray_controller_(base::MakeUnique<SystemTrayController>(
-          delegate_->GetShellConnector())),
+      system_tray_controller_(base::MakeUnique<SystemTrayController>()),
       system_tray_notifier_(base::MakeUnique<SystemTrayNotifier>()),
       wallpaper_delegate_(delegate_->CreateWallpaperDelegate()),
       window_cycle_controller_(base::MakeUnique<WindowCycleController>()),

ash/public/interfaces/system_tray.mojom

@@ -6,6 +6,9 @@ module ash.mojom;
 
 // Allows clients (e.g. Chrome browser) to control the ash system tray menu.
 interface SystemTray {
+  // Sets the client interface.
+  SetClient(SystemTrayClient client);
+
   // Sets the clock to use 24 hour time formatting if |use_24_hour| is true.
   // Otherwise sets 12 hour time formatting.
   SetUse24HourClock(bool use_24_hour);

chrome/browser/chrome_content_browser_manifest_overlay.json

@@ -24,7 +24,6 @@
           "ash::mojom::ShelfController",
           "ash::mojom::ShutdownController",
           "ash::mojom::SystemTray",
-          "ash::mojom::SystemTrayClient",
           "ash::mojom::WallpaperController",
           "ash::mojom::WallpaperManager",
           "ash::mojom::VolumeController",

chrome/browser/chromeos/chrome_interface_factory.cc

@@ -22,7 +22,6 @@
 #include "chrome/browser/ui/ash/ash_util.h"
 #include "chrome/browser/ui/ash/chrome_new_window_client.h"
 #include "chrome/browser/ui/ash/keyboard_ui_service.h"
-#include "chrome/browser/ui/ash/system_tray_client.h"
 #include "chrome/browser/ui/ash/volume_controller_chromeos.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -125,11 +124,6 @@ class FactoryImpl {
                                            std::move(request));
   }
 
-  void BindRequest(ash::mojom::SystemTrayClientRequest request) {
-    system_tray_client_bindings_.AddBinding(SystemTrayClient::Get(),
-                                            std::move(request));
-  }
-
   void BindRequest(ash::mojom::WallpaperManagerRequest request) {
     WallpaperManager::Get()->BindRequest(std::move(request));
   }
@@ -154,7 +148,6 @@ class FactoryImpl {
   std::unique_ptr<ChromeLaunchable> launchable_;
   std::unique_ptr<ChromeNewWindowClient> new_window_client_;
   mojo::BindingSet<ash::mojom::NewWindowClient> new_window_client_bindings_;
-  mojo::BindingSet<ash::mojom::SystemTrayClient> system_tray_client_bindings_;
   std::unique_ptr<VolumeController> volume_controller_;
   std::unique_ptr<AppListPresenterService> app_list_presenter_service_;
   mojo::BindingSet<app_list::mojom::AppListPresenter>
@@ -186,8 +179,6 @@ bool ChromeInterfaceFactory::OnConnect(
                                                    main_thread_task_runner_);
   FactoryImpl::AddFactory<ash::mojom::NewWindowClient>(
       registry, main_thread_task_runner_);
-  FactoryImpl::AddFactory<ash::mojom::SystemTrayClient>(
-      registry, main_thread_task_runner_);
   FactoryImpl::AddFactory<ash::mojom::VolumeController>(
       registry, main_thread_task_runner_);
   FactoryImpl::AddFactory<ash::mojom::WallpaperManager>(

chrome/browser/ui/ash/system_tray_client.cc

@@ -9,8 +9,6 @@
 #include "ash/common/wm_shell.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/shell.h"
-#include "base/bind.h"
-#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
@@ -63,7 +61,20 @@ void ShowSettingsSubPageForActiveUser(const std::string& sub_page) {
 
 }  // namespace
 
-SystemTrayClient::SystemTrayClient() {
+SystemTrayClient::SystemTrayClient() : binding_(this) {
+  service_manager::Connector* connector =
+      content::ServiceManagerConnection::GetForProcess()->GetConnector();
+  // Connect to the SystemTray interface in ash. Under mash the SystemTray
+  // interface is in the ash process. In classic ash we provide it to ourself.
+  if (chrome::IsRunningInMash()) {
+    connector->ConnectToInterface("ash", &system_tray_);
+  } else {
+    connector->ConnectToInterface(content::mojom::kBrowserServiceName,
+                                  &system_tray_);
+  }
+  // Register this object as the client interface implementation.
+  system_tray_->SetClient(binding_.CreateInterfacePtrAndBind());
+
   // If this observes clock setting changes before ash comes up the IPCs will
   // be queued on |system_tray_|.
   g_browser_process->platform_part()->GetSystemClock()->AddObserver(this);
@@ -314,30 +325,5 @@ void SystemTrayClient::RequestRestartForUpdate() {
 
 void SystemTrayClient::OnSystemClockChanged(
     chromeos::system::SystemClock* clock) {
-  ConnectToSystemTray();
   system_tray_->SetUse24HourClock(clock->ShouldUse24HourClock());
 }
-
-void SystemTrayClient::ConnectToSystemTray() {
-  if (system_tray_.is_bound())
-    return;
-
-  service_manager::Connector* connector =
-      content::ServiceManagerConnection::GetForProcess()->GetConnector();
-  // Under mash the SystemTray interface is in the ash process. In classic ash
-  // we provide it to ourself.
-  if (chrome::IsRunningInMash()) {
-    connector->ConnectToInterface("ash", &system_tray_);
-  } else {
-    connector->ConnectToInterface(content::mojom::kBrowserServiceName,
-                                  &system_tray_);
-  }
-
-  // Tolerate ash crashing and coming back up.
-  system_tray_.set_connection_error_handler(base::Bind(
-      &SystemTrayClient::OnClientConnectionError, base::Unretained(this)));
-}
-
-void SystemTrayClient::OnClientConnectionError() {
-  system_tray_.reset();
-}

chrome/browser/ui/ash/system_tray_client.h

@@ -69,15 +69,12 @@ class SystemTrayClient : public ash::mojom::SystemTrayClient,
   // chromeos::system::SystemClockObserver:
   void OnSystemClockChanged(chromeos::system::SystemClock* clock) override;
 
-  // Connects or reconnects the |system_tray_| interface.
-  void ConnectToSystemTray();
-
-  // Handles errors on the |system_tray_| interface connection.
-  void OnClientConnectionError();
-
   // System tray mojo service in ash.
   ash::mojom::SystemTrayPtr system_tray_;
 
+  // Binds this object to the client interface.
+  mojo::Binding<ash::mojom::SystemTrayClient> binding_;
+
   DISALLOW_COPY_AND_ASSIGN(SystemTrayClient);
 };