Expose AssistantScreenContextController and AssistantSetupController.

Previously, out of process calls to AssistantScreenContextController and
AssistantSetupController were proxied through AssistantController. Now,
we expose those subcontrollers directly for binding.

Bug: b:119272394
Change-Id: I136d0e025761b4fe99151218512f83eeb00a6541
Reviewed-on: https://chromium-review.googlesource.com/c/1327532
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611327}

ash/assistant/assistant_controller.cc

@@ -99,23 +99,6 @@ void AssistantController::SetAssistantImageDownloader(
   assistant_image_downloader_ = std::move(assistant_image_downloader);
 }
 
-// TODO(dmblack): Call SetAssistantSetup directly on AssistantSetupController
-// instead of going through AssistantController.
-void AssistantController::SetAssistantSetup(
-    mojom::AssistantSetupPtr assistant_setup) {
-  assistant_setup_ = std::move(assistant_setup);
-  assistant_setup_controller_->SetAssistantSetup(assistant_setup_.get());
-}
-
-// TODO(dmblack): Expose AssistantScreenContextController over mojo rather
-// than implementing RequestScreenshot here in AssistantController.
-void AssistantController::RequestScreenshot(
-    const gfx::Rect& rect,
-    RequestScreenshotCallback callback) {
-  assistant_screen_context_controller_->RequestScreenshot(rect,
-                                                          std::move(callback));
-}
-
 void AssistantController::OpenAssistantSettings() {
   // Launch Assistant settings via deeplink.
   OpenUrl(assistant::util::CreateAssistantSettingsDeepLink());

ash/assistant/assistant_controller.h

@@ -27,7 +27,6 @@
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "mojo/public/cpp/bindings/interface_ptr_set.h"
 #include "services/content/public/mojom/navigable_contents_factory.mojom.h"
-#include "ui/gfx/geometry/rect.h"
 
 namespace ash {
 
@@ -65,15 +64,10 @@ class ASH_EXPORT AssistantController
 
   // mojom::AssistantController:
   // TODO(updowndota): Refactor Set() calls to use a factory pattern.
-  // TODO(dmblack): Expose RequestScreenshot(...) over mojo through
-  // AssistantScreenContextController.
   void SetAssistant(
       chromeos::assistant::mojom::AssistantPtr assistant) override;
   void SetAssistantImageDownloader(
       mojom::AssistantImageDownloaderPtr assistant_image_downloader) override;
-  void SetAssistantSetup(mojom::AssistantSetupPtr assistant_setup) override;
-  void RequestScreenshot(const gfx::Rect& rect,
-                         RequestScreenshotCallback callback) override;
   void OpenAssistantSettings() override;
 
   // AssistantControllerObserver:
@@ -158,8 +152,6 @@ class ASH_EXPORT AssistantController
 
   mojom::AssistantImageDownloaderPtr assistant_image_downloader_;
 
-  mojom::AssistantSetupPtr assistant_setup_;
-
   std::unique_ptr<AssistantCacheController> assistant_cache_controller_;
 
   std::unique_ptr<AssistantInteractionController>

ash/assistant/assistant_screen_context_controller.cc

@@ -47,7 +47,7 @@ std::vector<uint8_t> DownsampleAndEncodeImage(gfx::Image image) {
 }
 
 void EncodeScreenshotAndRunCallback(
-    mojom::AssistantController::RequestScreenshotCallback callback,
+    mojom::AssistantScreenContextController::RequestScreenshotCallback callback,
     std::unique_ptr<ui::LayerTreeOwner> layer_owner,
     gfx::Image image) {
   base::PostTaskWithTraitsAndReplyWithResult(
@@ -129,6 +129,7 @@ std::unique_ptr<ui::LayerTreeOwner> CreateLayerForAssistantSnapshot(
 AssistantScreenContextController::AssistantScreenContextController(
     AssistantController* assistant_controller)
     : assistant_controller_(assistant_controller),
+      binding_(this),
       screen_context_request_factory_(this) {
   assistant_controller_->AddObserver(this);
 }
@@ -137,6 +138,11 @@ AssistantScreenContextController::~AssistantScreenContextController() {
   assistant_controller_->RemoveObserver(this);
 }
 
+void AssistantScreenContextController::BindRequest(
+    mojom::AssistantScreenContextControllerRequest request) {
+  binding_.Bind(std::move(request));
+}
+
 void AssistantScreenContextController::SetAssistant(
     chromeos::assistant::mojom::Assistant* assistant) {
   assistant_ = assistant;
@@ -154,7 +160,8 @@ void AssistantScreenContextController::RemoveModelObserver(
 
 void AssistantScreenContextController::RequestScreenshot(
     const gfx::Rect& rect,
-    mojom::AssistantController::RequestScreenshotCallback callback) {
+    mojom::AssistantScreenContextController::RequestScreenshotCallback
+        callback) {
   aura::Window* root_window = Shell::Get()->GetRootWindowForNewWindows();
 
   std::unique_ptr<ui::LayerTreeOwner> layer_owner =

ash/assistant/assistant_screen_context_controller.h

@@ -13,6 +13,7 @@
 #include "ash/assistant/model/assistant_ui_model_observer.h"
 #include "ash/public/interfaces/assistant_controller.mojom.h"
 #include "base/macros.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "ui/gfx/geometry/rect.h"
 
 namespace ui {
@@ -25,13 +26,16 @@ class AssistantController;
 class AssistantScreenContextModelObserver;
 
 class ASH_EXPORT AssistantScreenContextController
-    : public AssistantControllerObserver,
+    : public ash::mojom::AssistantScreenContextController,
+      public AssistantControllerObserver,
       public AssistantUiModelObserver {
  public:
   explicit AssistantScreenContextController(
       AssistantController* assistant_controller);
   ~AssistantScreenContextController() override;
 
+  void BindRequest(mojom::AssistantScreenContextControllerRequest request);
+
   // Provides a pointer to the |assistant| owned by AssistantController.
   void SetAssistant(chromeos::assistant::mojom::Assistant* assistant);
 
@@ -42,12 +46,11 @@ class ASH_EXPORT AssistantScreenContextController
   void AddModelObserver(AssistantScreenContextModelObserver* observer);
   void RemoveModelObserver(AssistantScreenContextModelObserver* observer);
 
-  // Requests a screenshot for the region defined by |rect| (given in DP). If
-  // an empty rect is supplied, the entire screen is captured. Upon screenshot
-  // completion, the specified |callback| is run.
+  // ash::mojom::AssistantScreenContextController:
   void RequestScreenshot(
       const gfx::Rect& rect,
-      mojom::AssistantController::RequestScreenshotCallback callback);
+      mojom::AssistantScreenContextController::RequestScreenshotCallback
+          callback) override;
 
   // AssistantControllerObserver:
   void OnAssistantControllerConstructed() override;
@@ -66,6 +69,8 @@ class ASH_EXPORT AssistantScreenContextController
  private:
   AssistantController* const assistant_controller_;  // Owned by Shell.
 
+  mojo::Binding<mojom::AssistantScreenContextController> binding_;
+
   // Owned by AssistantController.
   chromeos::assistant::mojom::Assistant* assistant_ = nullptr;
 

ash/assistant/assistant_setup_controller.cc

@@ -12,7 +12,7 @@ namespace ash {
 
 AssistantSetupController::AssistantSetupController(
     AssistantController* assistant_controller)
-    : assistant_controller_(assistant_controller) {
+    : assistant_controller_(assistant_controller), binding_(this) {
   assistant_controller_->AddObserver(this);
 }
 
@@ -20,9 +20,14 @@ AssistantSetupController::~AssistantSetupController() {
   assistant_controller_->RemoveObserver(this);
 }
 
+void AssistantSetupController::BindRequest(
+    mojom::AssistantSetupControllerRequest request) {
+  binding_.Bind(std::move(request));
+}
+
 void AssistantSetupController::SetAssistantSetup(
-    mojom::AssistantSetup* assistant_setup) {
-  assistant_setup_ = assistant_setup;
+    mojom::AssistantSetupPtr assistant_setup) {
+  assistant_setup_ = std::move(assistant_setup);
 }
 
 void AssistantSetupController::OnDeepLinkReceived(

ash/assistant/assistant_setup_controller.h

@@ -10,21 +10,26 @@
 
 #include "ash/assistant/assistant_controller_observer.h"
 #include "ash/assistant/ui/main_stage/assistant_opt_in_view.h"
+#include "ash/public/interfaces/assistant_controller.mojom.h"
 #include "ash/public/interfaces/assistant_setup.mojom.h"
 #include "base/macros.h"
+#include "mojo/public/cpp/bindings/binding.h"
 
 namespace ash {
 
 class AssistantController;
 
-class AssistantSetupController : public AssistantControllerObserver,
+class AssistantSetupController : public mojom::AssistantSetupController,
+                                 public AssistantControllerObserver,
                                  public AssistantOptInDelegate {
  public:
   explicit AssistantSetupController(AssistantController* assistant_controller);
   ~AssistantSetupController() override;
 
-  // Sets the controller's internal |assistant_setup_| reference.
-  void SetAssistantSetup(mojom::AssistantSetup* assistant_setup);
+  void BindRequest(mojom::AssistantSetupControllerRequest request);
+
+  // mojom::AssistantSetupController:
+  void SetAssistantSetup(mojom::AssistantSetupPtr assistant_setup) override;
 
   // AssistantControllerObserver:
   void OnDeepLinkReceived(
@@ -39,8 +44,10 @@ class AssistantSetupController : public AssistantControllerObserver,
 
   AssistantController* const assistant_controller_;  // Owned by Shell.
 
-  mojom::AssistantSetup* assistant_setup_ =
-      nullptr;  // Owned by AssistantController.
+  mojo::Binding<mojom::AssistantSetupController> binding_;
+
+  // Interface to AssistantSetup in chrome/browser.
+  mojom::AssistantSetupPtr assistant_setup_;
 
   DISALLOW_COPY_AND_ASSIGN(AssistantSetupController);
 };

ash/manifest.json

@@ -18,6 +18,8 @@
           "ash.mojom.AppListController",
           "ash.mojom.AshMessageCenterController",
           "ash.mojom.AssistantController",
+          "ash.mojom.AssistantScreenContextController",
+          "ash.mojom.AssistantSetupController",
           "ash.mojom.AssistantVolumeControl",
           "ash.mojom.CastConfig",
           "ash.mojom.CrosDisplayConfigController",

ash/mojo_interface_factory.cc

@@ -11,6 +11,8 @@
 #include "ash/accessibility/accessibility_focus_ring_controller.h"
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/assistant/assistant_controller.h"
+#include "ash/assistant/assistant_screen_context_controller.h"
+#include "ash/assistant/assistant_setup_controller.h"
 #include "ash/cast_config_controller.h"
 #include "ash/display/ash_display_controller.h"
 #include "ash/display/cros_display_config.h"
@@ -87,6 +89,20 @@ void BindAssistantControllerRequestOnMainThread(
   Shell::Get()->assistant_controller()->BindRequest(std::move(request));
 }
 
+void BindAssistantScreenContextControllerRequestOnMainThread(
+    mojom::AssistantScreenContextControllerRequest request) {
+  Shell::Get()
+      ->assistant_controller()
+      ->screen_context_controller()
+      ->BindRequest(std::move(request));
+}
+
+void BindAssistantSetupControllerRequestOnMainThread(
+    mojom::AssistantSetupControllerRequest request) {
+  Shell::Get()->assistant_controller()->setup_controller()->BindRequest(
+      std::move(request));
+}
+
 void BindAssistantVolumeControlRequestOnMainThread(
     mojom::AssistantVolumeControlRequest request) {
   Shell::Get()->assistant_controller()->BindRequest(std::move(request));
@@ -243,6 +259,13 @@ void RegisterInterfaces(
     registry->AddInterface(
         base::BindRepeating(&BindAssistantControllerRequestOnMainThread),
         main_thread_task_runner);
+    registry->AddInterface(
+        base::BindRepeating(
+            &BindAssistantScreenContextControllerRequestOnMainThread),
+        main_thread_task_runner);
+    registry->AddInterface(
+        base::BindRepeating(&BindAssistantSetupControllerRequestOnMainThread),
+        main_thread_task_runner);
     registry->AddInterface(
         base::BindRepeating(&BindAssistantVolumeControlRequestOnMainThread),
         main_thread_task_runner);

ash/public/interfaces/assistant_controller.mojom

@@ -22,14 +22,26 @@ interface AssistantController {
   SetAssistantImageDownloader(
     AssistantImageDownloader assistant_image_downloader);
 
-  // Provides an interface to the |assistant_setup| owned by AssistantClient.
-  SetAssistantSetup(AssistantSetup assistant_setup);
-
-  // Requests screenshot of specified |rect| region and returns the screenshot
-  // encoded in JPEG format. If |rect| is empty, it returns fullscreen
-  // screenshot.
-  RequestScreenshot(gfx.mojom.Rect rect) => (array<uint8> screenshot);
-
   // Opens Google Assistant settings.
   OpenAssistantSettings();
 };
+
+// Interface to the AssistantScreenContextController which is owned by the
+// AssistantController. Currently used by the Assistant service to request
+// screenshots.
+interface AssistantScreenContextController {
+  // Requests a screenshot of the region enclosed by |rect| and returns the
+  // screenshot encoded in JPEG format. If |rect| is empty, it returns a
+  // fullscreen screenshot.
+  RequestScreenshot(gfx.mojom.Rect rect) => (array<uint8> screenshot);
+};
+
+// Interface to the AssistantSetupController which is owned by the
+// AssistantController. Currently used by AssistantSetup in chrome/browser
+// to provide an interface to itself. This is used for triggering a runtime
+// onboarding flow.
+interface AssistantSetupController {
+  // Provides an interface to the |assistant_setup| owned by AssistantClient
+  // in chrome/browser.
+  SetAssistantSetup(AssistantSetup assistant_setup);
+};
\ No newline at end of file

chrome/browser/ui/ash/assistant/assistant_setup.cc

@@ -79,12 +79,12 @@ class AssistantHotwordNotificationDelegate
 
 AssistantSetup::AssistantSetup(service_manager::Connector* connector)
     : connector_(connector), binding_(this) {
-  // Bind to the Assistant controller in ash.
-  ash::mojom::AssistantControllerPtr assistant_controller;
-  connector_->BindInterface(ash::mojom::kServiceName, &assistant_controller);
+  // Bind to the AssistantSetupController in ash.
+  ash::mojom::AssistantSetupControllerPtr setup_controller;
+  connector_->BindInterface(ash::mojom::kServiceName, &setup_controller);
   ash::mojom::AssistantSetupPtr ptr;
   binding_.Bind(mojo::MakeRequest(&ptr));
-  assistant_controller->SetAssistantSetup(std::move(ptr));
+  setup_controller->SetAssistantSetup(std::move(ptr));
 
   arc::VoiceInteractionControllerClient::Get()->AddObserver(this);
 }

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -338,7 +338,7 @@ void AssistantManagerServiceImpl::StartMetalayerInteraction(
   if (!IsScreenContextAllowed(service_->assistant_state()))
     return;
 
-  service_->assistant_controller()->RequestScreenshot(
+  service_->assistant_screen_context_controller()->RequestScreenshot(
       region,
       base::BindOnce(&AssistantManagerServiceImpl::SendScreenContextRequest,
                      weak_factory_.GetWeakPtr(), /*assistant_extra=*/nullptr,
@@ -1113,7 +1113,7 @@ void AssistantManagerServiceImpl::CacheScreenContext(
       base::BindOnce(&AssistantManagerServiceImpl::CacheAssistantStructure,
                      weak_factory_.GetWeakPtr(), on_done));
 
-  service_->assistant_controller()->RequestScreenshot(
+  service_->assistant_screen_context_controller()->RequestScreenshot(
       gfx::Rect(),
       base::BindOnce(&AssistantManagerServiceImpl::CacheAssistantScreenshot,
                      weak_factory_.GetWeakPtr(), on_done));

chromeos/services/assistant/service.cc

@@ -281,9 +281,6 @@ void Service::CreateAssistantManagerService() {
       service_binding_.GetConnector(), std::move(battery_monitor), this,
       network_connection_tracker_);
 
-  // Bind to Assistant controller in ash.
-  service_binding_.GetConnector()->BindInterface(ash::mojom::kServiceName,
-                                                 &assistant_controller_);
   assistant_settings_manager_ =
       assistant_manager_service_.get()->GetAssistantSettingsManager();
 #else
@@ -298,10 +295,18 @@ void Service::FinalizeAssistantManagerService() {
 
   // Using session_observer_binding_ as a flag to control onetime initialization
   if (!session_observer_binding_) {
+    // Bind to the AssistantController in ash.
+    service_binding_.GetConnector()->BindInterface(ash::mojom::kServiceName,
+                                                   &assistant_controller_);
+
     mojom::AssistantPtr ptr;
     BindAssistantConnection(mojo::MakeRequest(&ptr));
     assistant_controller_->SetAssistant(std::move(ptr));
 
+    // Bind to the AssistantScreenContextController in ash.
+    service_binding_.GetConnector()->BindInterface(
+        ash::mojom::kServiceName, &assistant_screen_context_controller_);
+
     registry_.AddInterface<mojom::Assistant>(base::BindRepeating(
         &Service::BindAssistantConnection, base::Unretained(this)));
 

chromeos/services/assistant/service.h

@@ -58,10 +58,18 @@ class Service : public service_manager::Service,
   ~Service() override;
 
   mojom::Client* client() { return client_.get(); }
+
   mojom::DeviceActions* device_actions() { return device_actions_.get(); }
+
   ash::mojom::AssistantController* assistant_controller() {
     return assistant_controller_.get();
   }
+
+  ash::mojom::AssistantScreenContextController*
+  assistant_screen_context_controller() {
+    return assistant_screen_context_controller_.get();
+  }
+
   ash::AssistantStateBase* assistant_state() { return &assistant_state_; }
 
   void RequestAccessToken();
@@ -155,6 +163,8 @@ class Service : public service_manager::Service,
   base::Optional<std::string> access_token_;
 
   ash::mojom::AssistantControllerPtr assistant_controller_;
+  ash::mojom::AssistantScreenContextControllerPtr
+      assistant_screen_context_controller_;
   ash::AssistantStateProxy assistant_state_;
 
   network::NetworkConnectionTracker* network_connection_tracker_;