Add from_user param to screen context requests.

This parameter is needed to distinguish between user initiated requests
and requests that we make ourselves.

User generated requests initiate a conversation interaction that will
return contextual cards. This may result from a suggestion chip press
or Assistant stylus tool selection.

Requests that do not originate from the user will not return contextual
cards or start a conversation interaction. Instead, we will cache the
screen context to send alongside subsequent voice/text queries for
context. This portion is not yet wired up.

Bug: b:112113187
Change-Id: I12cd531745d174738988808d7637c8529ddd80d0
Reviewed-on: https://chromium-review.googlesource.com/1170428
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582459}

ash/assistant/assistant_screen_context_controller.cc

@@ -171,14 +171,15 @@ void AssistantScreenContextController::RequestScreenshot(
 }
 
 void AssistantScreenContextController::RequestScreenContext(
-    const gfx::Rect& rect) {
+    const gfx::Rect& rect,
+    bool from_user) {
   // Abort any request in progress and update request state.
   screen_context_request_factory_.InvalidateWeakPtrs();
   assistant_screen_context_model_.SetRequestState(
       ScreenContextRequestState::kInProgress);
 
   assistant_->RequestScreenContext(
-      rect,
+      rect, from_user,
       base::BindOnce(
           &AssistantScreenContextController::OnScreenContextRequestFinished,
           screen_context_request_factory_.GetWeakPtr()));
@@ -214,13 +215,13 @@ void AssistantScreenContextController::OnUiVisibilityChanged(
     return;
 
   // Request screen context for the entire screen.
-  RequestScreenContext(gfx::Rect());
+  RequestScreenContext(gfx::Rect(), /*from_user=*/false);
 }
 
 void AssistantScreenContextController::OnHighlighterSelectionRecognized(
     const gfx::Rect& rect) {
   // Request screen context for the selected region.
-  RequestScreenContext(rect);
+  RequestScreenContext(rect, /*from_user=*/true);
 }
 
 void AssistantScreenContextController::OnScreenContextRequestFinished() {

ash/assistant/assistant_screen_context_controller.h

@@ -76,7 +76,7 @@ class ASH_EXPORT AssistantScreenContextController
   std::unique_ptr<ui::LayerTreeOwner> CreateLayerForAssistantSnapshotForTest();
 
  private:
-  void RequestScreenContext(const gfx::Rect& rect);
+  void RequestScreenContext(const gfx::Rect& rect, bool from_user);
 
   AssistantController* const assistant_controller_;  // Owned by Shell.
 

ash/assistant/ui/assistant_container_view_unittest.cc

@@ -65,11 +65,11 @@ class AssistantContainerViewTest : public AshTestBase {
     controller_->SetAssistant(std::move(assistant));
 
     // Mock any screen context requests by immediately invoking callback.
-    ON_CALL(*assistant_, DoRequestScreenContext(testing::_, testing::_))
+    ON_CALL(*assistant_,
+            DoRequestScreenContext(testing::_, testing::_, testing::_))
         .WillByDefault(testing::Invoke(
-            [](const gfx::Rect& rect, base::OnceClosure* callback) {
-              std::move(*callback).Run();
-            }));
+            [](const gfx::Rect& rect, bool from_user,
+               base::OnceClosure* callback) { std::move(*callback).Run(); }));
   }
 
   base::test::ScopedFeatureList scoped_feature_list_;

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -195,6 +195,7 @@ void AssistantManagerServiceImpl::SendUpdateSettingsUiRequest(
 
 void AssistantManagerServiceImpl::RequestScreenContext(
     const gfx::Rect& region,
+    bool from_user,
     RequestScreenContextCallback callback) {
   if (!assistant_enabled_ || !context_enabled_) {
     // If context is not enabled, we notify assistant immediately to activate
@@ -204,10 +205,15 @@ void AssistantManagerServiceImpl::RequestScreenContext(
   }
 
   // We wait for the closure to execute twice: once for the screenshot and once
-  // for the view hierarchy.
+  // for the view hierarchy. If the screen context request was user initiated,
+  // we then proceed to send a screen context query to the server. Otherwise,
+  // we cache our screen context to send alongside subsequent voice/text
+  // queries. In all cases, |callback| will be run to inform our caller that
+  // context grabbing has been completed.
   auto on_done = base::BarrierClosure(
       2, base::BindOnce(
-             &AssistantManagerServiceImpl::SendContextQueryAndRunCallback,
+             from_user ? &AssistantManagerServiceImpl::SendScreenContextQuery
+                       : &AssistantManagerServiceImpl::CacheScreenContext,
              weak_factory_.GetWeakPtr(), std::move(callback)));
 
   service_->client()->RequestAssistantStructure(
@@ -845,15 +851,33 @@ void AssistantManagerServiceImpl::OnAssistantScreenshotReceived(
   std::move(on_done).Run();
 }
 
-void AssistantManagerServiceImpl::SendContextQueryAndRunCallback(
+void AssistantManagerServiceImpl::CacheScreenContext(
     RequestScreenContextCallback callback) {
+  // TODO(dmblack): Cache view hierarchy and screenshot data so that we can
+  // send it alongside subsequent voice/text queries for additional context.
+  assistant_extra_.reset();
+  assistant_tree_.reset();
+  assistant_screenshot_.clear();
+
+  // Run callback now that screen context has been successfully grabbed.
+  std::move(callback).Run();
+}
+
+void AssistantManagerServiceImpl::SendScreenContextQuery(
+    RequestScreenContextCallback callback) {
+  // Note: This call initiates a user facing interaction which will interrupt
+  // any other user interaction in progress. As such, it should only be called
+  // as a direct result of a query for screen related content initiated by the
+  // user, e.g. via suggestion chip press or Assistant stylus tool selection.
   assistant_manager_internal_->SendScreenContextRequest(
-      std::vector<std::string>{
-          CreateContextProto(AssistantBundle{
-              std::move(assistant_extra_), std::move(assistant_tree_),
-          }),
-          CreateContextProto(assistant_screenshot_)});
+      {CreateContextProto(AssistantBundle{
+           std::move(assistant_extra_), std::move(assistant_tree_),
+       }),
+       CreateContextProto(assistant_screenshot_)});
+
   assistant_screenshot_.clear();
+
+  // Run callback now that screen context has been successfully grabbed.
   std::move(callback).Run();
 }
 

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -94,6 +94,7 @@ class AssistantManagerServiceImpl
   void DismissNotification(
       mojom::AssistantNotificationPtr notification) override;
   void RequestScreenContext(const gfx::Rect& region,
+                            bool from_user,
                             RequestScreenContextCallback callback) override;
 
   // AssistantActionObserver overrides:
@@ -178,7 +179,8 @@ class AssistantManagerServiceImpl
 
   void RegisterFallbackMediaHandler();
 
-  void SendContextQueryAndRunCallback(RequestScreenContextCallback callback);
+  void CacheScreenContext(RequestScreenContextCallback callback);
+  void SendScreenContextQuery(RequestScreenContextCallback callback);
 
   void OnAssistantStructureReceived(
       base::OnceClosure on_done,

chromeos/services/assistant/fake_assistant_manager_service_impl.cc

@@ -48,6 +48,7 @@ void FakeAssistantManagerServiceImpl::SendUpdateSettingsUiRequest(
 
 void FakeAssistantManagerServiceImpl::RequestScreenContext(
     const gfx::Rect& region,
+    bool from_user,
     RequestScreenContextCallback callback) {}
 
 void FakeAssistantManagerServiceImpl::StartVoiceInteraction() {}

chromeos/services/assistant/fake_assistant_manager_service_impl.h

@@ -53,6 +53,7 @@ class FakeAssistantManagerServiceImpl : public AssistantManagerService {
   void DismissNotification(
       mojom::AssistantNotificationPtr notification) override;
   void RequestScreenContext(const gfx::Rect& region,
+                            bool from_user,
                             RequestScreenContextCallback callback) override;
 
  private:

chromeos/services/assistant/public/mojom/assistant.mojom

@@ -47,8 +47,13 @@ interface Assistant {
 
   // Asks assistant to grab screen context. |region| controls the part of the
   // screen to snapshot. If |region| is empty, it takes fullscreen screenshot.
-  // After grabbing completion, the callback will be invoked.
-  RequestScreenContext(gfx.mojom.Rect region) => ();
+  // The |from_user| parameter is true if the request originated from the user,
+  // false otherwise. A user request for screen context may come as a result of
+  // a suggestion chip press or Assistant stylus selection. A request that did
+  // not originate from the user occurs on launch of Assistant to provide
+  // additional context for subsequent voice/text queries. After grab
+  // completion, the callback will be invoked.
+  RequestScreenContext(gfx.mojom.Rect region, bool from_user) => ();
 };
 
 // Subscribes to assistant's interaction event. These events are server driven

chromeos/services/assistant/test_support/mock_assistant.h

@@ -46,15 +46,16 @@ class MockAssistant : public mojom::Assistant {
                void(chromeos::assistant::mojom::AssistantNotificationPtr));
 
   // Mock DoRequestScreenContext in lieu of RequestScreenContext.
-  MOCK_METHOD2(DoRequestScreenContext,
-               void(const gfx::Rect&, base::OnceClosure*));
+  MOCK_METHOD3(DoRequestScreenContext,
+               void(const gfx::Rect&, bool from_user, base::OnceClosure*));
 
   // Note: We can't mock RequestScreenContext directly because of the move
   // semantics required around base::OnceClosure. Instead, we route calls to a
   // mockable delegate method, DoRequestScreenContext.
   void RequestScreenContext(const gfx::Rect& region,
+                            bool from_user,
                             base::OnceClosure callback) override {
-    DoRequestScreenContext(region, &callback);
+    DoRequestScreenContext(region, from_user, &callback);
   }
 
  private: