SystemWebAppManagerBrowserTest tests bookmark and web apps

The test suite SystemWebAppManagerBrowserTest is now parameterized,
and runs with both bookmark and web app backend implementations.

Not yet updated:
SystemWebAppManagerAdditionalSearchTermsTest.AdditionalSearchTerms
and SystemWebAppManagerNotShownInLauncherTest.NotShownInLauncher
currently fails with web apps. Tracked in crbug.com/1054195

In system_web_app_manager_browsertest.cc, helper methods
EvalJs and ExecJs are retired as they have no clients.

TBR=pkasting@chromium.org

Bug: 1054195
Change-Id: Ib76ed1e2b14f42bf81138c61f5c263652dbe1ad2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2086355
Commit-Queue: Eric Willigers <ericwilligers@chromium.org>
Auto-Submit: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746683}

chrome/browser/chromeos/web_applications/system_web_app_integration_test.cc

@@ -17,7 +17,7 @@
 #include "ui/aura/window.h"
 
 SystemWebAppIntegrationTest::SystemWebAppIntegrationTest()
-    : SystemWebAppManagerBrowserTest(false /* install_mock */) {}
+    : SystemWebAppManagerBrowserTestBase(false /* install_mock */) {}
 
 SystemWebAppIntegrationTest::~SystemWebAppIntegrationTest() = default;
 

chrome/browser/chromeos/web_applications/system_web_app_integration_test.h

@@ -18,8 +18,9 @@ class Profile;
 
 // Test harness for how ChromeOS System Web Apps integrate with the System Web
 // App platform.
+// TODO(crbug.com/1058274): Inherit from SystemWebAppManagerBrowserTest.
 class SystemWebAppIntegrationTest
