[System Web Apps] Prevent System Web Apps installing in tests by default.

This CL changes the System PWA test environment to use an empty
TestSystemWebAppManager by default in unit tests (which use TestingProfile),
and to install no System PWAs in browser tests unless
SystemWebAppManager::InstallSystemAppsForTesting() is called.

This essentially makes the production configuration of SystemWebAppManager
opt-in rather than opt-out.

This is necessary for enabling the SystemWebApps feature by default without
impacting the existing default test state (particularly installed extension
counts).

TBR=jamescook@chromium.org

Bug: 836128
Change-Id: I3395c23ca8e7bd89a661316182d9720cadd4c2b6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609006Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660740}

chrome/browser/ui/settings_window_manager_browsertest_chromeos.cc

@@ -63,13 +63,9 @@ class SettingsWindowManagerTest : public InProcessBrowserTest,
     if (!EnableSystemWebApps())
       return;
 
-    // Wait for the Settings System Web App to install.
-    base::RunLoop run_loop;
     web_app::WebAppProvider::Get(browser()->profile())
         ->system_web_app_manager()
-        .on_apps_synchronized()
-        .Post(FROM_HERE, run_loop.QuitClosure());
-    run_loop.Run();
+        .InstallSystemAppsForTesting();
   }
 
   bool EnableSystemWebApps() { return GetParam(); }
