Show Quick Answers network error message.

This CL includes:
- Notify Quick Answers client when there is a network error.
- Show an error message when notified that there is a error. (UI and
message is not final).

Bug: 1047042
Test: Unit tests
Change-Id: I2e9c3618ef1965d2fe94cc10b04a9851fc7f366a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2029246Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Li Lin <llin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737142}

chrome/browser/renderer_context_menu/quick_answers_menu_observer.cc

@@ -30,6 +30,7 @@ using chromeos::quick_answers::ResultType;
 // TODO(llin): Update the placeholder after finalizing on the design.
 constexpr char kLoadingPlaceholder[] = "Loading...";
 constexpr char kNoResult[] = "See result in Assistant";
+constexpr char kNetworkError[] = "Cannot connect to internet.";
 
 }  // namespace
 
@@ -151,6 +152,14 @@ void QuickAnswersMenuObserver::OnQuickAnswerReceived(
   quick_answer_ = std::move(quick_answer);
 }
 
+void QuickAnswersMenuObserver::OnNetworkError() {
+  proxy_->UpdateMenuItem(IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_ANSWER,
+                         /*enabled=*/false,
+                         /*hidden=*/false,
+                         /*title=*/base::UTF8ToUTF16(kNetworkError));
+  quick_answer_received_time_ = base::TimeTicks::Now();
+}
+
 void QuickAnswersMenuObserver::OnEligibilityChanged(bool eligible) {
   is_eligible_ = eligible;
 }

chrome/browser/renderer_context_menu/quick_answers_menu_observer.h

@@ -39,6 +39,7 @@ class QuickAnswersMenuObserver
   void OnQuickAnswerReceived(
       std::unique_ptr<chromeos::quick_answers::QuickAnswer> answer) override;
   void OnEligibilityChanged(bool eligible) override;
+  void OnNetworkError() override;
 
   void SetQuickAnswerClientForTesting(
       std::unique_ptr<chromeos::quick_answers::QuickAnswersClient>

chrome/browser/renderer_context_menu/quick_answers_menu_observer_browsertest.cc

@@ -253,3 +253,27 @@ IN_PROC_BROWSER_TEST_F(QuickAnswersMenuObserverTest, FeatureIneligible) {
   // Verify that no Quick Answer menu items shown.
   ASSERT_EQ(0u, menu()->GetMenuSize());
 }
+
+IN_PROC_BROWSER_TEST_F(QuickAnswersMenuObserverTest, NetworkError) {
+  MockQuickAnswerClient();
+  InitMenu();
+
+  observer_->OnNetworkError();
+
+  // Verify that quick answer menu items is showing.
+  ASSERT_EQ(3u, menu()->GetMenuSize());
+
+  // Verify the query menu item.
+  VerifyMenuItems(
+      /*index=*/0,
+      /*command_id=*/IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_QUERY,
+      /*expected_title=*/"sel",
+      /*enabled=*/true);
+
+  // Verify the answer menu item.
+  VerifyMenuItems(
+      /*index=*/1,
+      /*command_id=*/IDC_CONTENT_CONTEXT_QUICK_ANSWERS_INLINE_ANSWER,
+      /*expected_title=*/"Cannot connect to internet.",
+      /*enabled=*/false);
+}

chromeos/components/quick_answers/quick_answers_client.cc

@@ -93,10 +93,8 @@ void QuickAnswersClient::SendRequest(
   delegate_->OnRequestPreprocessFinish(processed_request);
 
   // Load and parse search result.
-  search_results_loader_ = std::make_unique<SearchResultLoader>(
-      url_loader_factory_,
-      base::BindOnce(&QuickAnswersClient::OnQuickAnswerReceived,
-                     base::Unretained(this)));
+  search_results_loader_ =
+      std::make_unique<SearchResultLoader>(url_loader_factory_, this);
   search_results_loader_->Fetch(processed_request.selected_text);
 }
 
@@ -113,6 +111,11 @@ void QuickAnswersClient::NotifyEligibilityChanged() {
   }
 }
 
