Enable Help App instead of Release Notes in suggestion chip.

This is based on the feature HelpAppReleaseNotes. It will have a
different launch URL and message than the Help App normally does. Has
the same logic for when it displays as the Release Notes PWA.

Bug: b/161755085
Change-Id: Ie3778178411001b8e9c99deffbae7231387b7ce8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2351480
Commit-Queue: Rachel Carpenter <carpenterr@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797575}

chrome/browser/ui/app_list/internal_app/internal_app_metadata.cc

@@ -20,6 +20,8 @@
 #include "chrome/browser/apps/app_service/app_service_metrics.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/chromeos/release_notes/release_notes_storage.h"
+#include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
 #include "chrome/browser/ui/app_list/app_list_client_impl.h"
@@ -69,13 +71,6 @@ const std::vector<InternalApp>& GetInternalAppListImpl(bool get_all,
             /*recommendable=*/true,
             /*searchable=*/false,
             /*show_in_launcher=*/false, apps::BuiltInAppName::kContinueReading,
-            /*searchable_string_resource_id=*/0},
-
-           {ash::kReleaseNotesAppId, IDS_RELEASE_NOTES_NOTIFICATION_TITLE,
-            IDR_RELEASE_NOTES_APP_192,
-            /*recommendable=*/true,
-            /*searchable=*/false,
-            /*show_in_launcher=*/false, apps::BuiltInAppName::kReleaseNotes,
             /*searchable_string_resource_id=*/0}});
 
   static base::NoDestructor<std::vector<InternalApp>> internal_app_list;
