Run some MediaApp integration tests with guest and incognito profiles.

The AppService presents itself to the system differently in these
profile configurations, which has resulted in subtle crashes.

This CL configures SystemWebAppManagerBrowserTest with a third gtest
parameter type. It "defaults" to TestProfileType::kRegular and
establishes a way for system apps to neatly specify a set of their
tests that should have coverage for all profile types.

Bespoke tests for the HelpApp are migrated to this setup, and a subset
of pertinent MediaApp tests are parameterised for profiles.

Verified the desired coverage is satisfied by commenting out code
in web_file_tasks.cc that fixed things that have crashed in the past.
E.g. Unconditionally doing `profile = profile->GetOriginalProfile()`
causes 6 of the guest mode tests to crash, and omitting that completely
causes DCHECK(..IsAppServiceAvailableForProfile) to fail for the
incognito tests.

Bug: b/166730281
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: I22e7062eab0d4d21329602ec27afba89197e7c16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2398450Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Rachel Carpenter <carpenterr@chromium.org>
Reviewed-by: Jiewei Qian  <qjw@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#807259}

chrome/browser/apps/app_service/app_service_proxy_factory.cc

@@ -13,6 +13,7 @@
 #include "chrome/browser/web_applications/web_app_provider_factory.h"
 #include "chrome/common/chrome_features.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "content/public/common/content_switches.h"
 #include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/extension_registry_factory.h"
 
@@ -60,6 +61,9 @@ AppServiceProxy* AppServiceProxyFactory::GetForProfile(Profile* profile) {
                 "whether this is appropriate as you may be leaking information "
                 "out of this profile. Returning the AppServiceProxy attached "
                 "to the parent profile instead.";
+    // Fail tests that would trigger DumpWithoutCrashing.
+    DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
+        switches::kTestType));
     base::debug::DumpWithoutCrashing();
   }
 

chrome/browser/chromeos/file_manager/file_manager_test_util.cc