+void QuickAnswersClient::OnNetworkError() {
+  DCHECK(delegate_);
+  delegate_->OnNetworkError();
+}
+
 void QuickAnswersClient::OnQuickAnswerReceived(
     std::unique_ptr<QuickAnswer> quick_answer) {
   DCHECK(delegate_);

chromeos/components/quick_answers/quick_answers_client.h

@@ -25,7 +25,9 @@ struct QuickAnswer;
 struct QuickAnswersRequest;
 
 // Quick answers client to load and parse quick answer results.
-class QuickAnswersClient : public ash::AssistantStateObserver {
+class QuickAnswersClient
+    : public ash::AssistantStateObserver,
+      public SearchResultLoader::SearchResultLoaderDelegate {
  public:
   // A delegate interface for the QuickAnswersClient.
   class QuickAnswersDelegate {
@@ -45,6 +47,9 @@ class QuickAnswersClient : public ash::AssistantStateObserver {
     // Invoked when feature eligibility changed.
     virtual void OnEligibilityChanged(bool eligible) {}
 
+    // Invoked when there is a network error.
+    virtual void OnNetworkError() {}
+
    protected:
     QuickAnswersDelegate() = default;
     virtual ~QuickAnswersDelegate() = default;
@@ -66,11 +71,15 @@ class QuickAnswersClient : public ash::AssistantStateObserver {
   void OnLocaleChanged(const std::string& locale) override;
   void OnAssistantStateDestroyed() override;
 
+  // SearchResultLoaderDelegate:
+  void OnNetworkError() override;
+  void OnQuickAnswerReceived(
+      std::unique_ptr<QuickAnswer> quick_answer) override;
+
   // Send a quick answer request. Virtual for testing.
   virtual void SendRequest(const QuickAnswersRequest& quick_answers_request);
 
  private:
-  void OnQuickAnswerReceived(std::unique_ptr<QuickAnswer> quick_answer);
   void NotifyEligibilityChanged();
 
   network::mojom::URLLoaderFactory* url_loader_factory_ = nullptr;

chromeos/components/quick_answers/quick_answers_client_unittest.cc

@@ -34,6 +34,7 @@ class MockQuickAnswersDelegate
   MOCK_METHOD1(OnQuickAnswerReceived, void(std::unique_ptr<QuickAnswer>));
   MOCK_METHOD1(OnRequestPreprocessFinish, void(const QuickAnswersRequest&));
   MOCK_METHOD1(OnEligibilityChanged, void(bool));
+  MOCK_METHOD0(OnNetworkError, void());
 };
 
 }  // namespace
@@ -178,6 +179,14 @@ TEST_F(QuickAnswersClientTest, UnsupportedLocale) {
       /*assistant_state=*/ash::mojom::AssistantAllowedState::ALLOWED,
       /*locale=*/"en-GB");
 }
+
+TEST_F(QuickAnswersClientTest, NetworkError) {
+  // Verify that OnNetworkError is called.
+  EXPECT_CALL(*mock_delegate_, OnNetworkError());
+  EXPECT_CALL(*mock_delegate_, OnQuickAnswerReceived(::testing::_)).Times(0);
+
+  client_->OnNetworkError();
+}
 // TODO(b/144800297): Add more unit tests for sending request.
 
 }  // namespace quick_answers

chromeos/components/quick_answers/search_result_loader.cc

@@ -101,11 +101,8 @@ GURL BuildRequestUrl(const std::string& selected_text) {
 }  // namespace
 
 SearchResultLoader::SearchResultLoader(URLLoaderFactory* url_loader_factory,
-                                       CompleteCallback complete_callback) {
-  complete_callback_ = std::move(complete_callback);
-
-  network_loader_factory_ = url_loader_factory;
-}
+                                       SearchResultLoaderDelegate* delegate)
+    : network_loader_factory_(url_loader_factory), delegate_(delegate) {}
 
 SearchResultLoader::~SearchResultLoader() = default;
 
@@ -133,7 +130,7 @@ void SearchResultLoader::OnSimpleURLLoaderComplete(
   if (!response_body || loader_->NetError() != net::OK ||
       !loader_->ResponseInfo() || !loader_->ResponseInfo()->headers) {
     RecordLoadingStatus(LoadStatus::kNetworkError, duration);
-    std::move(complete_callback_).Run(/*quick_answer=*/nullptr);
+    delegate_->OnNetworkError();
     return;
   }
 
@@ -153,7 +150,7 @@ void SearchResultLoader::OnResultParserComplete(
     RecordLoadingStatus(LoadStatus::kSuccess, duration);
     RecordResult(quick_answer->result_type, duration);
   }
-  std::move(complete_callback_).Run(std::move(quick_answer));
+  delegate_->OnQuickAnswerReceived(std::move(quick_answer));
 }
 
 }  // namespace quick_answers