-    : public web_app::SystemWebAppManagerBrowserTest {
+    : public web_app::SystemWebAppManagerBrowserTestBase {
  public:
   SystemWebAppIntegrationTest();
   ~SystemWebAppIntegrationTest() override;

chrome/browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc

@@ -158,7 +158,7 @@ using SystemWebAppNonClientFrameViewBrowserTest =
     web_app::SystemWebAppManagerBrowserTest;
 
 // System Web Apps don't get the hosted app buttons.
-IN_PROC_BROWSER_TEST_F(SystemWebAppNonClientFrameViewBrowserTest,
+IN_PROC_BROWSER_TEST_P(SystemWebAppNonClientFrameViewBrowserTest,
                        HideHostedAppButtonContainer) {
   Browser* app_browser =
       WaitForSystemAppInstallAndLaunch(web_app::SystemAppType::SETTINGS);
@@ -267,3 +267,9 @@ IN_PROC_BROWSER_TEST_F(BrowserNonClientFrameViewBrowserTest, SaveCardIcon) {
   EXPECT_TRUE(app_frame_view_->Contains(icon));
   EXPECT_TRUE(icon->GetVisible());
 }
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SystemWebAppNonClientFrameViewBrowserTest,
+                         ::testing::Values(web_app::ProviderType::kBookmarkApps,
+                                           web_app::ProviderType::kWebApps),
+                         web_app::ProviderTypeParamToString);

chrome/browser/web_applications/system_web_app_manager_browsertest.cc

@@ -41,7 +41,7 @@
 
 namespace web_app {
 
-SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest(
+SystemWebAppManagerBrowserTestBase::SystemWebAppManagerBrowserTestBase(
     bool install_mock) {
   scoped_feature_list_.InitWithFeatures(
       {features::kSystemWebApps, blink::features::kNativeFileSystemAPI,
@@ -53,18 +53,19 @@ SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest(
   }
 }
 
-SystemWebAppManagerBrowserTest::~SystemWebAppManagerBrowserTest() = default;
+SystemWebAppManagerBrowserTestBase::~SystemWebAppManagerBrowserTestBase() =
+    default;
 
-SystemWebAppManager& SystemWebAppManagerBrowserTest::GetManager() {
+SystemWebAppManager& SystemWebAppManagerBrowserTestBase::GetManager() {
   return WebAppProvider::Get(browser()->profile())->system_web_app_manager();
 }
 
-SystemAppType SystemWebAppManagerBrowserTest::GetMockAppType() {
+SystemAppType SystemWebAppManagerBrowserTestBase::GetMockAppType() {
   DCHECK(maybe_installation_);
   return maybe_installation_->GetType();
 }
 
-void SystemWebAppManagerBrowserTest::WaitForTestSystemAppInstall() {
+void SystemWebAppManagerBrowserTestBase::WaitForTestSystemAppInstall() {
   // Wait for the System Web Apps to install.
   if (maybe_installation_) {
     maybe_installation_->WaitForAppInstall();
@@ -73,7 +74,7 @@ void SystemWebAppManagerBrowserTest::WaitForTestSystemAppInstall() {
   }
 }
 
-Browser* SystemWebAppManagerBrowserTest::WaitForSystemAppInstallAndLaunch(
+Browser* SystemWebAppManagerBrowserTestBase::WaitForSystemAppInstallAndLaunch(
     SystemAppType system_app_type) {
   WaitForTestSystemAppInstall();
   apps::AppLaunchParams params = LaunchParamsForApp(system_app_type);
@@ -84,7 +85,7 @@ Browser* SystemWebAppManagerBrowserTest::WaitForSystemAppInstallAndLaunch(
   return browser;
 }
 
-apps::AppLaunchParams SystemWebAppManagerBrowserTest::LaunchParamsForApp(
+apps::AppLaunchParams SystemWebAppManagerBrowserTestBase::LaunchParamsForApp(
     SystemAppType system_app_type) {
   base::Optional<AppId> app_id =
       GetManager().GetAppIdForSystemApp(system_app_type);
@@ -95,7 +96,7 @@ apps::AppLaunchParams SystemWebAppManagerBrowserTest::LaunchParamsForApp(
       apps::mojom::AppLaunchSource::kSourceTest);
 }
 
-content::WebContents* SystemWebAppManagerBrowserTest::LaunchApp(
+content::WebContents* SystemWebAppManagerBrowserTestBase::LaunchApp(
     const apps::AppLaunchParams& params) {
   // Use apps::LaunchService::OpenApplication() to get the most coverage. E.g.,
   // this is what is invoked by file_manager::file_tasks::ExecuteWebTask() on
@@ -104,24 +105,20 @@ content::WebContents* SystemWebAppManagerBrowserTest::LaunchApp(
       ->OpenApplication(params);
 }
 
-content::EvalJsResult EvalJs(content::WebContents* web_contents,
-                             const std::string& script) {
-  // Set world_id = 1 to bypass Content Security Policy restriction.
-  return content::EvalJs(web_contents, script,
-                         content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
-                         1 /*world_id*/);
-}
-
-::testing::AssertionResult ExecJs(content::WebContents* web_contents,
-                                  const std::string& script) {
-  // Set world_id = 1 to bypass Content Security Policy restriction.
-  return content::ExecJs(web_contents, script,
-                         content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
-                         1 /*world_id*/);
+SystemWebAppManagerBrowserTest::SystemWebAppManagerBrowserTest(
+    bool install_mock)
+    : SystemWebAppManagerBrowserTestBase(install_mock) {
+  if (GetParam() == ProviderType::kWebApps) {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kDesktopPWAsWithoutExtensions);
+  } else if (GetParam() == ProviderType::kBookmarkApps) {
+    scoped_feature_list_.InitAndDisableFeature(
+        features::kDesktopPWAsWithoutExtensions);
+  }
 }
 
 // Test that System Apps install correctly with a manifest.
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest, Install) {
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerBrowserTest, Install) {
   Browser* app_browser = WaitForSystemAppInstallAndLaunch(GetMockAppType());
 
   AppId app_id = app_browser->app_controller()->GetAppId();
@@ -158,7 +155,7 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest, Install) {
 
 // Check the toolbar is not shown for system web apps for pages on the chrome://
 // scheme but is shown off the chrome:// scheme.
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest,
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerBrowserTest,
                        ToolbarVisibilityForSystemWebApp) {
   Browser* app_browser = WaitForSystemAppInstallAndLaunch(GetMockAppType());
   // In scope, the toolbar should not be visible.
@@ -187,7 +184,7 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest,
 // Check launch files are passed to application.
 // Note: This test uses ExecuteScriptXXX instead of ExecJs and EvalJs because of
 // some quirks surrounding origin trials and content security policies.
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest,
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerBrowserTest,
                        LaunchFilesForSystemWebApp) {
   WaitForTestSystemAppInstall();
   apps::AppLaunchParams params = LaunchParamsForApp(GetMockAppType());
@@ -263,11 +260,11 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest,
 }
 
 class SystemWebAppManagerLaunchFilesBrowserTest
-    : public SystemWebAppManagerBrowserTest,
+    : public SystemWebAppManagerBrowserTestBase,
       public testing::WithParamInterface<std::vector<base::Feature>> {
  public:
   SystemWebAppManagerLaunchFilesBrowserTest()
-      : SystemWebAppManagerBrowserTest(/*install_mock=*/false) {
+      : SystemWebAppManagerBrowserTestBase(/*install_mock=*/false) {
     scoped_feature_list_.InitWithFeatures(GetParam(), {});
     maybe_installation_ =
         TestSystemWebAppInstallation::SetUpAppThatReceivesLaunchDirectory();
@@ -278,7 +275,7 @@ class SystemWebAppManagerLaunchFilesBrowserTest
 };
 
 // Launching behavior for apps that do not want to received launch directory are
-// tested in |SystemWebAppManagerBrowserTest.LaunchFilesForSystemWebApp|.
+// tested in |SystemWebAppManagerBrowserTestBase.LaunchFilesForSystemWebApp|.
 // Note: This test uses ExecuteScriptXXX instead of ExecJs and EvalJs because of
 // some quirks surrounding origin trials and content security policies.
 IN_PROC_BROWSER_TEST_P(SystemWebAppManagerLaunchFilesBrowserTest,
@@ -457,16 +454,23 @@ class SystemWebAppManagerNotShownInLauncherTest
   }
 };
 
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerNotShownInLauncherTest,
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerNotShownInLauncherTest,
                        NotShownInLauncher) {
+  // TODO(crbug.com/1054195): Make the expectation unconditional.
+  const web_app::ProviderType provider = provider_type();
+
   WaitForSystemAppInstallAndLaunch(GetMockAppType());
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
 
   apps::AppServiceProxy* proxy =
       apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
-      app_id, [](const apps::AppUpdate& update) {
-        EXPECT_EQ(apps::mojom::OptionalBool::kFalse, update.ShowInLauncher());
+      app_id, [provider](const apps::AppUpdate& update) {
+        if (provider == ProviderType::kWebApps) {
+          EXPECT_EQ(apps::mojom::OptionalBool::kTrue, update.ShowInLauncher());
+        } else {
+          EXPECT_EQ(apps::mojom::OptionalBool::kFalse, update.ShowInLauncher());
+        }
       });
 }
 
@@ -480,17 +484,26 @@ class SystemWebAppManagerAdditionalSearchTermsTest
   }
 };
 
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerAdditionalSearchTermsTest,
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerAdditionalSearchTermsTest,
                        AdditionalSearchTerms) {
+  // TODO(crbug.com/1054195): Make the expectation unconditional.
+  const web_app::ProviderType provider = provider_type();
+
   WaitForSystemAppInstallAndLaunch(GetMockAppType());
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
 
   apps::AppServiceProxy* proxy =
       apps::AppServiceProxyFactory::GetForProfile(browser()->profile());
   proxy->AppRegistryCache().ForOneApp(
-      app_id, [](const apps::AppUpdate& update) {
-        EXPECT_EQ(std::vector<std::string>({"Security"}),
-                  update.AdditionalSearchTerms());
+      app_id, [provider](const apps::AppUpdate& update) {
+        // TODO(crbug.com/1054195): Unconditionally expect "Security".
+        if (provider == ProviderType::kBookmarkApps) {
+          EXPECT_EQ(std::vector<std::string>({"Security"}),
+                    update.AdditionalSearchTerms());
+        } else {
+          EXPECT_EQ(std::vector<std::string>({}),
+                    update.AdditionalSearchTerms());
+        }
       });
 }
 
@@ -504,7 +517,7 @@ class SystemWebAppManagerChromeUntrustedTest
   }
 };
 
-IN_PROC_BROWSER_TEST_F(SystemWebAppManagerChromeUntrustedTest, Install) {
+IN_PROC_BROWSER_TEST_P(SystemWebAppManagerChromeUntrustedTest, Install) {
   Browser* app_browser = WaitForSystemAppInstallAndLaunch(GetMockAppType());
   AppId app_id = GetManager().GetAppIdForSystemApp(GetMockAppType()).value();
   EXPECT_EQ(app_id, app_browser->app_controller()->GetAppId());
@@ -523,12 +536,45 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerChromeUntrustedTest, Install) {
             app_id);
 }
 
+// We test with and without enabling kDesktopPWAsWithoutExtensions.
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SystemWebAppManagerBrowserTest,
+                         ::testing::Values(ProviderType::kBookmarkApps,
+                                           ProviderType::kWebApps),
+                         ProviderTypeParamToString);
+
 INSTANTIATE_TEST_SUITE_P(
     PermissionContext,
     SystemWebAppManagerLaunchFilesBrowserTest,
     testing::Values(
         /*default_enabled_permission_context*/ std::vector<base::Feature>(),
-        /*origin_scoped_permission_context*/ std::vector<base::Feature>(
-            {features::kNativeFileSystemOriginScopedPermissions})));
+        /*origin_scoped_permission_context*/
+        std::vector<base::Feature>(
+            {features::kNativeFileSystemOriginScopedPermissions}),
+        /*default_enabled_permission_context*/
+        std::vector<base::Feature>({features::kDesktopPWAsWithoutExtensions}),
+        /*origin_scoped_permission_context*/
+        std::vector<base::Feature>(
+            {features::kNativeFileSystemOriginScopedPermissions,
+             features::kDesktopPWAsWithoutExtensions})));
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SystemWebAppManagerNotShownInLauncherTest,
+                         ::testing::Values(ProviderType::kBookmarkApps,
+                                           ProviderType::kWebApps),
+                         ProviderTypeParamToString);
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SystemWebAppManagerAdditionalSearchTermsTest,
+                         ::testing::Values(ProviderType::kBookmarkApps,
+                                           ProviderType::kWebApps),
+                         ProviderTypeParamToString);
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         SystemWebAppManagerChromeUntrustedTest,
+                         ::testing::Values(ProviderType::kBookmarkApps,
+                                           ProviderType::kWebApps),
+                         ProviderTypeParamToString);
 
 }  // namespace web_app

chrome/browser/web_applications/system_web_app_manager_browsertest.h

@@ -11,6 +11,7 @@
 #include "base/test/scoped_feature_list.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"
 #include "chrome/test/base/in_process_browser_test.h"
 
 class Browser;
@@ -28,15 +29,17 @@ namespace web_app {
 
 enum class SystemAppType;
 
-class SystemWebAppManagerBrowserTest : public InProcessBrowserTest {
+// Clients should use SystemWebAppManagerBrowserTest, so test can be run with
+// both the new web apps provider and the legacy bookmark apps provider.
+class SystemWebAppManagerBrowserTestBase : public InProcessBrowserTest {
  public:
   // Performs common initialization for testing SystemWebAppManager features.
   // If true, |install_mock| installs a WebUIController that serves a mock
   // System PWA, and ensures the WebAppProvider associated with the startup
   // profile is a TestWebAppProviderCreator.
-  explicit SystemWebAppManagerBrowserTest(bool install_mock = true);
+  explicit SystemWebAppManagerBrowserTestBase(bool install_mock = true);
 
-  ~SystemWebAppManagerBrowserTest() override;
+  ~SystemWebAppManagerBrowserTestBase() override;
 
   // Returns the SystemWebAppManager for browser()->profile(). This will be a
   // TestSystemWebAppManager if initialized with |install_mock| true.
@@ -66,7 +69,20 @@ class SystemWebAppManagerBrowserTest : public InProcessBrowserTest {
 
   base::test::ScopedFeatureList scoped_feature_list_;
 
-  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerBrowserTest);
+  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerBrowserTestBase);
+};
+
+class SystemWebAppManagerBrowserTest
+    : public SystemWebAppManagerBrowserTestBase,
+      public ::testing::WithParamInterface<web_app::ProviderType> {
+ public:
+  explicit SystemWebAppManagerBrowserTest(bool install_mock = true);
+  ~SystemWebAppManagerBrowserTest() override = default;
+
+  web_app::ProviderType provider_type() const { return GetParam(); }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 }  // namespace web_app