Only show consent view if there is a supported intent generated.

Bug: b/158798335
Test: unit tests
Change-Id: If70621c33e7b6f6da85e8ed570b3c412874503d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2242574
Commit-Queue: Li Lin <llin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779078}

ash/quick_answers/quick_answers_controller_impl.cc

@@ -60,30 +60,18 @@ void QuickAnswersControllerImpl::MaybeShowQuickAnswers(
   query_ = title;
   context_ = context;
 
-  // Show user-consent notice informing user about the feature if required.
-  if (consent_controller_->ShouldShowConsent()) {
-    if (!quick_answers_ui_controller_->is_showing_user_consent_view()) {
-      quick_answers_ui_controller_->CreateUserConsentView(anchor_bounds);
-      consent_controller_->StartConsent();
-    }
-
-    // Quick-Answers will only be displayed after explicit or tacit consent is
-    // obtained.
-    return;
-  }
-
-  if (!chromeos::features::IsQuickAnswersTextAnnotatorEnabled()) {
-    // If text annotator is not enabled, always shows quick answers view with
-    // placeholder. Otherwise, only shows quick answers view if the predicted
-    // intent is not |kUnknown| at |OnRequestPreprocessFinish|.
+  QuickAnswersRequest request = BuildRequest();
+  if (chromeos::features::IsQuickAnswersTextAnnotatorEnabled()) {
+    // Send the request for preprocessing. Only shows quick answers view if the
+    // predicted intent is not |kUnknown| at |OnRequestPreprocessFinish|.
+    quick_answers_client_->SendRequestForPreprocessing(request);
+  } else if (!MaybeShowUserConsent()) {
+    // Text annotator is not enabled and consent view is not showing, shows
+    // quick answers view with placeholder and send the request.
     quick_answers_ui_controller_->CreateQuickAnswersView(anchor_bounds, title_,
                                                          query_);
+    quick_answers_client_->SendRequest(request);
   }
-
-  QuickAnswersRequest request;
-  request.selected_text = title;
-  request.context = context;
-  quick_answers_client_->SendRequest(request);
 }
 
 void QuickAnswersControllerImpl::DismissQuickAnswers(bool is_active) {
@@ -141,20 +129,28 @@ void QuickAnswersControllerImpl::OnRequestPreprocessFinished(
     return;
   }
 
+  if (processed_request.preprocessed_output.intent_type ==
+      chromeos::quick_answers::IntentType::kUnknown) {
+    return;
+  }
+
   query_ = processed_request.preprocessed_output.query;
   title_ = processed_request.preprocessed_output.intent_text;
 
-  if (processed_request.preprocessed_output.intent_type !=
-      chromeos::quick_answers::IntentType::kUnknown) {
+  if (!MaybeShowUserConsent()) {
     quick_answers_ui_controller_->CreateQuickAnswersView(anchor_bounds_, title_,
                                                          query_);
+    quick_answers_client_->FetchQuickAnswers(processed_request);
   }
 }
 
 void QuickAnswersControllerImpl::OnRetryQuickAnswersRequest() {
-  QuickAnswersRequest request;
-  request.selected_text = query_;
-  quick_answers_client_->SendRequest(request);
+  QuickAnswersRequest request = BuildRequest();
+  if (chromeos::features::IsQuickAnswersTextAnnotatorEnabled()) {
+    quick_answers_client_->SendRequestForPreprocessing(request);
+  } else {
+    quick_answers_client_->SendRequest(request);
+  }
 }
 
 void QuickAnswersControllerImpl::OnQuickAnswerClick() {
@@ -196,4 +192,22 @@ void QuickAnswersControllerImpl::MaybeDismissQuickAnswersConsent() {
   quick_answers_ui_controller_->CloseUserConsentView();
 }
 
+bool QuickAnswersControllerImpl::MaybeShowUserConsent() {
+  if (consent_controller_->ShouldShowConsent()) {
+    // Show user-consent notice informing user about the feature if required.
+    if (!quick_answers_ui_controller_->is_showing_user_consent_view()) {
+      quick_answers_ui_controller_->CreateUserConsentView(anchor_bounds_);
+      consent_controller_->StartConsent();
+    }
+    return true;
+  }
+  return false;
+}
+
+QuickAnswersRequest QuickAnswersControllerImpl::BuildRequest() {
+  QuickAnswersRequest request;
+  request.selected_text = title_;
+  request.context = context_;
+  return request;
+}
 }  // namespace ash

ash/quick_answers/quick_answers_controller_impl.h

@@ -87,6 +87,11 @@ class ASH_EXPORT QuickAnswersControllerImpl
  private:
   void MaybeDismissQuickAnswersConsent();
 
+  // Return true if the user consent view is showing.
+  bool MaybeShowUserConsent();
+
+  chromeos::quick_answers::QuickAnswersRequest BuildRequest();
+
   // Bounds of the anchor view.
   gfx::Rect anchor_bounds_;
 

chromeos/components/quick_answers/quick_answers_client.cc

@@ -125,13 +125,24 @@ void QuickAnswersClient::OnAssistantStateDestroyed() {
   assistant_state_ = nullptr;
 }
 
-void QuickAnswersClient::SendRequest(
+void QuickAnswersClient::SendRequestForPreprocessing(
     const QuickAnswersRequest& quick_answers_request) {
-  RecordSelectedTextLength(quick_answers_request.selected_text.length());
+  SendRequestInternal(quick_answers_request, /*skip_fetch=*/true);
+}
 
-  // Generate intent from |quick_answers_request|.
-  intent_generator_ = CreateIntentGenerator(quick_answers_request);
-  intent_generator_->GenerateIntent(quick_answers_request);
+void QuickAnswersClient::FetchQuickAnswers(
+    const QuickAnswersRequest& preprocessed_request) {
+  DCHECK(!preprocessed_request.preprocessed_output.query.empty());
+
+  result_loader_ =
+      CreateResultLoader(preprocessed_request.preprocessed_output.intent_type);
+  // Load and parse search result.
+  result_loader_->Fetch(preprocessed_request.preprocessed_output.query);
+}
+
+void QuickAnswersClient::SendRequest(
+    const QuickAnswersRequest& quick_answers_request) {
+  SendRequestInternal(quick_answers_request, /*skip_fetch=*/false);
 }
 
 void QuickAnswersClient::OnQuickAnswerClick(ResultType result_type) {
@@ -168,12 +179,13 @@ std::unique_ptr<ResultLoader> QuickAnswersClient::CreateResultLoader(
 }
 
 std::unique_ptr<IntentGenerator> QuickAnswersClient::CreateIntentGenerator(
-    const QuickAnswersRequest& request) {
+    const QuickAnswersRequest& request,
+    bool skip_fetch) {
   if (g_testing_intent_generator_factory_callback)
     return g_testing_intent_generator_factory_callback->Run();
   return std::make_unique<IntentGenerator>(
       base::BindOnce(&QuickAnswersClient::IntentGeneratorCallback,
-                     weak_factory_.GetWeakPtr(), request));
+                     weak_factory_.GetWeakPtr(), request, skip_fetch));
 }
 
 void QuickAnswersClient::OnNetworkError() {
@@ -188,8 +200,19 @@ void QuickAnswersClient::OnQuickAnswerReceived(
   delegate_->OnQuickAnswerReceived(std::move(quick_answer));
 }
 
+void QuickAnswersClient::SendRequestInternal(
+    const QuickAnswersRequest& quick_answers_request,
+    bool skip_fetch) {
+  RecordSelectedTextLength(quick_answers_request.selected_text.length());
+
+  // Generate intent from |quick_answers_request|.
+  intent_generator_ = CreateIntentGenerator(quick_answers_request, skip_fetch);
+  intent_generator_->GenerateIntent(quick_answers_request);
+}
+
 void QuickAnswersClient::IntentGeneratorCallback(
     const QuickAnswersRequest& quick_answers_request,
+    bool skip_fetch,
     const std::string& intent_text,
     IntentType intent_type) {
   DCHECK(delegate_);
@@ -210,9 +233,8 @@ void QuickAnswersClient::IntentGeneratorCallback(
     }
   }
 
-  result_loader_ = CreateResultLoader(intent_type);
-  // Load and parse search result.
-  result_loader_->Fetch(processed_request.preprocessed_output.query);
+  if (!skip_fetch)
+    FetchQuickAnswers(processed_request);
 }
 
 base::TimeDelta QuickAnswersClient::GetImpressionDuration() const {

chromeos/components/quick_answers/quick_answers_client.h

@@ -89,7 +89,13 @@ class QuickAnswersClient : public ash::AssistantStateObserver,
   void OnQuickAnswerReceived(
       std::unique_ptr<QuickAnswer> quick_answer) override;
 
-  // Send a quick answer request. Virtual for testing.
+  // Send a quick answer request for preprocessing only.
+  void SendRequestForPreprocessing(
+      const QuickAnswersRequest& quick_answers_request);
+  // Fetch quick answers result from the server.
+  void FetchQuickAnswers(const QuickAnswersRequest& processed_request);
+  // Send a quick answer request. The request is preprocessed before fetching
+  // the result from the server. Virtual for testing.
   virtual void SendRequest(const QuickAnswersRequest& quick_answers_request);
 
   // User clicks on the Quick Answers result. Virtual for testing.
@@ -118,10 +124,16 @@ class QuickAnswersClient : public ash::AssistantStateObserver,
 
   // Creates an |IntentGenerator| instance.
   std::unique_ptr<IntentGenerator> CreateIntentGenerator(
-      const QuickAnswersRequest& request);
+      const QuickAnswersRequest& request,
+      bool skip_fetch);
 
   void NotifyEligibilityChanged();
+  // Preprocesses the |QuickAnswersRequest| and fetch quick answers result. Only
+  // preprocesses the request and skip fetching result if |skip_fetch| is true.
+  void SendRequestInternal(const QuickAnswersRequest& quick_answers_request,
+                           bool skip_fetch);
   void IntentGeneratorCallback(const QuickAnswersRequest& quick_answers_request,
+                               bool skip_fetch,
                                const std::string& intent_text,
                                IntentType intent_type);
   base::TimeDelta GetImpressionDuration() const;

chromeos/components/quick_answers/quick_answers_client_unittest.cc

@@ -301,8 +301,8 @@ TEST_F(QuickAnswersClientTest, SendRequest) {
       &result_loader_factory_callback_);
 
   client_->SendRequest(*quick_answers_request);
-  client_->IntentGeneratorCallback(*quick_answers_request, "sel",
-                                   IntentType::kDictionary);
+  client_->IntentGeneratorCallback(*quick_answers_request, /*skip_fetch=*/false,
+                                   "sel", IntentType::kDictionary);
 
   std::unique_ptr<QuickAnswer> quick_answer = std::make_unique<QuickAnswer>();
   quick_answer->primary_answer = "answer";
@@ -311,6 +311,40 @@ TEST_F(QuickAnswersClientTest, SendRequest) {
   client_->OnQuickAnswerReceived(std::move(quick_answer));
 }
 
+TEST_F(QuickAnswersClientTest, SendRequestForPreprocessing) {
+  std::unique_ptr<QuickAnswersRequest> quick_answers_request =
+      std::make_unique<QuickAnswersRequest>();
+  quick_answers_request->selected_text = "sel";
+
+  // Verify that |GenerateIntent| is called.
+  EXPECT_CALL(*mock_intent_generator_,
+              GenerateIntent(QuickAnswersRequestEqual(*quick_answers_request)));
+  QuickAnswersClient::SetIntentGeneratorFactoryForTesting(
+      &intent_generator_factory_callback_);
+
+  mock_result_loader_ =
+      std::make_unique<MockResultLoader>(&test_url_loader_factory_, nullptr);
+  EXPECT_CALL(*mock_result_loader_, Fetch(::testing::_)).Times(0);
+  QuickAnswersClient::SetResultLoaderFactoryForTesting(
+      &result_loader_factory_callback_);
+
+  client_->SendRequestForPreprocessing(*quick_answers_request);
+}
+
+TEST_F(QuickAnswersClientTest, FetchQuickAnswers) {
+  std::unique_ptr<QuickAnswersRequest> quick_answers_request =
+      std::make_unique<QuickAnswersRequest>();
+  quick_answers_request->preprocessed_output.query = "Define:sel";
+
+  mock_result_loader_ =
+      std::make_unique<MockResultLoader>(&test_url_loader_factory_, nullptr);
+  EXPECT_CALL(*mock_result_loader_, Fetch(::testing::Eq("Define:sel")));
+  QuickAnswersClient::SetResultLoaderFactoryForTesting(
+      &result_loader_factory_callback_);
+
+  client_->FetchQuickAnswers(*quick_answers_request);
+}
+
 TEST_F(QuickAnswersClientTest, NotSendRequestForUnknownIntent) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
@@ -326,8 +360,8 @@ TEST_F(QuickAnswersClientTest, NotSendRequestForUnknownIntent) {
   QuickAnswersClient::SetResultLoaderFactoryForTesting(
       &result_loader_factory_callback_);
 
-  client_->IntentGeneratorCallback(*quick_answers_request, "sel",
-                                   IntentType::kUnknown);
+  client_->IntentGeneratorCallback(*quick_answers_request, /*skip_fetch=*/false,
+                                   "sel", IntentType::kUnknown);
 }
 
 TEST_F(QuickAnswersClientTest, PreprocessDefinitionIntent) {
@@ -348,8 +382,8 @@ TEST_F(QuickAnswersClientTest, PreprocessDefinitionIntent) {
               OnRequestPreprocessFinished(
                   QuickAnswersRequestWithOutputEqual(*processed_request)));
 
-  client_->IntentGeneratorCallback(*quick_answers_request, "unfathomable",
-                                   IntentType::kDictionary);
+  client_->IntentGeneratorCallback(*quick_answers_request, /*skip_fetch=*/false,
+                                   "unfathomable", IntentType::kDictionary);
 }
 
 TEST_F(QuickAnswersClientTest, PreprocessTranslationIntent) {
@@ -370,8 +404,8 @@ TEST_F(QuickAnswersClientTest, PreprocessTranslationIntent) {
               OnRequestPreprocessFinished(
                   QuickAnswersRequestWithOutputEqual(*processed_request)));
 
-  client_->IntentGeneratorCallback(*quick_answers_request, "intent text",
-                                   IntentType::kTranslation);
+  client_->IntentGeneratorCallback(*quick_answers_request, /*skip_fetch=*/false,
+                                   "intent text", IntentType::kTranslation);
 }
 
 TEST_F(QuickAnswersClientTest, PreprocessUnitConversionIntent) {
@@ -392,8 +426,8 @@ TEST_F(QuickAnswersClientTest, PreprocessUnitConversionIntent) {
               OnRequestPreprocessFinished(
                   QuickAnswersRequestWithOutputEqual(*processed_request)));
 
-  client_->IntentGeneratorCallback(*quick_answers_request, "20ft",
-                                   IntentType::kUnit);
+  client_->IntentGeneratorCallback(*quick_answers_request, /*skip_fetch=*/false,
+                                   "20ft", IntentType::kUnit);
 }
 
 }  // namespace quick_answers