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

This reverts commit 9bdd1b02.

Reason for revert:
../../chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc:164: Failure
Expected equality of these values:
  kNumApps + kNumDefaultApps
    Which is: 8
  service->GetNumSyncItemsForTest()
    Which is: 15
This test failure is fixed by a different revert:
https://chromium-review.googlesource.com/c/chromium/src/+/2568995
This CL does not introduce any new app installations on start up
to cause the app count to change.

Original change's description:
> Revert "Fix default web apps not appearing in app list/management/search on Chrome OS"
>
> This reverts commit 090a7e86.
>
> Reason for revert: Suspected it introduced sync_integration_tests failure in SingleClientAppListSyncTestWithVerifier.AppListSomeApps, see https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/10364/overview
>
> Original change's description:
> > 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}
>
> TBR=alancutter@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,glenrob@chromium.org
>
> Change-Id: I291b2c2bb25ceb9a244382c4cee7ff190c36a493
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1153233
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2566769
> Reviewed-by: Olga Sharonova <olka@chromium.org>
> Commit-Queue: Olga Sharonova <olka@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#832739}

TBR=alancutter@chromium.org,olka@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,glenrob@chromium.org

# Not skipping CQ checks because this is a reland.

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

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_requirement_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_requirement_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_requirement_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;