Add browser test for system notifications for RunOnOsLogin

This CL adds a browser test for system notification for autostarted PWAs
as a follow-up to https://crrev.com/c/4632564.

Also changed the PreinstalledWebAppManager and the WebAppRunOnOsLoginManager to use AutoResets for testing (removed TODO from crbug.com/1434692).

Bug: b/279737385, b/293863792
Change-Id: Ica56ab3c7ee62ed3e46d8f7346ed43f7179dc79d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4727930
Commit-Queue: Sebastian Fiß <sfiss@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187257}

chrome/browser/chromeos/extensions/login_screen/login/cleanup/web_app_cleanup_handler_browsertest.cc

@@ -4,8 +4,10 @@
 
 #include "chrome/browser/chromeos/extensions/login_screen/login/cleanup/web_app_cleanup_handler.h"
 
+#include <memory>
 #include <string>
 
+#include "base/auto_reset.h"
 #include "base/test/test_future.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/web_applications/web_app_controller_browsertest.h"
@@ -47,14 +49,11 @@ constexpr char kApp4InstallURL[] = "https://example_url4.com/install";
 
 class WebAppCleanupHandlerBrowserTest : public WebAppControllerBrowserTest {
  protected:
-  WebAppCleanupHandlerBrowserTest() = default;
+  WebAppCleanupHandlerBrowserTest()
+      : skip_preinstalled_web_app_startup_(
+            PreinstalledWebAppManager::SkipStartupForTesting()) {}
   ~WebAppCleanupHandlerBrowserTest() override = default;
 
-  void SetUp() override {
-    PreinstalledWebAppManager::SkipStartupForTesting();
-    WebAppControllerBrowserTest::SetUp();
-  }
-
   AppId InstallWebApp(std::u16string title,
                       GURL start_url,
                       GURL install_url,
@@ -83,6 +82,7 @@ class WebAppCleanupHandlerBrowserTest : public WebAppControllerBrowserTest {
 
   WebAppRegistrar& registrar_unsafe() { return provider().registrar_unsafe(); }
 
+  base::AutoReset<bool> skip_preinstalled_web_app_startup_;
   chromeos::WebAppCleanupHandler web_app_cleanup_handler_;
 };
 

chrome/browser/ui/ash/shelf/chrome_shelf_controller_unittest.cc

@@ -504,7 +504,9 @@ class ChromeShelfControllerTestBase : public BrowserWithTestWindowTest,
                                       public apps::AppRegistryCache::Observer {
  protected:
   ChromeShelfControllerTestBase()
-      : BrowserWithTestWindowTest(Browser::TYPE_NORMAL) {}
+      : BrowserWithTestWindowTest(Browser::TYPE_NORMAL),
+        skip_preinstalled_web_app_startup_(
+            web_app::PreinstalledWebAppManager::SkipStartupForTesting()) {}
 
   void SetUp() override {
     base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
@@ -515,8 +517,6 @@ class ChromeShelfControllerTestBase : public BrowserWithTestWindowTest,
 
     ash::ConciergeClient::InitializeFake(/*fake_cicerone_client=*/nullptr);
 
-    web_app::PreinstalledWebAppManager::SkipStartupForTesting();
-
     app_list::AppListSyncableServiceFactory::SetUseInTesting(true);
 
     BrowserWithTestWindowTest::SetUp();
@@ -1407,6 +1407,8 @@ class ChromeShelfControllerTestBase : public BrowserWithTestWindowTest,
 
   PinAssertionMap pin_assertions_;
 
+  base::AutoReset<bool> skip_preinstalled_web_app_startup_;
+
   base::ScopedObservation<apps::AppRegistryCache,
                           apps::AppRegistryCache::Observer>
       app_registry_cache_observer_{this};

chrome/browser/ui/web_applications/web_app_run_on_os_login_notification.cc

@@ -28,9 +28,6 @@
 
 namespace {
 
-const char kRunOnOsLoginNotificationId[] = "run_on_os_login";
-const char kRunOnOsLoginNotifierId[] = "run_on_os_login_notifier";
-
 message_center::Notification CreateNotification(
     const std::vector<std::string>& app_names,
     base::WeakPtr<Profile> profile) {
@@ -54,12 +51,14 @@ message_center::Notification CreateNotification(
       l10n_util::GetStringUTF16(IDS_RUN_ON_OS_LOGIN_ENABLED_LEARN_MORE));
 
 #if (BUILDFLAG(IS_CHROMEOS_ASH))
-  message_center::NotifierId notifier_id = message_center::NotifierId(
-      message_center::NotifierType::SYSTEM_COMPONENT, kRunOnOsLoginNotifierId,
-      ash::NotificationCatalogName::kWebAppSettings);
+  message_center::NotifierId notifier_id =
+      message_center::NotifierId(message_center::NotifierType::SYSTEM_COMPONENT,
+                                 web_app::kRunOnOsLoginNotifierId,
+                                 ash::NotificationCatalogName::kWebAppSettings);
 #else
-  message_center::NotifierId notifier_id = message_center::NotifierId(
-      message_center::NotifierType::SYSTEM_COMPONENT, kRunOnOsLoginNotifierId);
+  message_center::NotifierId notifier_id =
+      message_center::NotifierId(message_center::NotifierType::SYSTEM_COMPONENT,
+                                 web_app::kRunOnOsLoginNotifierId);
 #endif  // (BUILDFLAG(IS_CHROMEOS_ASH))
 
   base::RepeatingClosure click_callback = base::BindRepeating(
@@ -78,7 +77,7 @@ message_center::Notification CreateNotification(
 
   message_center::Notification notification{
       message_center::NOTIFICATION_TYPE_SIMPLE,
-      std::string(kRunOnOsLoginNotificationId),
+      std::string(web_app::kRunOnOsLoginNotificationId),
       title,
       message,
       ui::ImageModel(),
@@ -96,6 +95,10 @@ message_center::Notification CreateNotification(
 }  // namespace
 
 namespace web_app {
+
+const char kRunOnOsLoginNotificationId[] = "run_on_os_login";
+const char kRunOnOsLoginNotifierId[] = "run_on_os_login_notifier";
+
 void DisplayRunOnOsLoginNotification(const std::vector<std::string>& app_names,
                                      base::WeakPtr<Profile> profile) {
   if (!profile) {

chrome/browser/ui/web_applications/web_app_run_on_os_login_notification.h

@@ -9,6 +9,10 @@
 #include "chrome/browser/profiles/profile.h"
 
 namespace web_app {
+
+extern const char kRunOnOsLoginNotificationId[];
+extern const char kRunOnOsLoginNotifierId[];
+
 void DisplayRunOnOsLoginNotification(const std::vector<std::string>& app_names,
                                      base::WeakPtr<Profile> profile);
 }  // namespace web_app

chrome/browser/web_applications/commands/dedupe_install_urls_command_unittest.cc

@@ -28,13 +28,16 @@ class DedupeInstallUrlsCommandTest : public WebAppTest {
  public:
   DedupeInstallUrlsCommandTest()
       : bypass_dependencies_(
-            PreinstalledWebAppManager::BypassAwaitingDependenciesForTesting()) {
-  }
+            PreinstalledWebAppManager::BypassAwaitingDependenciesForTesting()),
+        skip_preinstalled_web_app_startup_(
+            PreinstalledWebAppManager::SkipStartupForTesting()),
+        bypass_offline_manifest_requirement_(
+            PreinstalledWebAppManager::
+                BypassOfflineManifestRequirementForTesting()) {}
 
   void SetUp() override {
     WebAppTest::SetUp();
 
-    PreinstalledWebAppManager::SkipStartupForTesting();
     PreinstalledWebAppManager::BypassOfflineManifestRequirementForTesting();
 
     fake_web_contents_manager_ = static_cast<FakeWebContentsManager*>(
@@ -117,6 +120,8 @@ class DedupeInstallUrlsCommandTest : public WebAppTest {
   raw_ptr<FakeWebContentsManager, DisableDanglingPtrDetection>
       fake_web_contents_manager_;
   base::AutoReset<bool> bypass_dependencies_;
+  base::AutoReset<bool> skip_preinstalled_web_app_startup_;
+  base::AutoReset<bool> bypass_offline_manifest_requirement_;
 };
 
 TEST_F(DedupeInstallUrlsCommandTest,

chrome/browser/web_applications/preinstalled_web_app_manager.cc

@@ -597,8 +597,8 @@ void PreinstalledWebAppManager::RegisterProfilePrefs(
 }
 
 // static
-void PreinstalledWebAppManager::SkipStartupForTesting() {
-  g_skip_startup_for_testing_ = true;
+base::AutoReset<bool> PreinstalledWebAppManager::SkipStartupForTesting() {
+  return {&g_skip_startup_for_testing_, true};
 }
 
 // static
@@ -608,26 +608,29 @@ PreinstalledWebAppManager::BypassAwaitingDependenciesForTesting() {
 }
 
 // static
-void PreinstalledWebAppManager::BypassOfflineManifestRequirementForTesting() {
-  g_bypass_offline_manifest_requirement_for_testing_ = true;
+base::AutoReset<bool>
+PreinstalledWebAppManager::BypassOfflineManifestRequirementForTesting() {
+  return {&g_bypass_offline_manifest_requirement_for_testing_, true};
 }
 
 // static
-void PreinstalledWebAppManager::
-    OverridePreviousUserUninstallConfigForTesting() {
-  g_override_previous_user_uninstall_for_testing_ = true;
+base::AutoReset<bool>
+PreinstalledWebAppManager::OverridePreviousUserUninstallConfigForTesting() {
+  return {&g_override_previous_user_uninstall_for_testing_, true};
 }
 
 // static
-void PreinstalledWebAppManager::SetConfigsForTesting(
+base::AutoReset<const base::Value::List*>
+PreinstalledWebAppManager::SetConfigsForTesting(
     const base::Value::List* configs) {
-  g_configs_for_testing = configs;
+  return {&g_configs_for_testing, configs, nullptr};
 }
 
 // static
-void PreinstalledWebAppManager::SetFileUtilsForTesting(
+base::AutoReset<FileUtilsWrapper*>
+PreinstalledWebAppManager::SetFileUtilsForTesting(
     FileUtilsWrapper* file_utils) {
-  g_file_utils_for_testing = file_utils;
+  return {&g_file_utils_for_testing, file_utils, nullptr};
 }
 
 PreinstalledWebAppManager::PreinstalledWebAppManager(Profile* profile)

chrome/browser/web_applications/preinstalled_web_app_manager.h

@@ -77,17 +77,14 @@ class PreinstalledWebAppManager {
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
-  // TODO(crbug.com/1434692): All these should return a base::AutoReset<bool> to
-  // avoid leaking override state beyond unit test execution.
-  static void SkipStartupForTesting();
+  static base::AutoReset<bool> SkipStartupForTesting();
   static base::AutoReset<bool> BypassAwaitingDependenciesForTesting();
-  static void BypassOfflineManifestRequirementForTesting();
-
-  static void OverridePreviousUserUninstallConfigForTesting();
-  static void SetConfigDirForTesting(const base::FilePath* config_dir);
-
-  static void SetConfigsForTesting(const base::Value::List* configs);
-  static void SetFileUtilsForTesting(FileUtilsWrapper* file_utils);
+  static base::AutoReset<bool> BypassOfflineManifestRequirementForTesting();
+  static base::AutoReset<bool> OverridePreviousUserUninstallConfigForTesting();
+  static base::AutoReset<const base::Value::List*> SetConfigsForTesting(
+      const base::Value::List* configs);
+  static base::AutoReset<FileUtilsWrapper*> SetFileUtilsForTesting(
+      FileUtilsWrapper* file_utils);
 
   explicit PreinstalledWebAppManager(Profile* profile);
   PreinstalledWebAppManager(const PreinstalledWebAppManager&) = delete;