@@ -178,6 +174,10 @@ IN_PROC_BROWSER_TEST_P(SettingsWindowManagerTest, SplitSettings) {
   EXPECT_EQ(1u, observer_.new_settings_count());
   EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
 
+  content::WebContents* web_contents =
+      observer_.browser()->tab_strip_model()->GetWebContentsAt(0);
+  EXPECT_EQ(chrome::kChromeUIOSSettingsHost, web_contents->GetURL().host());
+
   // Showing an OS sub-page reuses the OS settings window.
   settings_manager_->ShowOSSettings(browser()->profile(),
                                     chrome::kBluetoothSubPage);

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.cc

@@ -27,11 +27,12 @@
 
 namespace web_app {
 
-base::Optional<std::string> GetAppIdForSystemWebApp(Profile* profile,
-                                                    SystemAppType app_type) {
-  return WebAppProvider::Get(profile)
-      ->system_web_app_manager()
-      .GetAppIdForSystemApp(app_type);
+base::Optional<web_app::AppId> GetAppIdForSystemWebApp(Profile* profile,
+                                                       SystemAppType app_type) {
+  auto* provider = WebAppProvider::Get(profile);
+  return provider
+             ? provider->system_web_app_manager().GetAppIdForSystemApp(app_type)
+             : base::Optional<web_app::AppId>();
 }
 
 Browser* LaunchSystemWebApp(Profile* profile,
@@ -51,7 +52,7 @@ Browser* LaunchSystemWebApp(Profile* profile,
     }
   }
 
-  base::Optional<std::string> app_id =
+  base::Optional<web_app::AppId> app_id =
       GetAppIdForSystemWebApp(profile, app_type);
   // TODO(calamity): Queue a task to launch app after it is installed.
   if (!app_id)
@@ -83,7 +84,7 @@ Browser* LaunchSystemWebApp(Profile* profile,
 Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type) {
   // TODO(calamity): Determine whether, during startup, we need to wait for
   // app install and then provide a valid answer here.
-  base::Optional<std::string> app_id =
+  base::Optional<web_app::AppId> app_id =
       GetAppIdForSystemWebApp(profile, app_type);
   if (!app_id)
     return nullptr;

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/optional.h"
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "url/gurl.h"
 
@@ -17,8 +18,8 @@ class Profile;
 namespace web_app {
 
 // Returns the PWA system App ID for the given system app type.
-base::Optional<std::string> GetAppIdForSystemWebApp(Profile* profile,
-                                                    SystemAppType app_type);
+base::Optional<web_app::AppId> GetAppIdForSystemWebApp(Profile* profile,
+                                                       SystemAppType app_type);
 
 // Launches a System App to the given URL, reusing any existing window for the
 // app. Returns the browser for the System App, or nullptr if launch/focus

chrome/browser/web_applications/BUILD.gn

@@ -48,6 +48,10 @@ source_set("web_applications") {
     "//skia",
   ]
 
+  if (is_chromeos) {
+    deps += [ "//chromeos/constants" ]
+  }
+
   public_deps = [
     "//chrome/browser/web_applications/proto",
   ]
@@ -82,9 +86,11 @@ source_set("web_applications_test_support") {
   deps = [
     ":web_app_test_group",
     ":web_applications",
+    "//base/test:test_support",
+    "//chrome/browser",
     "//chrome/browser/web_applications/components",
-    "//chrome/test:test_support",
     "//components/sync:test_support_model",
+    "//content/test:test_support",
     "//testing/gtest",
   ]
 }

chrome/browser/web_applications/bookmark_apps/BUILD.gn

@@ -40,6 +40,7 @@ source_set("test_support") {
     "//chrome/browser/web_applications",
     "//chrome/browser/web_applications:web_app_test_group",
     "//chrome/browser/web_applications:web_applications_on_extensions",
+    "//chrome/browser/web_applications:web_applications_test_support",
     "//chrome/browser/web_applications/components",
     "//components/keyed_service/content",
   ]

chrome/browser/web_applications/bookmark_apps/test_web_app_provider.cc

@@ -12,11 +12,22 @@
 #include "chrome/browser/web_applications/components/policy/web_app_policy_manager.h"
 #include "chrome/browser/web_applications/components/web_app_utils.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/test/test_system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_provider_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 
 namespace web_app {
 
+// static
+std::unique_ptr<KeyedService> TestWebAppProvider::BuildDefault(
+    content::BrowserContext* context) {
+  Profile* profile = Profile::FromBrowserContext(context);
+  auto provider = std::make_unique<TestWebAppProvider>(profile);
+  provider->Init();
+
+  return provider;
+}
+
 TestWebAppProvider::TestWebAppProvider(Profile* profile)
     : WebAppProvider(profile) {}
 

chrome/browser/web_applications/bookmark_apps/test_web_app_provider.h

@@ -25,6 +25,11 @@ class WebAppPolicyManager;
 
 class TestWebAppProvider : public WebAppProvider {
  public:
+  // Builds a default WebAppProvider that won't be started. To activate this
+  // default instance, call WebAppProvider::StartRegistry().
+  static std::unique_ptr<KeyedService> BuildDefault(
+      content::BrowserContext* context);
+
   explicit TestWebAppProvider(Profile* profile);
   ~TestWebAppProvider() override;
 

chrome/browser/web_applications/components/app_registrar.cc

@@ -12,7 +12,10 @@ namespace web_app {
 
 AppRegistrar::AppRegistrar(Profile* profile) : profile_(profile) {}
 
-AppRegistrar::~AppRegistrar() = default;
+AppRegistrar::~AppRegistrar() {
+  for (AppRegistrarObserver& observer : observers_)
+    observer.OnAppRegistrarDestroyed();
+}
 
 void AppRegistrar::AddObserver(AppRegistrarObserver* observer) {
   observers_.AddObserver(observer);

chrome/browser/web_applications/components/app_registrar.h

@@ -64,7 +64,7 @@ class AppRegistrar {
  private:
   Profile* profile_;
 
-  base::ObserverList<AppRegistrarObserver> observers_;
+  base::ObserverList<AppRegistrarObserver, /*check_empty=*/true> observers_;
 };
 
 }  // namespace web_app

chrome/browser/web_applications/components/app_registrar_observer.h

@@ -15,6 +15,7 @@ class AppRegistrarObserver : public base::CheckedObserver {
   virtual void OnWebAppInstalled(const AppId& app_id) {}
   virtual void OnWebAppUninstalled(const AppId& app_id) {}
   virtual void OnAppRegistrarShutdown() {}
+  virtual void OnAppRegistrarDestroyed() {}
 };
 
 }  // namespace web_app

chrome/browser/web_applications/components/install_manager.h

@@ -117,6 +117,7 @@ class InstallManager {
                                         WebAppInstallabilityCheckCallback);
 
   // Called before the web app system gets destroyed.
+  // TODO(calamity): Rename to Shutdown().
   void Reset();
 
   void AddObserver(InstallManagerObserver* observer);

chrome/browser/web_applications/components/pending_app_manager.h

@@ -48,6 +48,8 @@ class PendingAppManager {
   PendingAppManager();
   virtual ~PendingAppManager();
 
+  virtual void Shutdown() = 0;
+
   // Queues an installation operation with the highest priority. Essentially
   // installing the app immediately if there are no ongoing operations or
   // installing the app right after the current operation finishes. Runs its

chrome/browser/web_applications/components/test_pending_app_manager.h

@@ -61,6 +61,7 @@ class TestPendingAppManager : public PendingAppManager {
   bool HasAppIdWithInstallSource(
       const AppId& app_id,
       web_app::InstallSource install_source) const override;
+  void Shutdown() override {}
 
  private:
   void DoInstall(InstallOptions install_options, OnceInstallCallback callback);

chrome/browser/web_applications/components/web_app_tab_helper_base.cc

@@ -93,6 +93,10 @@ void WebAppTabHelperBase::OnAppRegistrarShutdown() {
   ResetAppId();
 }
 
+void WebAppTabHelperBase::OnAppRegistrarDestroyed() {
+  observer_.RemoveAll();
+}
+
 void WebAppTabHelperBase::ResetAppId() {
   app_id_.clear();
 

chrome/browser/web_applications/components/web_app_tab_helper_base.h

@@ -84,6 +84,7 @@ class WebAppTabHelperBase
   void OnWebAppInstalled(const AppId& installed_app_id) override;
   void OnWebAppUninstalled(const AppId& uninstalled_app_id) override;
   void OnAppRegistrarShutdown() override;
+  void OnAppRegistrarDestroyed() override;
 
   void ResetAppId();
 

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -58,8 +58,16 @@ PendingBookmarkAppManager::PendingBookmarkAppManager(
 
 PendingBookmarkAppManager::~PendingBookmarkAppManager() = default;
 
+void PendingBookmarkAppManager::Shutdown() {
+  shutting_down_ = true;
+  web_contents_.reset();
+}
+
 void PendingBookmarkAppManager::Install(web_app::InstallOptions install_options,
                                         OnceInstallCallback callback) {
+  if (shutting_down_)
+    return;
+
   pending_tasks_and_callbacks_.push_front(std::make_unique<TaskAndCallback>(
       task_factory_.Run(profile_, registrar_, install_finalizer_,
                         std::move(install_options)),

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -58,6 +58,7 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager {
   ~PendingBookmarkAppManager() override;
 
   // web_app::PendingAppManager
+  void Shutdown() override;
   void Install(web_app::InstallOptions install_options,
                OnceInstallCallback callback) override;
   void InstallApps(std::vector<web_app::InstallOptions> install_options_list,
@@ -112,6 +113,8 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager {
 
   std::deque<std::unique_ptr<TaskAndCallback>> pending_tasks_and_callbacks_;
 
+  bool shutting_down_ = false;
+
   base::WeakPtrFactory<PendingBookmarkAppManager> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(PendingBookmarkAppManager);

chrome/browser/web_applications/system_web_app_manager.cc

@@ -20,6 +20,7 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/version_info/version_info.h"
+#include "content/public/common/content_switches.h"
 
 namespace web_app {
 
@@ -27,7 +28,6 @@ namespace {
 
 base::flat_map<SystemAppType, GURL> CreateSystemWebApps() {
   base::flat_map<SystemAppType, GURL> urls;
-
 // TODO(calamity): Split this into per-platform functions.
 #if defined(OS_CHROMEOS)
   urls[SystemAppType::DISCOVER] = GURL(chrome::kChromeUIDiscoverURL);
@@ -55,9 +55,18 @@ InstallOptions CreateInstallOptionsForSystemApp(const GURL& url) {
 
 SystemWebAppManager::SystemWebAppManager(Profile* profile,
                                          PendingAppManager* pending_app_manager)
-    : pref_service_(profile->GetPrefs()),
+    : on_apps_synchronized_(new base::OneShotEvent()),
+      pref_service_(profile->GetPrefs()),
       pending_app_manager_(pending_app_manager),
       weak_ptr_factory_(this) {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          ::switches::kTestType)) {
+    // Always update in tests, and return early to avoid populating with real
+    // system apps.
+    update_policy_ = UpdatePolicy::kAlwaysUpdate;
+    return;
+  }
+
 #if defined(OFFICIAL_BUILD)
   // Official builds should trigger updates whenever the version number changes.
   update_policy_ = UpdatePolicy::kOnVersionChange;
@@ -94,11 +103,25 @@ void SystemWebAppManager::Start() {
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-base::Optional<std::string> SystemWebAppManager::GetAppIdForSystemApp(
+void SystemWebAppManager::InstallSystemAppsForTesting() {
+  on_apps_synchronized_.reset(new base::OneShotEvent());
+  system_app_urls_ = CreateSystemWebApps();
+  Start();
+
+  // Wait for the System Web Apps to install.
+  base::RunLoop run_loop;
+  on_apps_synchronized().Post(FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+base::Optional<AppId> SystemWebAppManager::GetAppIdForSystemApp(
     SystemAppType id) const {
-  auto app = system_app_urls_.find(id);
-  DCHECK(app != system_app_urls_.end());
-  return pending_app_manager_->LookupAppId(app->second);
+  auto app_url_it = system_app_urls_.find(id);
+
+  if (app_url_it == system_app_urls_.end())
+    return base::Optional<AppId>();
+
+  return pending_app_manager_->LookupAppId(app_url_it->second);
 }
 
 bool SystemWebAppManager::IsSystemWebApp(const AppId& app_id) const {
@@ -137,8 +160,8 @@ void SystemWebAppManager::OnAppsSynchronized(
                              CurrentVersion().GetString());
   }
 
-  if (!on_apps_synchronized_.is_signaled())
-    on_apps_synchronized_.Signal();
+  if (!on_apps_synchronized_->is_signaled())
+    on_apps_synchronized_->Signal();
 }
 
 bool SystemWebAppManager::NeedsUpdate() const {

chrome/browser/web_applications/system_web_app_manager.h

@@ -56,16 +56,21 @@ class SystemWebAppManager {
 
   static bool IsEnabled();
 
+  // The SystemWebAppManager is disabled in browser tests by default because it
+  // pollutes the startup state. Call this to enable them for SystemWebApp
+  // specific tests.
+  void InstallSystemAppsForTesting();
+
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
   // Returns the app id for the given System App |id|.
-  base::Optional<std::string> GetAppIdForSystemApp(SystemAppType id) const;
+  base::Optional<AppId> GetAppIdForSystemApp(SystemAppType id) const;
 
   // Returns whether |app_id| points to an installed System App.
   bool IsSystemWebApp(const AppId& app_id) const;
 
   const base::OneShotEvent& on_apps_synchronized() const {
-    return on_apps_synchronized_;
+    return *on_apps_synchronized_;
   }
 
  protected:
@@ -79,7 +84,7 @@ class SystemWebAppManager {
   void OnAppsSynchronized(PendingAppManager::SynchronizeResult result);
   bool NeedsUpdate() const;
 
-  base::OneShotEvent on_apps_synchronized_;
+  std::unique_ptr<base::OneShotEvent> on_apps_synchronized_;
 
   UpdatePolicy update_policy_;
 

chrome/browser/web_applications/test/test_system_web_app_manager.cc

@@ -12,7 +12,9 @@ namespace web_app {
 TestSystemWebAppManager::TestSystemWebAppManager(
     Profile* profile,
     PendingAppManager* pending_app_manager)
-    : SystemWebAppManager(profile, pending_app_manager) {}
+    : SystemWebAppManager(profile, pending_app_manager) {
+  SetSystemApps(base::flat_map<SystemAppType, GURL>());
+}
 
 TestSystemWebAppManager::~TestSystemWebAppManager() = default;
 

chrome/browser/web_applications/web_app_provider.cc

@@ -106,6 +106,23 @@ WebAppUiDelegate& WebAppProvider::ui_delegate() {
   return *ui_delegate_;
 }
 
+void WebAppProvider::Shutdown() {
+  // Destroy subsystems.
+  // The order of destruction is the reverse order of creation:
+  // TODO(calamity): Make subsystem destruction happen in destructor.
+  web_app_policy_manager_.reset();
+  system_web_app_manager_.reset();
+  pending_app_manager_.reset();
+
+  install_manager_.reset();
+  install_finalizer_.reset();
+  icon_manager_.reset();
+  registrar_.reset();
+  database_.reset();
+  database_factory_.reset();
+  audio_focus_id_map_.reset();
+}
+
 void WebAppProvider::CreateWebAppsSubsystems(Profile* profile) {
   database_factory_ = std::make_unique<WebAppDatabaseFactory>(profile);
   database_ = std::make_unique<WebAppDatabase>(database_factory_.get());
@@ -206,11 +223,19 @@ void WebAppProvider::Observe(int type,
                              const content::NotificationDetails& detals) {
   DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
 
+  ProfileDestroyed();
+}
+
+void WebAppProvider::ProfileDestroyed() {
+  // Do Reset() for each subsystem to nullify pointers (detach subsystems).
+  if (install_manager_)
+    install_manager_->Reset();
+
   // KeyedService::Shutdown() gets called when the profile is being destroyed,
   // but after DCHECK'ing that no RenderProcessHosts are being leaked. The
   // "chrome::NOTIFICATION_PROFILE_DESTROYED" notification gets sent before the
   // DCHECK so we use that to clean up RenderProcessHosts instead.
-  Reset();
+  pending_app_manager_->Shutdown();
 }
 
 void WebAppProvider::SetRegistryReadyCallback(base::OnceClosure callback) {
@@ -231,27 +256,6 @@ int WebAppProvider::CountUserInstalledApps() const {
   return extensions::CountUserInstalledBookmarkApps(profile_);
 }
 
-void WebAppProvider::Reset() {
-  // TODO(loyso): Make it independent to the order of destruction via using two
-  // end-to-end passes:
-  // 1) Do Reset() for each subsystem to nullify pointers (detach subsystems).
-  install_manager_->Reset();
-
-  // 2) Destroy subsystems.
-  // The order of destruction is the reverse order of creation:
-  web_app_policy_manager_.reset();
-  system_web_app_manager_.reset();
-  pending_app_manager_.reset();
-
-  install_manager_.reset();
-  install_finalizer_.reset();
-  icon_manager_.reset();
-  registrar_.reset();
-  database_.reset();
-  database_factory_.reset();
-  audio_focus_id_map_.reset();
-}
-
 void WebAppProvider::OnScanForExternalWebApps(
     std::vector<InstallOptions> desired_apps_install_options) {
   pending_app_manager_->SynchronizeInstalledApps(

chrome/browser/web_applications/web_app_provider.h

@@ -72,7 +72,10 @@ class WebAppProvider : public WebAppProviderBase,
   WebAppPolicyManager* policy_manager() override;
   WebAppUiDelegate& ui_delegate() override;
 
-  const SystemWebAppManager& system_web_app_manager() {
+  // KeyedService:
+  void Shutdown() override;
+
+  SystemWebAppManager& system_web_app_manager() {
     return *system_web_app_manager_;
   }
 
@@ -106,10 +109,12 @@ class WebAppProvider : public WebAppProviderBase,
 
   void OnRegistryReady();
 
-  void Reset();
-
   void OnScanForExternalWebApps(std::vector<InstallOptions>);
 
+  // Called just before profile destruction. All WebContents must be destroyed
+  // by the end of this method.
+  void ProfileDestroyed();
+
   // New extension-independent subsystems:
   std::unique_ptr<WebAppAudioFocusIdMap> audio_focus_id_map_;
   std::unique_ptr<WebAppDatabaseFactory> database_factory_;

chrome/test/BUILD.gn

@@ -334,11 +334,16 @@ static_library("test_support") {
 
     public_deps += [
       "//apps:test_support",
+      "//chrome/browser/web_applications:web_applications_test_support",
+      "//chrome/browser/web_applications/bookmark_apps:test_support",
       "//chrome/common/extensions/api",
       "//components/guest_view/browser:test_support",
       "//extensions:test_support",
       "//google_apis/drive:test_support",
     ]
+
+    allow_circular_includes_from =
+        [ "//chrome/browser/web_applications:web_applications_test_support" ]
   }
 
   if (enable_message_center) {

chrome/test/base/testing_profile.cc

@@ -128,6 +128,8 @@
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 #include "chrome/browser/extensions/extension_system_factory.h"
 #include "chrome/browser/extensions/test_extension_system.h"
+#include "chrome/browser/web_applications/bookmark_apps/test_web_app_provider.h"
+#include "chrome/browser/web_applications/web_app_provider_factory.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "extensions/browser/event_router_factory.h"
 #include "extensions/browser/extension_pref_value_map.h"
@@ -496,6 +498,9 @@ void TestingProfile::Init() {
 
   extensions::EventRouterFactory::GetInstance()->SetTestingFactory(
       this, BrowserContextKeyedServiceFactory::TestingFactory());
+
+  web_app::WebAppProviderFactory::GetInstance()->SetTestingFactory(
+      this, base::BindRepeating(&web_app::TestWebAppProvider::BuildDefault));
 #endif
 
   // Prefs for incognito profiles are set in CreateIncognitoPrefService() by