Fix default web apps not appearing in app list/management/search on Chrome OS

This CL updates Chrome OS default web apps to be deployed to Chrome OS's
app surfaces (app list, app search and app management) while maintaining
the "do not deploy to OS" logic for non-Chrome OS.

This CL upstreams the "do not deploy to OS" logic in
GetPreinstalledWebApps() up to
ExternalWebAppManager::PostProcessConfigs() where it can apply to all
preinstalled web app configs uniformally.

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=478376&signed_aid=ic4Nc7r-RtFyMKtH3FYXtA==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=478378&signed_aid=VMFIAzHdQv62wv7H0Cvz7Q==&inline=1

Bug: 1153233
Change-Id: Idcbb0d15fc8d3f333d1d59e27b8ff624fe7b1c64
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2562625
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/master@{#832686}

chrome/browser/web_applications/external_web_app_manager.cc

@@ -65,6 +65,7 @@ const base::FilePath::CharType kWebAppsSubDirectory[] =
 #endif
 
 bool g_skip_startup_for_testing_ = false;
+bool g_bypass_offline_manifest_requirment_for_testing_ = false;
 const base::FilePath* g_config_dir_for_testing = nullptr;
 const std::vector<base::Value>* g_configs_for_testing = nullptr;
 const FileUtilsWrapper* g_file_utils_for_testing = nullptr;
@@ -232,6 +233,10 @@ void ExternalWebAppManager::SkipStartupForTesting() {
   g_skip_startup_for_testing_ = true;
 }
 
+void ExternalWebAppManager::BypassOfflineManifestRequirementForTesting() {
+  g_bypass_offline_manifest_requirment_for_testing_ = true;
+}
+
 void ExternalWebAppManager::SetConfigDirForTesting(
     const base::FilePath* config_dir) {
   g_config_dir_for_testing = config_dir;
@@ -332,6 +337,29 @@ void ExternalWebAppManager::PostProcessConfigs(ConsumeInstallOptions callback,
   for (ExternalInstallOptions& options : GetPreinstalledWebApps())
     parsed_configs.options_list.push_back(std::move(options));
 
+  // Set common install options.
+  for (ExternalInstallOptions& options : parsed_configs.options_list) {
+    ALLOW_UNUSED_LOCAL(options);
+    DCHECK_EQ(options.install_source, ExternalInstallSource::kExternalDefault);
+
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
+    if (!g_bypass_offline_manifest_requirment_for_testing_) {
+      // Non-Chrome OS platforms are not permitted to fetch the web app install
+      // URLs during start up.
+      DCHECK(options.app_info_factory);
+      options.only_use_app_info_factory = true;
+    }
+
+    // Preinstalled web apps should not have OS shortcuts of any kind outside of
+    // Chrome OS.
+    options.add_to_applications_menu = false;
+    options.add_to_search = false;
+    options.add_to_management = false;
+    options.add_to_desktop = false;
+    options.add_to_quick_launch_bar = false;
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+  }
+
   bool is_new_user = IsNewUser();
   std::string user_type = apps::DetermineUserType(profile_);
   size_t disabled_count = 0;

chrome/browser/web_applications/external_web_app_manager.h

@@ -56,6 +56,7 @@ class ExternalWebAppManager {
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
   static void SkipStartupForTesting();
+  static void BypassOfflineManifestRequirementForTesting();
   static void SetConfigDirForTesting(const base::FilePath* config_dir);
   static void SetConfigsForTesting(const std::vector<base::Value>* configs);
   static void SetFileUtilsForTesting(const FileUtilsWrapper* file_utils);

chrome/browser/web_applications/external_web_app_manager_browsertest.cc

@@ -14,11 +14,11 @@
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/components/external_app_install_features.h"
-#include "chrome/browser/web_applications/components/os_integration_manager.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/components/web_app_id_constants.h"
 #include "chrome/browser/web_applications/preinstalled_web_apps/preinstalled_web_apps.h"
 #include "chrome/browser/web_applications/test/test_file_utils.h"
+#include "chrome/browser/web_applications/test/test_os_integration_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "content/public/test/browser_test.h"
@@ -38,12 +38,15 @@ class ExternalWebAppManagerBrowserTest
  public:
   ExternalWebAppManagerBrowserTest() {
     ExternalWebAppManager::SkipStartupForTesting();
+    WebAppProvider::SetOsIntegrationManagerFactoryForTesting(
+        [](Profile* profile) -> std::unique_ptr<OsIntegrationManager> {
+          return std::make_unique<TestOsIntegrationManager>(profile, nullptr,
+                                                            nullptr, nullptr);
+        });
   }
 
   void SetUpOnMainThread() override {
     ExtensionBrowserTest::SetUpOnMainThread();
-    os_hooks_suppress_ =
-        OsIntegrationManager::ScopedSuppressOsHooksForTesting();
   }
 
   GURL GetAppUrl() const {
@@ -125,6 +128,7 @@ class ExternalWebAppManagerBrowserTest
 
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
                        LaunchQueryParamsBasic) {
+  ExternalWebAppManager::BypassOfflineManifestRequirementForTesting();
   ASSERT_TRUE(embedded_test_server()->Start());
 
   GURL start_url = embedded_test_server()->GetURL("/web_apps/basic.html");
@@ -156,6 +160,7 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
                        LaunchQueryParamsDuplicate) {
+  ExternalWebAppManager::BypassOfflineManifestRequirementForTesting();
   ASSERT_TRUE(embedded_test_server()->Start());
 
   GURL install_url = embedded_test_server()->GetURL(
@@ -190,6 +195,7 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
                        LaunchQueryParamsComplex) {
+  ExternalWebAppManager::BypassOfflineManifestRequirementForTesting();
   ASSERT_TRUE(embedded_test_server()->Start());
 
   GURL install_url = embedded_test_server()->GetURL(
@@ -225,6 +231,7 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest, UninstallAndReplace) {
+  ExternalWebAppManager::BypassOfflineManifestRequirementForTesting();
   ASSERT_TRUE(embedded_test_server()->Start());
   Profile* profile = browser()->profile();
 
@@ -566,6 +573,20 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest, PreinstalledWebApps) {
     EXPECT_EQ(provider.registrar().GetAppLaunchUrl(expectation.app_id),
               GURL(expectation.launch_url));
   }
+
+  // Note that default web apps *DO* show app icons on Chrome OS however it
+  // is done via the |WebAppsChromeOs| publishing live our current app state to
+  // the app service rather than writing shortcut files as the case on all other
+  // desktop platforms.
+  auto* test_os_integration_manager =
+      provider.os_integration_manager().AsTestOsIntegrationManager();
+  EXPECT_EQ(test_os_integration_manager->num_create_shortcuts_calls(), 0u);
+  EXPECT_EQ(test_os_integration_manager->num_create_file_handlers_calls(), 0u);
+  EXPECT_EQ(test_os_integration_manager->num_register_run_on_os_login_calls(),
+            0u);
+  EXPECT_EQ(
+      test_os_integration_manager->num_add_app_to_quick_launch_bar_calls(), 0u);
+  EXPECT_FALSE(test_os_integration_manager->did_add_to_desktop());
 }
 #endif  // BUILDFLAG(GOOGLE_CHROME_BRANDING)
 

chrome/browser/web_applications/external_web_app_migration_browsertest.cc

@@ -63,6 +63,7 @@ class ExternalWebAppMigrationBrowserTest : public InProcessBrowserTest {
  public:
   ExternalWebAppMigrationBrowserTest() {
     ExternalWebAppManager::SkipStartupForTesting();
+    ExternalWebAppManager::BypassOfflineManifestRequirementForTesting();
     disable_scope_ =
         extensions::ExtensionService::DisableExternalUpdatesForTesting();
   }

chrome/browser/web_applications/preinstalled_web_apps/preinstalled_web_apps.cc

@@ -31,7 +31,9 @@ namespace {
 std::vector<ExternalInstallOptions>* g_preinstalled_app_data_for_testing =
     nullptr;
 
-std::vector<ExternalInstallOptions> GetPreinstalledAppData() {
+}  // namespace
+
+std::vector<ExternalInstallOptions> GetPreinstalledWebApps() {
   if (g_preinstalled_app_data_for_testing)
     return *g_preinstalled_app_data_for_testing;
 
@@ -63,8 +65,6 @@ std::vector<ExternalInstallOptions> GetPreinstalledAppData() {
   return {};
 }
 
-}  // namespace
-
 ScopedTestingPreinstalledAppData::ScopedTestingPreinstalledAppData() {
   DCHECK_EQ(nullptr, g_preinstalled_app_data_for_testing);
   g_preinstalled_app_data_for_testing = &apps;
@@ -75,30 +75,4 @@ ScopedTestingPreinstalledAppData::~ScopedTestingPreinstalledAppData() {
   g_preinstalled_app_data_for_testing = nullptr;
 }
 
-std::vector<ExternalInstallOptions> GetPreinstalledWebApps() {
-  std::vector<ExternalInstallOptions> result;
-
-  for (ExternalInstallOptions& app_data : GetPreinstalledAppData()) {
-    DCHECK_EQ(app_data.install_source, ExternalInstallSource::kExternalDefault);
-
-#if !BUILDFLAG(IS_CHROMEOS_ASH)
-    // Non-Chrome OS platforms are not permitted to fetch the web app install
-    // URLs during start up.
-    DCHECK(app_data.only_use_app_info_factory);
-    DCHECK(app_data.app_info_factory);
-#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
-
-    // Preinstalled web apps should not have OS shortcuts of any kind.
-    app_data.add_to_applications_menu = false;
-    app_data.add_to_desktop = false;
-    app_data.add_to_quick_launch_bar = false;
-    app_data.add_to_search = false;
-    app_data.add_to_management = false;
-    app_data.require_manifest = true;
-    result.push_back(std::move(app_data));
-  }
-
-  return result;
-}
-
 }  // namespace web_app

chrome/browser/web_applications/preinstalled_web_apps/preinstalled_web_apps.h

@@ -11,6 +11,9 @@
 
 namespace web_app {
 
+// Returns the list of web apps that should be pre-installed on new profiles.
+std::vector<ExternalInstallOptions> GetPreinstalledWebApps();
+
 // A scoped helper to provide a testing set of preinstalled app data. This will
 // replace the default set.
 struct ScopedTestingPreinstalledAppData {
@@ -24,9 +27,6 @@ struct ScopedTestingPreinstalledAppData {
   std::vector<ExternalInstallOptions> apps;
 };
 
-// Returns the list of web apps that should be pre-installed on new profiles.
-std::vector<ExternalInstallOptions> GetPreinstalledWebApps();
-
 }  // namespace web_app
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_PREINSTALLED_WEB_APPS_PREINSTALLED_WEB_APPS_H_

chrome/browser/web_applications/test/test_os_integration_manager.cc

@@ -51,8 +51,10 @@ void TestOsIntegrationManager::InstallOsHooks(
   OsHooksResults os_hooks_results{false};
   last_options_ = options;
 
-  if (options.os_hooks[OsHookType::kFileHandlers])
+  if (options.os_hooks[OsHookType::kFileHandlers]) {
+    ++num_create_file_handlers_calls_;
     os_hooks_results[OsHookType::kFileHandlers] = true;
+  }
 
   did_add_to_desktop_ = options.add_to_desktop;
 

chrome/browser/web_applications/test/test_os_integration_manager.h

@@ -44,6 +44,10 @@ class TestOsIntegrationManager : public OsIntegrationManager {
     return num_create_shortcuts_calls_;
   }
 
+  size_t num_create_file_handlers_calls() const {
+    return num_create_file_handlers_calls_;
+  }
+
   size_t num_register_run_on_os_login_calls() const {
     return num_register_run_on_os_login_calls_;
   }
@@ -76,6 +80,7 @@ class TestOsIntegrationManager : public OsIntegrationManager {
 
  private:
   size_t num_create_shortcuts_calls_ = 0;
+  size_t num_create_file_handlers_calls_ = 0;
   size_t num_register_run_on_os_login_calls_ = 0;
   size_t num_add_app_to_quick_launch_bar_calls_ = 0;
   base::Optional<bool> did_add_to_desktop_;

chrome/browser/web_applications/web_app_provider.cc

@@ -41,6 +41,13 @@
 
 namespace web_app {
 
+namespace {
+
+WebAppProvider::OsIntegrationManagerFactory
+    g_os_integration_manager_factory_for_testing = nullptr;
+
+}  // namespace
+
 // static
 WebAppProvider* WebAppProvider::Get(Profile* profile) {
   return WebAppProviderFactory::GetForProfile(profile);
@@ -55,6 +62,12 @@ WebAppProvider* WebAppProvider::GetForWebContents(
   return WebAppProvider::Get(profile);
 }
 
+// static
+void WebAppProvider::SetOsIntegrationManagerFactoryForTesting(
+    OsIntegrationManagerFactory factory) {
+  g_os_integration_manager_factory_for_testing = factory;
+}
+
 WebAppProvider::WebAppProvider(Profile* profile) : profile_(profile) {
   DCHECK(AreWebAppsEnabled(profile_));
   // WebApp System must have only one instance in original profile.
@@ -207,15 +220,20 @@ void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
   install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
       profile, icon_manager.get(), std::move(legacy_finalizer));
 
-  auto file_handler_manager =
-      std::make_unique<WebAppFileHandlerManager>(profile);
-  auto protocol_handler_manager =
-      std::make_unique<ProtocolHandlerManager>(profile);
-  auto shortcut_manager = std::make_unique<WebAppShortcutManager>(
-      profile, icon_manager.get(), file_handler_manager.get());
-  os_integration_manager_ = std::make_unique<OsIntegrationManager>(
-      profile, std::move(shortcut_manager), std::move(file_handler_manager),
-      std::move(protocol_handler_manager));
+  if (g_os_integration_manager_factory_for_testing) {
+    os_integration_manager_ =
+        g_os_integration_manager_factory_for_testing(profile);
+  } else {
+    auto file_handler_manager =
+        std::make_unique<WebAppFileHandlerManager>(profile);
+    auto protocol_handler_manager =
+        std::make_unique<ProtocolHandlerManager>(profile);
+    auto shortcut_manager = std::make_unique<WebAppShortcutManager>(
+        profile, icon_manager.get(), file_handler_manager.get());
+    os_integration_manager_ = std::make_unique<OsIntegrationManager>(
+        profile, std::move(shortcut_manager), std::move(file_handler_manager),
+        std::move(protocol_handler_manager));
+  }
 
   migration_manager_ = std::make_unique<WebAppMigrationManager>(
       profile, database_factory_.get(), icon_manager.get(),

chrome/browser/web_applications/web_app_provider.h

@@ -60,6 +60,11 @@ class WebAppProvider : public WebAppProviderBase {
   static WebAppProvider* Get(Profile* profile);
   static WebAppProvider* GetForWebContents(content::WebContents* web_contents);
 
+  using OsIntegrationManagerFactory =
+      std::unique_ptr<OsIntegrationManager> (*)(Profile*);
+  static void SetOsIntegrationManagerFactoryForTesting(
+      OsIntegrationManagerFactory factory);
+
   explicit WebAppProvider(Profile* profile);
   WebAppProvider(const WebAppProvider&) = delete;
   WebAppProvider& operator=(const WebAppProvider&) = delete;