Don't always dismiss app list view when opening ChromeSearchResult.

Problem:
To add polish, SearchController closes the app list view when opening
search results. Newly added Assistant search results (surfaced as
launcher chips) often desire to stay in launcher(-embedded Assistant)
UI which is broken by this behavior.

Solution:
Make it optional whether SearchController should dismiss app list view
eagerly or not.

Bug: b:154152631
Change-Id: Ic37220b4a8bddb45277cbce4544cfae8ac69be56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2163853
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763544}

chrome/browser/ui/app_list/search/assistant_search_provider.cc

@@ -80,6 +80,12 @@ class AssistantSearchResult : public ChromeSearchResult {
         ash::kAssistantIcon,
         ash::AppListConfig::instance().suggestion_chip_icon_dimension(),
         gfx::kPlaceholderColor));
+
+    // If |action_url_| is an Assistant deep link, odds are we'll be going to
+    // launcher embedded Assistant UI when opening this result so we need to
+    // make sure that the app list view is *not* eagerly dismissed.
+    if (ash::assistant::util::IsDeepLinkUrl(action_url_))
+      set_dismiss_view_on_open(false);
   }
 
   AssistantSearchResult(const AssistantSearchResult&) = delete;
@@ -92,7 +98,6 @@ class AssistantSearchResult : public ChromeSearchResult {
     return ash::SearchResultType::ASSISTANT;
   }
 