@@ -83,6 +78,17 @@ const std::vector<InternalApp>& GetInternalAppListImpl(bool get_all,
   internal_app_list->insert(internal_app_list->begin(),
                             internal_app_list_static->begin(),
                             internal_app_list_static->end());
+
+  if (!base::FeatureList::IsEnabled(chromeos::features::kHelpAppReleaseNotes)) {
+    internal_app_list->push_back(
+        {ash::kReleaseNotesAppId, IDS_RELEASE_NOTES_NOTIFICATION_TITLE,
+         IDR_RELEASE_NOTES_APP_192,
+         /*recommendable=*/true,
+         /*searchable=*/false,
+         /*show_in_launcher=*/false, apps::BuiltInAppName::kReleaseNotes,
+         /*searchable_string_resource_id=*/0});
+  }
+
   const bool add_discover_app =
       get_all || !chromeos::ProfileHelper::IsEphemeralUserProfile(profile);
   if (base::FeatureList::IsEnabled(chromeos::features::kDiscoverApp) &&
@@ -118,14 +124,21 @@ const std::vector<InternalApp>& GetInternalAppList(const Profile* profile) {
   return GetInternalAppListImpl(false, profile);
 }
 
-bool IsSuggestionChip(const std::string& app_id) {
-  // App IDs for internal apps which should only be shown as suggestion chips.
-  static const char* kSuggestionChipIds[] = {ash::kInternalAppIdContinueReading,
-                                             ash::kReleaseNotesAppId};
+bool IsSuggestionChip(const std::string& app_id, Profile* profile) {
+  if (base::LowerCaseEqualsASCII(app_id, ash::kInternalAppIdContinueReading))
+    return true;
 
-  for (size_t i = 0; i < base::size(kSuggestionChipIds); ++i) {
-    if (base::LowerCaseEqualsASCII(app_id, kSuggestionChipIds[i]))
+  if (!base::FeatureList::IsEnabled(chromeos::features::kHelpAppReleaseNotes)) {
+    if (base::LowerCaseEqualsASCII(app_id, ash::kReleaseNotesAppId))
       return true;
+  } else {
+    // We show the Help App as a release notes suggestion chip a certain
+    // number of times.
+    if (chromeos::ReleaseNotesStorage(profile).ShouldShowSuggestionChip() &&
+        base::LowerCaseEqualsASCII(app_id,
+                                   chromeos::default_web_apps::kHelpAppId)) {
+      return true;
+    }
   }
   return false;
 }

chrome/browser/ui/app_list/internal_app/internal_app_metadata.h

@@ -54,7 +54,7 @@ struct InternalApp {
 const std::vector<InternalApp>& GetInternalAppList(const Profile* profile);
 
 // Returns true if the app should only be shown as a suggestion chip.
-bool IsSuggestionChip(const std::string& app_id);
+bool IsSuggestionChip(const std::string& app_id, Profile* profile);
 
 // Returns InternalApp by |app_id|.
 // Returns nullptr if |app_id| does not correspond to an internal app.

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

@@ -9,9 +9,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
-#include "chrome/browser/chromeos/login/login_manager_test.h"
-#include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
-#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/app_list_client_impl.h"
@@ -19,8 +17,14 @@
 #include "chrome/browser/ui/app_list/search/chrome_search_result.h"
 #include "chrome/browser/ui/app_list/search/search_controller.h"
 #include "chrome/browser/ui/app_list/test/chrome_app_list_test_support.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/test/browser_test.h"
 
 namespace app_list {
@@ -30,13 +34,12 @@ namespace app_list {
 // results that would be displayed via the AppListModelUpdater. This class is
 // also intended as in-code documentation for how to create future app list
 // search integration tests.
-class AppListSearchBrowserTest : public chromeos::LoginManagerTest {
+class AppListSearchBrowserTest : public InProcessBrowserTest {
  public:
   using ResultType = ash::AppListSearchResultType;
+  using DisplayType = ash::SearchResultDisplayType;
 
-  AppListSearchBrowserTest() : LoginManagerTest() {
-    login_mixin_.AppendRegularUsers(1);
-  }
+  AppListSearchBrowserTest() {}
   ~AppListSearchBrowserTest() override = default;
 
   AppListSearchBrowserTest(const AppListSearchBrowserTest&) = delete;
@@ -110,39 +113,7 @@ class AppListSearchBrowserTest : public chromeos::LoginManagerTest {
   // Session helpers
   //----------------
 
-  Profile* GetProfile() {
-    auto* profile = ProfileManager::GetActiveUserProfile();
-    CHECK(profile);
-    return profile;
-  }
-
-  base::FilePath GetProfileDir() { return profile_dir_; }
-
-  void DoLogin() { LoginUser(login_mixin_.users()[0].account_id); }
-
-  //-----
-  // Misc
-  //-----
-
-  // LoginManagerTest:
-  bool SetUpUserDataDirectory() override {
-    base::FilePath user_data_dir;
-    base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
-    const std::string& email =
-        login_mixin_.users()[0].account_id.GetUserEmail();
-    const std::string user_id_hash =
-        chromeos::ProfileHelper::GetUserIdHashByUserIdForTesting(email);
-    profile_dir_ = user_data_dir.Append(
-        chromeos::ProfileHelper::GetUserProfileDir(user_id_hash));
-    base::CreateDirectory(profile_dir_);
-
-    return true;
-  }
-
- protected:
-  base::FilePath profile_dir_;
-
-  chromeos::LoginManagerMixin login_mixin_{&mixin_host_};
+  Profile* GetProfile() { return browser()->profile(); }
 };
 
 // Test fixture for OS settings search. This subclass exists because changing a
@@ -166,9 +137,27 @@ class OsSettingsSearchBrowserTest : public AppListSearchBrowserTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+// Test fixture for Release notes search. This subclass exists because changing
+// a feature flag has to be done in the constructor. Otherwise, it could use
+// AppListSearchBrowserTest directly.
+class ReleaseNotesSearchBrowserTest : public AppListSearchBrowserTest {
+ public:
+  ReleaseNotesSearchBrowserTest() : AppListSearchBrowserTest() {
+    scoped_feature_list_.InitWithFeatures(
+        {chromeos::features::kHelpAppReleaseNotes}, {});
+  }
+  ~ReleaseNotesSearchBrowserTest() override = default;
+
+  ReleaseNotesSearchBrowserTest(const ReleaseNotesSearchBrowserTest&) = delete;
+  ReleaseNotesSearchBrowserTest& operator=(
+      const ReleaseNotesSearchBrowserTest&) = delete;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
 // Simply tests that neither zero-state nor query-based search cause a crash.
 IN_PROC_BROWSER_TEST_F(AppListSearchBrowserTest, SearchDoesntCrash) {
-  DoLogin();
   // This won't catch everything, because not all providers run on all queries,
   // and so we can't wait for all providers to finish. Instead, we wait on one
   // app and one non-app provider. Note file search (kLauncher) is generally the
@@ -181,7 +170,9 @@ IN_PROC_BROWSER_TEST_F(AppListSearchBrowserTest, SearchDoesntCrash) {
 
 // Test that searching for "wifi" correctly returns a settings result for wifi.
 IN_PROC_BROWSER_TEST_F(OsSettingsSearchBrowserTest, AppListSearchForSettings) {
-  DoLogin();
+  web_app::WebAppProvider::Get(GetProfile())
+      ->system_web_app_manager()
+      .InstallSystemAppsForTesting();
   SearchAndWaitForProviders("wifi", {ResultType::kOsSettings});
 
   auto* result = FindResult("os-settings://networks?type=WiFi");
@@ -190,4 +181,52 @@ IN_PROC_BROWSER_TEST_F(OsSettingsSearchBrowserTest, AppListSearchForSettings) {
             "Wi-Fi networks, Network, Settings");
 }
 
+// Test that Help App shows up as Release notes if pref shows we have some times
+// left to show it.
+IN_PROC_BROWSER_TEST_F(ReleaseNotesSearchBrowserTest,
+                       AppListSearchHasSuggestionChip) {
+  web_app::WebAppProvider::Get(GetProfile())
+      ->system_web_app_manager()
+      .InstallSystemAppsForTesting();
+  GetProfile()->GetPrefs()->SetInteger(
+      prefs::kReleaseNotesSuggestionChipTimesLeftToShow, 1);
+
+  SearchAndWaitForProviders("",
+                            {ResultType::kInstalledApp, ResultType::kLauncher});
+
+  auto* result = FindResult(chromeos::default_web_apps::kHelpAppId);
+  ASSERT_TRUE(result);
+  // Has Release notes title.
+  EXPECT_EQ(base::UTF16ToASCII(result->title()),
+            "See what's new on your Chrome device");
+  // Displayed in first position.
+  EXPECT_EQ(result->position_priority(), 1.0f);
+  // Has override url defined for updates tab.
+  EXPECT_EQ(result->query_url(), GURL("chrome://help-app/updates"));
+  EXPECT_EQ(result->display_type(), DisplayType::kChip);
+}
+
+// Test that Help App shows up normally if pref shows we should no longer show
+// as suggestion chip.
+IN_PROC_BROWSER_TEST_F(ReleaseNotesSearchBrowserTest, AppListSearchHasApp) {
+  web_app::WebAppProvider::Get(GetProfile())
+      ->system_web_app_manager()
+      .InstallSystemAppsForTesting();
+  GetProfile()->GetPrefs()->SetInteger(
+      prefs::kReleaseNotesSuggestionChipTimesLeftToShow, 0);
+
+  SearchAndWaitForProviders("",
+                            {ResultType::kInstalledApp, ResultType::kLauncher});
+
+  auto* result = FindResult(chromeos::default_web_apps::kHelpAppId);
+  ASSERT_TRUE(result);
+  // Has regular app name as title.
+  EXPECT_EQ(base::UTF16ToASCII(result->title()), "Explore");
+  // No priority for position.
+  EXPECT_EQ(result->position_priority(), 0);
+  // No override url (will open app at default page).
+  EXPECT_FALSE(result->query_url().has_value());
+  EXPECT_EQ(result->display_type(), DisplayType::kTile);
+}
+
 }  // namespace app_list

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

@@ -33,6 +33,7 @@
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/extensions/gfx_utils.h"
 #include "chrome/browser/chromeos/release_notes/release_notes_storage.h"
+#include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/session_sync_service_factory.h"
@@ -42,11 +43,13 @@
 #include "chrome/browser/ui/app_list/search/app_service_app_result.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/app_search_result_ranker.h"
 #include "chrome/browser/ui/app_list/search/search_result_ranker/ranking_item_util.h"
+#include "chrome/grit/generated_resources.h"
 #include "chromeos/components/string_matching/fuzzy_tokenized_string_match.h"
 #include "chromeos/components/string_matching/tokenized_string.h"
 #include "chromeos/components/string_matching/tokenized_string_match.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync_sessions/session_sync_service.h"
+#include "ui/chromeos/devicetype_utils.h"
 
 namespace {
 
@@ -509,6 +512,18 @@ void AppSearchProvider::UpdateRecommendedResults(
         app->data_source()->CreateResult(app->id(), list_controller_, true);
     result->SetTitle(title);
 
+    if (app->id() == chromeos::default_web_apps::kHelpAppId) {
+      auto release_notes_storage =
+          std::make_unique<chromeos::ReleaseNotesStorage>(profile_);
+      // If we should show the release notes suggestion chip, change the title
+      // and url of the Help App. Otherwise leave as normal.
+      if (release_notes_storage->ShouldShowSuggestionChip()) {
+        result->SetTitle(ui::SubstituteChromeOSDeviceType(
+            IDS_RELEASE_NOTES_DEVICE_SPECIFIC_NOTIFICATION_TITLE));
+        result->SetQueryUrl(GURL("chrome://help-app/updates"));
+      }
+    }
+
     const auto find_in_app_list = id_to_app_list_index.find(app->id());
     const base::Time time = app->GetLastActivityTime();
 

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

@@ -12,6 +12,7 @@
 #include "chrome/browser/apps/app_service/app_service_proxy.h"
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/chromeos/release_notes/release_notes_storage.h"
+#include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/favicon/large_icon_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_client_impl.h"
@@ -19,6 +20,8 @@
 #include "chrome/browser/ui/app_list/app_service/app_service_context_menu.h"
 #include "chrome/browser/ui/app_list/internal_app/internal_app_metadata.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
+#include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/common/chrome_features.h"
 #include "components/favicon/core/large_icon_service.h"
 #include "components/services/app_service/public/cpp/app_update.h"
@@ -74,7 +77,7 @@ AppServiceAppResult::AppServiceAppResult(Profile* profile,
       break;
   }
 
-  if (IsSuggestionChip(id()))
+  if (IsSuggestionChip(id(), profile))
     HandleSuggestionChip(profile);
 }
 
@@ -154,6 +157,13 @@ void AppServiceAppResult::Launch(int event_flags,
   apps::AppServiceProxy* proxy =
       apps::AppServiceProxyFactory::GetForProfile(profile());
 
+  if (id() == chromeos::default_web_apps::kHelpAppId &&
+      query_url().has_value()) {
+    proxy->LaunchAppWithUrl(app_id(), event_flags, query_url().value(),
+                            launch_source, controller()->GetAppListDisplayId());
+    return;
+  }
+
   // For Chrome apps or Web apps, if it is non-platform app, it could be
   // selecting an existing delegate for the app, so call
   // ChromeLauncherController's ActivateApp interface. Platform apps or ARC
@@ -236,7 +246,9 @@ void AppServiceAppResult::HandleSuggestionChip(Profile* profile) {
   SetDisplayIndex(ash::SearchResultDisplayIndex::kFirstIndex);
   SetDisplayType(ash::SearchResultDisplayType::kChip);
 
-  if (id() == ash::kReleaseNotesAppId) {
+  // Either of these apps could be shown as the release notes suggestion chip.
+  if (id() == ash::kReleaseNotesAppId ||
+      id() == chromeos::default_web_apps::kHelpAppId) {
     SetNotifyVisibilityChange(true);
     // Make sure that if both Continue Reading and Release Notes are available,
     // Release Notes shows up first in the suggestion chip container.