chromeos/components/quick_answers/search_result_loader.h

@@ -8,7 +8,6 @@
 #include <memory>
 #include <string>
 
-#include "base/callback.h"
 #include "chromeos/components/quick_answers/search_result_parsers/search_response_parser.h"
 
 namespace network {
@@ -26,21 +25,37 @@ struct QuickAnswer;
 
 class SearchResultLoader {
  public:
-  // Callback used when downloading of |quick_answer| is complete.
-  // Note that |proactive_suggestions| may be |nullptr|.
-  using CompleteCallback =
-      base::OnceCallback<void(std::unique_ptr<QuickAnswer> quick_answer)>;
+  // A delegate interface for the SearchResultLoader.
+  class SearchResultLoaderDelegate {
+   public:
+    SearchResultLoaderDelegate(const SearchResultLoaderDelegate&) = delete;
+    SearchResultLoaderDelegate& operator=(const SearchResultLoaderDelegate&) =
+        delete;
+
+    // Invoked when there is a network error.
+    virtual void OnNetworkError() {}
+
+    // Invoked when the |quick_answer| is received. Note that |quick_answer| may
+    // be |nullptr| if no answer found for the selected content.
+    virtual void OnQuickAnswerReceived(
+        std::unique_ptr<QuickAnswer> quick_answer) {}
+
+   protected:
+    SearchResultLoaderDelegate() = default;
+    virtual ~SearchResultLoaderDelegate() = default;
+  };
 
   SearchResultLoader(network::mojom::URLLoaderFactory* url_loader_factory,
-                     CompleteCallback complete_callback);
+                     SearchResultLoaderDelegate* delegate);
   ~SearchResultLoader();
 
   SearchResultLoader(const SearchResultLoader&) = delete;
   SearchResultLoader& operator=(const SearchResultLoader&) = delete;
 
-  // Starts downloading of |proactive_suggestions| associated with |url_|,
-  // running |complete_callback| when finished. Note that this method should be
-  // called only once per ProactiveSuggestionsLoader instance.
+  // Starts downloading of |quick_answers| associated with |selected_text|,
+  // calling |SearchResultLoaderDelegate| methods when finished.
+  // Note that delegate methods should be called only once per
+  // SearchResultLoader instance.
   void Fetch(const std::string& selected_text);
 
  private:
@@ -50,7 +65,7 @@ class SearchResultLoader {
   std::unique_ptr<SearchResponseParser> search_response_parser_;
   network::mojom::URLLoaderFactory* network_loader_factory_;
   std::unique_ptr<network::SimpleURLLoader> loader_;
-  CompleteCallback complete_callback_;
+  SearchResultLoaderDelegate* const delegate_;
 
   // Time when the query is issued.
   base::TimeTicks fetch_start_time_;