-  // TODO(b:154152631): Prevent eager dismissal of launcher when opening.
   void Open(int event_flags) override {
     // Opening of |action_url_| is delegated to the Assistant controller as only
     // the Assistant controller knows how to handle Assistant deep links.

chrome/browser/ui/app_list/search/assistant_search_provider_unittest.cc

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "ash/assistant/model/assistant_suggestions_model.h"
+#include "ash/assistant/util/deep_link_util.h"
 #include "ash/public/cpp/app_list/app_list_config.h"
 #include "ash/public/cpp/assistant/assistant_state.h"
 #include "ash/public/cpp/assistant/controller/assistant_suggestions_controller.h"
@@ -57,6 +58,9 @@ class Expect {
   Expect& Matches(const AssistantSuggestion& starter) {
     EXPECT_EQ(r_.id(), "googleassistant://" + starter.id.ToString());
     EXPECT_EQ(r_.title(), base::UTF8ToUTF16(starter.text));
+    EXPECT_EQ(r_.dismiss_view_on_open(),
+              !ash::assistant::util::IsDeepLinkUrl(starter.action_url) &&
+                  !starter.action_url.is_empty());
     return *this;
   }
 

chrome/browser/ui/app_list/search/chrome_search_result.h

@@ -116,6 +116,11 @@ class ChromeSearchResult {
   double relevance() const { return relevance_; }
   void set_relevance(double relevance) { relevance_ = relevance; }
 
+  bool dismiss_view_on_open() const { return dismiss_view_on_open_; }
+  void set_dismiss_view_on_open(bool dismiss_view_on_open) {
+    dismiss_view_on_open_ = dismiss_view_on_open;
+  }
+
   // Invokes a custom action on the result. It does nothing by default.
   virtual void InvokeAction(int action_index, int event_flags);
 
@@ -167,6 +172,13 @@ class ChromeSearchResult {
   // sorted order, group multiplier and group boost.
   double relevance_ = 0;
 
+  // More often than not, calling Open() on a ChromeSearchResult will cause the
+  // app list view to be closed as a side effect. Because opening apps can take
+  // some time, the app list view is eagerly dismissed by default after invoking
+  // Open() for added polish. Some ChromeSearchResults may not appreciate this
+  // behavior so it can be disabled as needed.
+  bool dismiss_view_on_open_ = true;
+
   std::unique_ptr<ash::SearchResultMetadata> metadata_;
 
   AppListModelUpdater* model_updater_ = nullptr;

chrome/browser/ui/app_list/search/search_controller.cc

@@ -113,10 +113,12 @@ void SearchController::OpenResult(ChromeSearchResult* result, int event_flags) {
 
   result->Open(event_flags);
 
-  // Launching apps can take some time. It looks nicer to dismiss the app list.
-  // Do not close app list for home launcher.
-  if (!ash::TabletMode::Get() || !ash::TabletMode::Get()->InTabletMode())
+  // Launching apps can take some time. It looks nicer to eagerly dismiss the
+  // app list if |result| permits it. Do not close app list for home launcher.
+  if (result->dismiss_view_on_open() &&
+      (!ash::TabletMode::Get() || !ash::TabletMode::Get()->InTabletMode())) {
     list_controller_->DismissView();
+  }
 }
 
 void SearchController::InvokeResultAction(ChromeSearchResult* result,

chrome/browser/ui/app_list/search/search_controller_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/app_list/search/search_controller.h"
+
+#include <memory>
+#include <vector>
+
+#include "ash/public/cpp/tablet_mode.h"
+#include "ash/public/cpp/test/shell_test_api.h"
+#include "chrome/browser/ui/app_list/search/chrome_search_result.h"
+#include "chrome/browser/ui/app_list/test/test_app_list_controller_delegate.h"
+#include "chrome/test/base/chrome_ash_test_base.h"
+
+namespace app_list {
+namespace test {
+
+using ::test::TestAppListControllerDelegate;
+
+// TestSearchResult ------------------------------------------------------------
+
+class TestSearchResult : public ChromeSearchResult {
+ public:
+  TestSearchResult() = default;
+  TestSearchResult(const TestSearchResult&) = delete;
+  TestSearchResult& operator=(const TestSearchResult&) = delete;
+  ~TestSearchResult() override = default;
+
+ private:
+  // ChromeSearchResult:
+  ash::SearchResultType GetSearchResultType() const override {
+    return ash::SearchResultType::SEARCH_RESULT_TYPE_BOUNDARY;
+  }
+
+  void Open(int event_flags) override { NOTIMPLEMENTED(); }
+};
+
+// SearchControllerTest --------------------------------------------------------
+
+class SearchControllerTest : public ChromeAshTestBase {
+ public:
+  SearchControllerTest() = default;
+  SearchControllerTest(const SearchControllerTest&) = delete;
+  SearchControllerTest& operator=(const SearchControllerTest&) = delete;
+  ~SearchControllerTest() override = default;
+
+  SearchController& search_controller() { return search_controller_; }
+  TestAppListControllerDelegate& list_controller() { return list_controller_; }
+
+ private:
+  TestAppListControllerDelegate list_controller_;
+  SearchController search_controller_{/*model_updater=*/nullptr,
+                                      &list_controller_, /*profile=*/nullptr};
+};
+
+// Tests -----------------------------------------------------------------------
+
+TEST_F(SearchControllerTest, ShouldConditionallyDismissViewWhenOpeningResult) {
+  struct TestCase {
+    bool is_tablet_mode = false;
+    bool request_to_dismiss_view = false;
+    bool expect_to_dismiss_view = false;
+  };
+
+  std::vector<TestCase> test_cases;
+
+  test_cases.push_back({/*is_tablet_mode=*/false,
+                        /*request_to_dismiss_view=*/false,
+                        /*expect_to_dismiss_view=*/false});
+
+  test_cases.push_back({/*is_tablet_mode=*/true,
+                        /*request_to_dismiss_view=*/false,
+                        /*expect_to_dismiss_view=*/false});
+
+  test_cases.push_back({/*is_tablet_mode=*/true,
+                        /*request_to_dismiss_view=*/true,
+                        /*expect_to_dismiss_view=*/false});
+
+  test_cases.push_back({/*is_tablet_mode=*/false,
+                        /*request_to_dismiss_view=*/true,
+                        /*expect_to_dismiss_view=*/true});
+
+  auto result = std::make_unique<TestSearchResult>();
+  for (auto& test_case : test_cases) {
+    ash::ShellTestApi().SetTabletModeEnabledForTest(test_case.is_tablet_mode);
+
+    result->set_dismiss_view_on_open(test_case.request_to_dismiss_view);
+    search_controller().OpenResult(result.get(), /*event_flags=*/0);
+
+    EXPECT_EQ(test_case.expect_to_dismiss_view,
+              list_controller().did_dismiss_view());
+
+    list_controller().Reset();
+  }
+}
+
+}  // namespace test
+}  // namespace app_list

chrome/browser/ui/app_list/test/test_app_list_controller_delegate.cc

@@ -21,7 +21,9 @@ int64_t TestAppListControllerDelegate::GetAppListDisplayId() {
   return display::kInvalidDisplayId;
 }
 
-void TestAppListControllerDelegate::DismissView() {}
+void TestAppListControllerDelegate::DismissView() {
+  did_dismiss_view_ = true;
+}
 
 gfx::NativeWindow TestAppListControllerDelegate::GetAppListWindow() {
   return nullptr;
@@ -75,4 +77,9 @@ void TestAppListControllerDelegate::LaunchApp(
     int64_t display_id) {
 }
 
+void TestAppListControllerDelegate::Reset() {
+  did_dismiss_view_ = false;
+  last_opened_url_ = GURL();
+}
+
 }  // namespace test

chrome/browser/ui/app_list/test/test_app_list_controller_delegate.h

@@ -41,9 +41,13 @@ class TestAppListControllerDelegate : public AppListControllerDelegate {
                  int event_flags,
                  int64_t display_id) override;
 
+  void Reset();
+
+  bool did_dismiss_view() const { return did_dismiss_view_; }
   const GURL& last_opened_url() const { return last_opened_url_; }
 
  private:
+  bool did_dismiss_view_ = false;
   GURL last_opened_url_;
 };
 

chrome/test/BUILD.gn

@@ -4404,6 +4404,7 @@ test("unit_tests") {
       "../browser/ui/app_list/search/cros_action_history/cros_action_recorder_tab_tracker_unittest.cc",
       "../browser/ui/app_list/search/cros_action_history/cros_action_recorder_unittest.cc",
       "../browser/ui/app_list/search/launcher_search/launcher_search_icon_image_loader_unittest.cc",
+      "../browser/ui/app_list/search/search_controller_unittest.cc",
       "../browser/ui/app_list/search/search_result_ranker/app_launch_event_logger_unittest.cc",
       "../browser/ui/app_list/search/search_result_ranker/app_launch_predictor_test_util.h",
       "../browser/ui/app_list/search/search_result_ranker/app_launch_predictor_unittest.cc",