@@ -81,7 +81,7 @@ OpenOperationResult FolderInMyFiles::Open(const base::FilePath& file) {
   // That used to be enough to also launch a Browser for the WebApp. However,
   // since https://crrev.com/c/2121860, ExecuteFileTaskForUrl() goes through the
   // mojoAppService, so it's necessary to flush those calls for WebApps to open.
-  apps::AppServiceProxyFactory::GetForProfile(profile_)
+  apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(profile_)
       ->FlushMojoCallsForTesting();
 
   return open_result;

chrome/browser/chromeos/file_manager/file_tasks_browsertest.cc

@@ -11,10 +11,10 @@
 #include "chrome/browser/chromeos/file_manager/filesystem_api_util.h"
 #include "chrome/browser/chromeos/file_manager/volume_manager.h"
 #include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
-#include "chrome/browser/chromeos/web_applications/test/profile_test_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/test/profile_test_helper.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/constants/chromeos_features.h"

chrome/browser/chromeos/web_applications/help_app_integration_browsertest.cc

@@ -52,6 +52,8 @@ class HelpAppIntegrationTest : public SystemWebAppIntegrationTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+using HelpAppAllProfilesIntegrationTest = HelpAppIntegrationTest;
+
 // Waits for and expects that the correct url is opened.
 void WaitForAppToOpen(const GURL& expected_url) {
   // Start with a number of browsers (may include an incognito browser).
@@ -149,17 +151,15 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2InAppMetrics) {
   EXPECT_EQ(1, user_action_tester.GetActionCount("Discover.Help.TabClicked"));
 }
 
-// Test that the Help App shortcut doesn't crash an incognito browser.
-IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2Incognito) {
+IN_PROC_BROWSER_TEST_P(HelpAppAllProfilesIntegrationTest, HelpAppV2ShowHelp) {
   WaitForTestSystemAppInstall();
 
-  chrome::ShowHelp(CreateIncognitoBrowser(), chrome::HELP_SOURCE_KEYBOARD);
+  chrome::ShowHelp(browser(), chrome::HELP_SOURCE_KEYBOARD);
 
 #if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
   EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/")));
 #else
-  // We just have 2 browsers, the incognito and regular. Navigates chrome.
-  EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
+  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
   EXPECT_EQ(GURL(chrome::kChromeHelpViaKeyboardURL),
             chrome::FindLastActive()
                 ->tab_strip_model()
@@ -170,7 +170,8 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2Incognito) {
 
 // Test that launching the Help App's release notes opens the app on the Release
 // Notes page.
-IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2LaunchReleaseNotes) {
+IN_PROC_BROWSER_TEST_P(HelpAppAllProfilesIntegrationTest,
+                       HelpAppV2LaunchReleaseNotes) {
   WaitForTestSystemAppInstall();
 
   // There should be 1 browser window initially.
@@ -220,22 +221,6 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ReleaseNotesMetrics) {
 #endif
 }
 
-// Test that launching the Help App's release notes doesn't crash an incognito
-// browser.
-IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ReleaseNotesIncognito) {
-  WaitForTestSystemAppInstall();
-
-  chrome::LaunchReleaseNotes(CreateIncognitoBrowser()->profile(),
-                             apps::mojom::LaunchSource::kFromOtherApp);
-
-#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/updates")));
-#else
-  // We just have 2 browsers, the incognito and regular. No new app opens.
-  EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
-#endif
-}
-
 // Test that clicking the release notes notification opens Help App.
 IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest,
                        HelpAppV2LaunchReleaseNotesFromNotification) {
@@ -348,7 +333,7 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppV2ShowParentalControls) {
 }
 
 // Test that the Help App opens when Gesture help requested.
-IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppOpenGestures) {
+IN_PROC_BROWSER_TEST_P(HelpAppAllProfilesIntegrationTest, HelpAppOpenGestures) {
   WaitForTestSystemAppInstall();
   base::HistogramTester histogram_tester;
 
@@ -401,63 +386,6 @@ IN_PROC_BROWSER_TEST_P(HelpAppIntegrationTest, HelpAppOpenKeyboardShortcut) {
 INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_MANIFEST_INSTALL_P(
     HelpAppIntegrationTest);
 
-class HelpAppGuestSessionIntegrationTest : public HelpAppIntegrationTest {
- protected:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    command_line->AppendSwitch(chromeos::switches::kGuestSession);
-    command_line->AppendSwitch(::switches::kIncognito);
-    command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "hash");
-    command_line->AppendSwitchASCII(
-        chromeos::switches::kLoginUser,
-        user_manager::GuestAccountId().GetUserEmail());
-  }
-};
-
-// Test that the Help App shortcut doesn't crash in guest mode.
-IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest, HelpAppShowHelp) {
-  WaitForTestSystemAppInstall();
-
-  chrome::ShowHelp(browser(), chrome::HELP_SOURCE_KEYBOARD);
-
-#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/")));
-#else
-  // No new app should open on non-branded builds. Navigates chrome.
-  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
-  EXPECT_EQ(GURL(chrome::kChromeHelpViaKeyboardURL),
-            chrome::FindLastActive()
-                ->tab_strip_model()
-                ->GetActiveWebContents()
-                ->GetVisibleURL());
-#endif
-}
-
-// Test that the Help App release notes entry point doesn't crash in guest mode.
-IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest,
-                       HelpAppLaunchReleaseNotes) {
-  WaitForTestSystemAppInstall();
-
-  chrome::LaunchReleaseNotes(profile(),
-                             apps::mojom::LaunchSource::kFromOtherApp);
-
-#if defined(OS_CHROMEOS) && BUILDFLAG(GOOGLE_CHROME_BRANDING)
-  EXPECT_NO_FATAL_FAILURE(WaitForAppToOpen(GURL("chrome://help-app/updates")));
-#else
-  // Nothing should happen on non-branded builds.
-  EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
-#endif
-}
-
-// Test that Gesture help works in guest mode.
-IN_PROC_BROWSER_TEST_P(HelpAppGuestSessionIntegrationTest,
-                       HelpAppOpenGestures) {
-  WaitForTestSystemAppInstall();
-
-  SystemTrayClient::Get()->ShowGestureEducationHelp();
-
-  EXPECT_NO_FATAL_FAILURE(
-      WaitForAppToOpen(GURL("chrome://help-app/help/sub/3399710/id/9739838")));
-}
-
-INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_MANIFEST_INSTALL_P(
-    HelpAppGuestSessionIntegrationTest);
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_PROFILE_TYPES_P(
+    HelpAppAllProfilesIntegrationTest,
+    kManifestInstall);

