Use OnConversationTurnStartedInternal API in Assistant.

Previously we used the OnConversationTurnStarted API to notify us of
when an interaction was starting in LibAssistant. This API did not give
us any metadata about the interaction, so we couldn't match it with
confidence to one of our requests.

The new API exposes an ID through the |metadata| param. We can use that
ID to match an interaction request with a conversation turn. Once fully
wired up, this will allow us to stop pending a query before a conversation
turn starts and instead only do so within our interaction lifecycle. Once
lifecycle has been fixed, a number of downstream bugs will be resolved.

The CL to fix lifecycle will be a follow up. This CL only exposes the
interaction metadata that we will need in order to do so.

Bug: 941259, 973680
Change-Id: I32edf71251272aa21dc571df40f15b8de12cefba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1757425
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@{#689578}

ash/assistant/assistant_interaction_controller.cc

@@ -345,8 +345,10 @@ void AssistantInteractionController::OnCommittedQueryChanged(
   assistant::util::RecordAssistantQuerySource(assistant_query.source());
 }
 
+// TODO(dmblack): Set pending query from |metadata| and remove calls to set
+// pending query that occur outside of this method.
 void AssistantInteractionController::OnInteractionStarted(
-    bool is_voice_interaction) {
+    AssistantInteractionMetadataPtr metadata) {
   // Stop the interaction if the opt-in window is active.
   auto* assistant_setup = AssistantSetup::GetInstance();
   if (assistant_setup && assistant_setup->BounceOptInWindowIfActive()) {
@@ -354,6 +356,10 @@ void AssistantInteractionController::OnInteractionStarted(
     return;
   }
 
+  const bool is_voice_interaction =
+      chromeos::assistant::mojom::AssistantInteractionType::kVoice ==
+      metadata->type;
+
   if (is_voice_interaction) {
     // If the Assistant UI is not visible yet, and |is_voice_interaction| is
     // true, then it will be sure that Assistant is fired via OKG. ShowUi will

ash/assistant/assistant_interaction_controller.h

@@ -36,11 +36,13 @@ class AssistantInteractionController
       public AssistantViewDelegateObserver,
       public HighlighterController::Observer {
  public:
+  using AssistantInteractionMetadataPtr =
+      chromeos::assistant::mojom::AssistantInteractionMetadataPtr;
+  using AssistantInteractionResolution =
+      chromeos::assistant::mojom::AssistantInteractionResolution;
   using AssistantSuggestion = chromeos::assistant::mojom::AssistantSuggestion;
   using AssistantSuggestionPtr =
       chromeos::assistant::mojom::AssistantSuggestionPtr;
-  using AssistantInteractionResolution =
-      chromeos::assistant::mojom::AssistantInteractionResolution;
 
   explicit AssistantInteractionController(
       AssistantController* assistant_controller);
@@ -83,7 +85,7 @@ class AssistantInteractionController
   void OnHighlighterSelectionRecognized(const gfx::Rect& rect) override;
 
   // chromeos::assistant::mojom::AssistantInteractionSubscriber:
-  void OnInteractionStarted(bool is_voice_interaction) override;
+  void OnInteractionStarted(AssistantInteractionMetadataPtr metadata) override;
   void OnInteractionFinished(
       AssistantInteractionResolution resolution) override;
   void OnHtmlResponse(const std::string& response,

ash/assistant/test/assistant_ash_test_base.cc

@@ -61,13 +61,16 @@ const app_list::AssistantPageView* AssistantAshTestBase::page_view() const {
 
 void AssistantAshTestBase::MockAssistantInteractionWithResponse(
     const std::string& response_text) {
+  const std::string query = std::string("input text");
+
   // |controller_| will blackhole any server response if it hasn't sent
   // a request first, so we need to start by sending a request.
-  controller_->interaction_controller()->OnDialogPlateContentsCommitted(
-      "input text");
+  controller_->interaction_controller()->OnDialogPlateContentsCommitted(query);
   // Then the server can start an interaction and return the response.
   controller_->interaction_controller()->OnInteractionStarted(
-      /*is_voice_interaction=*/false);
+      chromeos::assistant::mojom::AssistantInteractionMetadata::New(
+          /*type=*/chromeos::assistant::mojom::AssistantInteractionType::kText,
+          /*query=*/query));
   controller_->interaction_controller()->OnTextResponse(response_text);
   controller_->interaction_controller()->OnInteractionFinished(
       AssistantInteractionController::AssistantInteractionResolution::kNormal);

chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h

@@ -498,10 +498,12 @@ class AutotestPrivateSendAssistantTextQueryFunction
   ~AutotestPrivateSendAssistantTextQueryFunction() override;
   ResponseAction Run() override;
 
-  using AssistantSuggestionPtr =
-      chromeos::assistant::mojom::AssistantSuggestionPtr;
+  using AssistantInteractionMetadataPtr =
+      chromeos::assistant::mojom::AssistantInteractionMetadataPtr;
   using AssistantInteractionResolution =
       chromeos::assistant::mojom::AssistantInteractionResolution;
+  using AssistantSuggestionPtr =
+      chromeos::assistant::mojom::AssistantSuggestionPtr;
 
   // chromeos::assistant::mojom::AssistantInteractionSubscriber:
   void OnHtmlResponse(const std::string& response,
@@ -509,7 +511,8 @@ class AutotestPrivateSendAssistantTextQueryFunction
   void OnTextResponse(const std::string& response) override;
   void OnInteractionFinished(
       AssistantInteractionResolution resolution) override;
-  void OnInteractionStarted(bool is_voice_interaction) override {}
+  void OnInteractionStarted(AssistantInteractionMetadataPtr metadata) override {
+  }
   void OnSuggestionsResponse(
       std::vector<AssistantSuggestionPtr> response) override {}
   void OnOpenUrlResponse(const GURL& url, bool in_background) override {}

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -13,6 +13,7 @@
 #include "base/i18n/rtl.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
 #include "base/unguessable_token.h"
@@ -404,6 +405,13 @@ void AssistantManagerServiceImpl::StartTextInteraction(const std::string& query,
         assistant_client::VoicelessOptions::Modality::TYPING_MODALITY;
   }
 
+  // Cache metadata about this interaction that can be resolved when the
+  // associated conversation turn starts in LibAssistant.
+  options.conversation_turn_id = base::NumberToString(next_interaction_id_++);
+  pending_interactions_[options.conversation_turn_id] =
+      mojom::AssistantInteractionMetadata::New(
+          /*type=*/mojom::AssistantInteractionType::kText, /*query=*/query);
+
   if (base::FeatureList::IsEnabled(
           assistant::features::kEnableTextQueriesWithClientDiscourseContext) &&
       assistant_extra_ && assistant_tree_) {
@@ -464,12 +472,13 @@ void AssistantManagerServiceImpl::DismissNotification(
       dismissed_interaction, "DismissNotification", options, [](auto) {});
 }
 
-void AssistantManagerServiceImpl::OnConversationTurnStarted(bool is_mic_open) {
+void AssistantManagerServiceImpl::OnConversationTurnStartedInternal(
+    const assistant_client::ConversationTurnMetadata& metadata) {
   service_->main_task_runner()->PostTask(
       FROM_HERE,
       base::BindOnce(
           &AssistantManagerServiceImpl::OnConversationTurnStartedOnMainThread,
-          weak_factory_.GetWeakPtr(), is_mic_open));
+          weak_factory_.GetWeakPtr(), metadata));
 }
 
 void AssistantManagerServiceImpl::OnConversationTurnFinished(
@@ -1198,13 +1207,27 @@ void AssistantManagerServiceImpl::MediaSessionMetadataChanged(
 }
 
 void AssistantManagerServiceImpl::OnConversationTurnStartedOnMainThread(
-    bool is_mic_open) {
+    const assistant_client::ConversationTurnMetadata& metadata) {
   platform_api_->GetAudioInputProvider()
       .GetAudioInput()
       .OnConversationTurnStarted();
 
-  interaction_subscribers_.ForAllPtrs([is_mic_open](auto* ptr) {
-    ptr->OnInteractionStarted(/*is_voice_interaction=*/is_mic_open);
+  // Retrieve the cached interaction metadata associated with this conversation
+  // turn or construct a new instance if there's no match in the cache.
+  mojom::AssistantInteractionMetadataPtr metadata_ptr;
+  auto it = pending_interactions_.find(metadata.id);
+  if (it != pending_interactions_.end()) {
+    metadata_ptr = std::move(it->second);
+    pending_interactions_.erase(it);
+  } else {
+    metadata_ptr = mojom::AssistantInteractionMetadata::New();
+    metadata_ptr->type = metadata.is_mic_open
+                             ? mojom::AssistantInteractionType::kVoice
+                             : mojom::AssistantInteractionType::kText;
+  }
+
+  interaction_subscribers_.ForAllPtrs([&metadata_ptr](auto* ptr) {
+    ptr->OnInteractionStarted(metadata_ptr->Clone());
   });
 }
 

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_IMPL_H_
 #define CHROMEOS_SERVICES_ASSISTANT_ASSISTANT_MANAGER_SERVICE_IMPL_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -153,7 +154,6 @@ class AssistantManagerServiceImpl
   void OnSpeechLevelUpdated(float speech_level) override;
 
   // assistant_client::ConversationStateListener overrides:
-  void OnConversationTurnStarted(bool is_mic_open) override;
   void OnConversationTurnFinished(
       assistant_client::ConversationStateListener::Resolution resolution)
       override;
@@ -168,6 +168,8 @@ class AssistantManagerServiceImpl
       const std::string& modify_setting_args_proto) override;
   bool IsSettingSupported(const std::string& setting_id) override;
   bool SupportsModifySettings() override;
+  void OnConversationTurnStartedInternal(
+      const assistant_client::ConversationTurnMetadata& metadata) override;
   void OnNotificationRemoved(const std::string& grouping_key) override;
   void OnCommunicationError(int error_code) override;
   // Last search source will be cleared after it is retrieved.
@@ -226,7 +228,8 @@ class AssistantManagerServiceImpl
 
   void HandleLaunchMediaIntentResponse(bool app_opened);
 
-  void OnConversationTurnStartedOnMainThread(bool is_mic_open);
+  void OnConversationTurnStartedOnMainThread(
+      const assistant_client::ConversationTurnMetadata& metadata);
   void OnConversationTurnFinishedOnMainThread(
       assistant_client::ConversationStateListener::Resolution resolution);
   void OnShowHtmlOnMainThread(const std::string& html,
@@ -320,6 +323,10 @@ class AssistantManagerServiceImpl
 
   base::Thread background_thread_;
 
+  int next_interaction_id_ = 1;
+  std::map<std::string, mojom::AssistantInteractionMetadataPtr>
+      pending_interactions_;
+
   bool receive_modify_settings_proto_response_ = false;
   bool receive_inline_response_ = false;
   std::string receive_url_response_;

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

@@ -137,14 +137,25 @@ interface Assistant {
   CreateTimer(mojo_base.mojom.TimeDelta duration);
 };
 
+// Describes an Assistant interaction.
+struct AssistantInteractionMetadata {
+  AssistantInteractionType type;
+  string query;
+};
+
+// Enumeration of possible Assistant interaction types.
+enum AssistantInteractionType {
+  kText,
+  kVoice,
+};
+
 // Subscribes to Assistant's interaction event. These events are server driven
 // in response to the user's direct interaction with the assistant. Responses
 // from the assistant may contain untrusted third-party content. Subscriber
 // implementations must be sure to handle the response data appropriately.
 interface AssistantInteractionSubscriber {
-  // Assistant interaction has started. In the event of a voice interaction,
-  // |is_voice_interaction| will be true. This implies that the mic is open.
-  OnInteractionStarted(bool is_voice_interaction);
+  // Assistant interaction has started.
+  OnInteractionStarted(AssistantInteractionMetadata metadata);
 
   // Assistant interaction has ended with the specified |resolution|.
   OnInteractionFinished(AssistantInteractionResolution resolution);