chrome/browser/chromeos/web_applications/media_app_integration_browsertest.cc

@@ -76,6 +76,10 @@ class MediaAppIntegrationWithFilesAppTest : public MediaAppIntegrationTest {
   }
 };
 
+using MediaAppIntegrationAllProfilesTest = MediaAppIntegrationTest;
+using MediaAppIntegrationWithFilesAppAllProfilesTest =
+    MediaAppIntegrationWithFilesAppTest;
+
 // Gets the base::FilePath for a named file in the test folder.
 base::FilePath TestFile(const std::string& ascii_name) {
   base::FilePath path;
@@ -170,7 +174,8 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppLaunchWithFile) {
 
 // Ensures that chrome://media-app is available as a file task for the ChromeOS
 // file manager and eligible for opening appropriate files / mime types.
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppEligibleOpenTask) {
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationAllProfilesTest,
+                       MediaAppEligibleOpenTask) {
   constexpr bool kIsDirectory = false;
   const extensions::EntryInfo image_entry(TestFile(kFilePng800x600),
                                           "image/png", kIsDirectory);
@@ -200,7 +205,8 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppEligibleOpenTask) {
   }
 }
 
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, HiddenInLauncherAndSearch) {
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationAllProfilesTest,
+                       HiddenInLauncherAndSearch) {
   WaitForTestSystemAppInstall();
 
   // Check system_web_app_manager has the correct attributes for Media App.
@@ -332,7 +338,7 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest,
 // End-to-end test to ensure that the MediaApp successfully registers as a file
 // handler with the ChromeOS file manager on startup and acts as the default
 // handler for a given file.
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest,
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppAllProfilesTest,
                        FileOpenUsesMediaApp) {
   WaitForTestSystemAppInstall();
   Browser* test_browser = chrome::FindBrowserWithActiveWindow();
@@ -359,7 +365,7 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest,
 
 // Test that the MediaApp can navigate other files in the directory of a file
 // that was opened, even if those files have changed since launch.
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest,
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppAllProfilesTest,
                        FileOpenCanTraverseDirectory) {
   WaitForTestSystemAppInstall();
 
@@ -419,7 +425,8 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest,
 }
 
 // Integration test for rename using the WritableFileSystem and Streams APIs.
-IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest, RenameFile) {
+IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppAllProfilesTest,
+                       RenameFile) {
   WaitForTestSystemAppInstall();
 
   file_manager::test::FolderInMyFiles folder(profile());
@@ -456,5 +463,14 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationWithFilesAppTest, RenameFile) {
 INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_WEB_APP_INFO_INSTALL_P(
     MediaAppIntegrationTest);
 
-INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_WEB_APP_INFO_INSTALL_P(
-    MediaAppIntegrationWithFilesAppTest);
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_PROFILE_TYPES_P(
+    MediaAppIntegrationAllProfilesTest,
+    kWebAppInfoInstall);
+
+// Note: All MediaAppIntegrationWithFilesAppTest cases above currently want
+// coverage for all profile types, so the "less" prarameterized prefix is not
+// instantiated to avoid a gtest warning.
+
+INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_PROFILE_TYPES_P(
+    MediaAppIntegrationWithFilesAppAllProfilesTest,
+    kWebAppInfoInstall);

chrome/browser/web_applications/system_web_app_manager_browsertest.cc

@@ -63,6 +63,20 @@
 #include "components/policy/core/common/policy_pref_names.h"
 #endif
 
+namespace {
+
+// Helper to call AppServiceProxyFactory::GetForProfile().
+apps::AppServiceProxy* GetAppServiceProxy(Profile* profile) {
+  // Crash if there is no AppService support for |profile|. GetForProfile() will
+  // DumpWithoutCrashing, which will not fail a test. No codepath should trigger
+  // that in normal operation.
+  DCHECK(
+      apps::AppServiceProxyFactory::IsAppServiceAvailableForProfile(profile));
+  return apps::AppServiceProxyFactory::GetForProfile(profile);
+}
+
+}  // namespace
+
 namespace web_app {
 
 SystemWebAppManagerBrowserTestBase::SystemWebAppManagerBrowserTestBase(
@@ -89,11 +103,13 @@ void SystemWebAppManagerBrowserTestBase::WaitForTestSystemAppInstall() {
   } else {
     GetManager().InstallSystemAppsForTesting();
   }
+
   // Ensure apps are registered with the |AppService| and populated in
-  // |AppListModel|.
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
-  proxy->FlushMojoCallsForTesting();
+  // |AppListModel|. Redirect to the profile that has an AppService that can be
+  // flushed. This logic differs from WebAppProviderFactory::GetContextToUse().
+  apps::AppServiceProxyFactory::GetForProfileRedirectInIncognito(
+      browser()->profile())
+      ->FlushMojoCallsForTesting();
 }
 
 apps::AppLaunchParams SystemWebAppManagerBrowserTestBase::LaunchParamsForApp(
@@ -115,10 +131,9 @@ content::WebContents* SystemWebAppManagerBrowserTestBase::LaunchApp(
   content::TestNavigationObserver navigation_observer(GetLaunchURL(params));
   navigation_observer.StartWatchingNewWebContents();
 
-  content::WebContents* web_contents =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile())
-          ->BrowserAppLauncher()
-          ->LaunchAppWithParams(params);
+  content::WebContents* web_contents = GetAppServiceProxy(browser()->profile())
+                                           ->BrowserAppLauncher()
+                                           ->LaunchAppWithParams(params);
 
   if (wait_for_load)
     navigation_observer.Wait();
@@ -181,6 +196,16 @@ SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest(
   }
 }
 
+void SystemWebAppManagerBrowserTest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  SystemWebAppManagerBrowserTestBase::SetUpCommandLine(command_line);
+  if (profile_type() == TestProfileType::kGuest) {
+    ConfigureCommandLineForGuestMode(command_line);
+  } else if (profile_type() == TestProfileType::kIncognito) {
+    command_line->AppendSwitch(::switches::kIncognito);
+  }
+}
+
 // Test that System Apps install correctly with a manifest.
 IN_PROC_BROWSER_TEST_P(SystemWebAppManagerWebAppInfoBrowserTest, Install) {
   WaitForTestSystemAppInstall();
@@ -216,8 +241,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerWebAppInfoBrowserTest, Install) {
 
   // OS Integration only relevant for Chrome OS.
 #if defined(OS_CHROMEOS)
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(apps::mojom::OptionalBool::kTrue, update.ShowInLauncher());
@@ -242,6 +266,16 @@ std::string SystemWebAppManagerTestParamsToString(
   if (std::get<1>(param_info.param) == InstallationType::kWebAppInfoInstall) {
     output.append("_WebAppInfoInstall");
   }
+  switch (std::get<2>(param_info.param)) {
+    case TestProfileType::kRegular:
+      break;
+    case TestProfileType::kIncognito:
+      output.append("_Incognito");
+      break;
+    case TestProfileType::kGuest:
+      output.append("_Guest");
+      break;
+  }
   return output;
 }
 
@@ -308,8 +342,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerWebAppInfoBrowserTest,
       maybe_installation_->GetAppUrl());
   navigation_observer.StartWatchingNewWebContents();
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
 
   proxy->Launch(GetManager().GetAppIdForSystemApp(GetMockAppType()).value(),
                 ui::EventFlags::EF_NONE,
@@ -330,8 +363,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerWebAppInfoBrowserTest,
       maybe_installation_->GetAppUrl());
   navigation_observer.StartWatchingNewWebContents();
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   auto intent = apps::mojom::Intent::New();
   intent->action = apps_util::kIntentActionView;
   intent->mime_type = "text/plain";
@@ -986,8 +1018,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerNotShownInLauncherTest,
 
   // OS Integration only relevant for Chrome OS.
 #if defined(OS_CHROMEOS)
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(apps::mojom::OptionalBool::kFalse, update.ShowInLauncher());
@@ -1022,8 +1053,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerNotShownInSearchTest,
 
   // OS Integration only relevant for Chrome OS.
 #if defined(OS_CHROMEOS)
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(apps::mojom::OptionalBool::kFalse, update.ShowInSearch());
@@ -1047,8 +1077,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerAdditionalSearchTermsTest,
   WaitForTestSystemAppInstall();
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(std::vector<std::string>({"Security"}),
@@ -1170,8 +1199,7 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerMigrationTest,
   WaitForTestSystemAppInstall();
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   const bool app_found = proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(std::vector<std::string>({"Security"}),
@@ -1192,8 +1220,7 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerMigrationTest,
   WaitForTestSystemAppInstall();
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   const bool app_found = proxy->AppRegistryCache().ForOneApp(
       app_id, [](const apps::AppUpdate& update) {
         EXPECT_EQ(std::vector<std::string>({"Security"}),
@@ -1411,8 +1438,7 @@ class SystemWebAppManagerAppSuspensionBrowserTest
       : SystemWebAppManagerBrowserTest(false) {}
 
   apps::mojom::Readiness GetAppReadiness(const AppId& app_id) {
-    apps::AppServiceProxy* proxy =
-        apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+    apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
     apps::mojom::Readiness readiness;
     bool app_found = proxy->AppRegistryCache().ForOneApp(
         app_id, [&readiness](const apps::AppUpdate& update) {
@@ -1423,8 +1449,7 @@ class SystemWebAppManagerAppSuspensionBrowserTest
   }
 
   apps::mojom::IconKeyPtr GetAppIconKey(const AppId& app_id) {
-    apps::AppServiceProxy* proxy =
-        apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+    apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
     apps::mojom::IconKeyPtr icon_key;
     bool app_found = proxy->AppRegistryCache().ForOneApp(
         app_id, [&icon_key](const apps::AppUpdate& update) {
@@ -1463,8 +1488,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerAppSuspensionBrowserTest,
     base::ListValue* list = update.Get();
     list->Clear();
   }
-  apps::AppServiceProxyFactory::GetForProfile(browser()->profile())
-      ->FlushMojoCallsForTesting();
+  GetAppServiceProxy(browser()->profile())->FlushMojoCallsForTesting();
   EXPECT_EQ(apps::mojom::Readiness::kReady, GetAppReadiness(*settings_id));
   EXPECT_FALSE(apps::IconEffects::kBlocked &
                GetAppIconKey(*settings_id)->icon_effects);
@@ -1487,8 +1511,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppManagerAppSuspensionBrowserTest,
     list->Append(policy::SystemFeature::OS_SETTINGS);
   }
 
-  apps::AppServiceProxy* proxy =
-      apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
+  apps::AppServiceProxy* proxy = GetAppServiceProxy(browser()->profile());
   proxy->FlushMojoCallsForTesting();
   EXPECT_EQ(apps::mojom::Readiness::kDisabledByPolicy,
             GetAppReadiness(*settings_id));

chrome/browser/web_applications/system_web_app_manager_browsertest.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/web_applications/test/profile_test_helper.h"
 #include "chrome/browser/web_applications/test/test_system_web_app_installation.h"
 #include "chrome/browser/web_applications/test/test_web_app_provider.h"
 #include "chrome/browser/web_applications/test/web_app_test.h"
@@ -40,8 +41,9 @@ class SystemWebAppManagerBrowserTestBase : public InProcessBrowserTest {
 
   ~SystemWebAppManagerBrowserTestBase() override;
 
-  // Returns the SystemWebAppManager for browser()->profile(). This will be a
-  // TestSystemWebAppManager if initialized with |install_mock| true.
+  // Returns the SystemWebAppManager for browser()->profile(). For incognito
+  // profiles, this will be the SystemWebAppManager of the original profile.
+  // Returns TestSystemWebAppManager if initialized with |install_mock| true.
   SystemWebAppManager& GetManager();
 
   // Returns SystemAppType of mocked app, only valid if |install_mock| is true.
@@ -103,7 +105,7 @@ class SystemWebAppManagerBrowserTestBase : public InProcessBrowserTest {
 enum class InstallationType { kManifestInstall, kWebAppInfoInstall };
 
 using SystemWebAppManagerTestParams =
-    std::tuple<ProviderType, InstallationType>;
+    std::tuple<ProviderType, InstallationType, TestProfileType>;
 
 class SystemWebAppManagerBrowserTest
     : public SystemWebAppManagerBrowserTestBase,
@@ -111,10 +113,15 @@ class SystemWebAppManagerBrowserTest
  public:
   explicit SystemWebAppManagerBrowserTest(bool install_mock = true);
   ~SystemWebAppManagerBrowserTest() override = default;
+
   ProviderType provider_type() const { return std::get<0>(GetParam()); }
   bool install_from_web_app_info() const {
     return std::get<1>(GetParam()) == InstallationType::kWebAppInfoInstall;
   }
+  TestProfileType profile_type() const { return std::get<2>(GetParam()); }
+
+  // InProcessBrowserTest:
+  void SetUpCommandLine(base::CommandLine* command_line) override;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -131,15 +138,16 @@ std::string SystemWebAppManagerTestParamsToString(
   INSTANTIATE_TEST_SUITE_P(All, SUITE, PARAMS,                         \
                            web_app::SystemWebAppManagerTestParamsToString)
 
-#define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P( \
-    SUITE)                                                                 \
-  INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_P(                         \
-      SUITE,                                                               \
-      ::testing::Combine(                                                  \
-          ::testing::Values(web_app::ProviderType::kBookmarkApps,          \
-                            web_app::ProviderType::kWebApps),              \
-          ::testing::Values(web_app::InstallationType::kManifestInstall,   \
-                            web_app::InstallationType::kWebAppInfoInstall)))
+#define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P(  \
+    SUITE)                                                                  \
+  INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_P(                          \
+      SUITE,                                                                \
+      ::testing::Combine(                                                   \
+          ::testing::Values(web_app::ProviderType::kBookmarkApps,           \
+                            web_app::ProviderType::kWebApps),               \
+          ::testing::Values(web_app::InstallationType::kManifestInstall,    \
+                            web_app::InstallationType::kWebAppInfoInstall), \
+          ::testing::Values(TestProfileType::kRegular)))
 
 #define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_MANIFEST_INSTALL_P( \
     SUITE)                                                                \
@@ -148,7 +156,8 @@ std::string SystemWebAppManagerTestParamsToString(
       ::testing::Combine(                                                 \
           ::testing::Values(web_app::ProviderType::kBookmarkApps,         \
                             web_app::ProviderType::kWebApps),             \
-          ::testing::Values(web_app::InstallationType::kManifestInstall)))
+          ::testing::Values(web_app::InstallationType::kManifestInstall), \
+          ::testing::Values(TestProfileType::kRegular)))
 
 #define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_WEB_APP_INFO_INSTALL_P( \
     SUITE)                                                                    \
@@ -157,6 +166,24 @@ std::string SystemWebAppManagerTestParamsToString(
       ::testing::Combine(                                                     \
           ::testing::Values(web_app::ProviderType::kBookmarkApps,             \
                             web_app::ProviderType::kWebApps),                 \
-          ::testing::Values(web_app::InstallationType::kWebAppInfoInstall)))
+          ::testing::Values(web_app::InstallationType::kWebAppInfoInstall),   \
+          ::testing::Values(TestProfileType::kRegular)))
+
+// Instantiates 2x1x3 = 6 versions of each test in |SUITE| to ensure coverage of
+// Guest and Incognito profiles, as well as regular profiles. This is designed
+// for testing specific apps that are present in these profile types, so only
+// one |INSTALL_TYPE| is used: either kManifestInstall or kWebAppInfoInstall.
+// This is currently only used on ChromeOS. Other platforms will likely need a
+// differently defined macro because there is no such thing as Guest mode.
+#define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_PROFILE_TYPES_P(   \
+    SUITE, INSTALL_TYPE)                                                     \
+  INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_P(                           \
+      SUITE, ::testing::Combine(                                             \
+                 ::testing::Values(web_app::ProviderType::kBookmarkApps,     \
+                                   web_app::ProviderType::kWebApps),         \
+                 ::testing::Values(web_app::InstallationType::INSTALL_TYPE), \
+                 ::testing::Values(TestProfileType::kRegular,                \
+                                   TestProfileType::kIncognito,              \
+                                   TestProfileType::kGuest)))
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_

chrome/browser/chromeos/web_applications/test/profile_test_helper.cc → chrome/browser/web_applications/test/profile_test_helper.cc

@@ -2,11 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/chromeos/web_applications/test/profile_test_helper.h"
+#include "chrome/browser/web_applications/test/profile_test_helper.h"
 
+#include "base/notreached.h"
+
+#if defined(OS_CHROMEOS)
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/account_id/account_id.h"
 #include "components/user_manager/user_names.h"
+#endif
 
 std::string TestProfileTypeToString(
     const ::testing::TestParamInfo<TestProfileType>& info) {
@@ -21,10 +25,14 @@ std::string TestProfileTypeToString(
 }
 
 void ConfigureCommandLineForGuestMode(base::CommandLine* command_line) {
+#if defined(OS_CHROMEOS)
   command_line->AppendSwitch(chromeos::switches::kGuestSession);
   command_line->AppendSwitch(::switches::kIncognito);
   command_line->AppendSwitchASCII(chromeos::switches::kLoginProfile, "hash");
   command_line->AppendSwitchASCII(
       chromeos::switches::kLoginUser,
       user_manager::GuestAccountId().GetUserEmail());
+#else
+  NOTREACHED();
+#endif
 }

chrome/browser/chromeos/web_applications/test/profile_test_helper.h → chrome/browser/web_applications/test/profile_test_helper.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_CHROMEOS_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_
-#define CHROME_BROWSER_CHROMEOS_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_
 
 #include <string>
 
@@ -26,7 +26,7 @@ std::string TestProfileTypeToString(
 
 // Adds the necessary flags to |command_line| to start a browser test in guest
 // mode. Should be invoked in SetUpCommandLine(). Any test can call this: it is
-// not coupled to TestProfileTypeMixin.
+// not coupled to TestProfileTypeMixin. Should only be invoked on ChromeOS.
 void ConfigureCommandLineForGuestMode(base::CommandLine* command_line);
 
 // "Mixin" for configuring a test harness to parameterize on different profile
@@ -58,4 +58,4 @@ class TestProfileTypeMixin
   }
 };
 
-#endif  // CHROME_BROWSER_CHROMEOS_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_TEST_PROFILE_TEST_HELPER_H_

chrome/test/BUILD.gn

@@ -2565,8 +2565,6 @@ if (!is_android) {
         "../browser/chromeos/web_applications/settings_app_integration_browsertest.cc",
         "../browser/chromeos/web_applications/system_web_app_integration_test.cc",
         "../browser/chromeos/web_applications/system_web_app_integration_test.h",
-        "../browser/chromeos/web_applications/test/profile_test_helper.cc",
-        "../browser/chromeos/web_applications/test/profile_test_helper.h",
         "../browser/chromeos/wilco_dtc_supportd/wilco_dtc_supportd_web_request_service_browsertest.cc",
         "../browser/download/notification/download_notification_browsertest.cc",
         "../browser/drive/drive_notification_manager_factory_browsertest.cc",
@@ -5894,6 +5892,8 @@ if (!is_android) {
       "../browser/ui/search/instant_test_utils.h",
       "../browser/ui/search/local_ntp_test_utils.cc",
       "../browser/ui/search/local_ntp_test_utils.h",
+      "../browser/web_applications/test/profile_test_helper.cc",
+      "../browser/web_applications/test/profile_test_helper.h",
       "base/in_process_browser_test.cc",
       "base/in_process_browser_test.h",
       "base/in_process_browser_test_mac.mm",