[System PWAs] Add a System Web App Manager behind kSystemWebApps feature.

This CL adds a System Web App Manager which installs Bookmark Apps which
point at a chrome:// URL.

This CL adds chrome://settings and chrome://oobe/discover as demo System
Apps when the flag is enabled, and uninstalls them if the flag is
disabled.

Change-Id: I1a97ab095b19b5c0fd50a345848a7b5de0d20f16
Reviewed-on: https://chromium-review.googlesource.com/1227865
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593475}

chrome/browser/extensions/bookmark_app_helper.cc

@@ -434,8 +434,10 @@ void BookmarkAppHelper::OnDidPerformInstallableCheck(
       base::BindOnce(&BookmarkAppHelper::OnIconsDownloaded,
                      weak_factory_.GetWeakPtr())));
 
-  // If the manifest specified icons, don't use the page icons.
-  if (!data.manifest->icons.empty())
+  // If the manifest specified icons, or this is a System App, don't use the
+  // page icons.
+  if (!data.manifest->icons.empty() ||
+      contents_->GetVisibleURL().SchemeIs(content::kChromeUIScheme))
     web_app_icon_downloader_->SkipPageFavicons();
 
   web_app_icon_downloader_->Start();
@@ -526,6 +528,16 @@ void BookmarkAppHelper::OnBubbleCompleted(
       crx_installer_->set_creation_flags(Extension::WAS_INSTALLED_BY_DEFAULT);
     }
 
+    if (is_system_app_) {
+      // System Apps are considered EXTERNAL_COMPONENT as they are downloaded
+      // from the WebUI they point to. COMPONENT seems like the more correct
+      // value, but usages (icon loading, filesystem cleanup), are tightly
+      // coupled to this value, making it unsuitable.
+      crx_installer_->set_install_source(Manifest::EXTERNAL_COMPONENT);
+      // InstallWebApp will OR the creation flags with FROM_BOOKMARK.
+      crx_installer_->set_creation_flags(Extension::WAS_INSTALLED_BY_DEFAULT);
+    }
+
     crx_installer_->InstallWebApp(web_app_info_);
 
     if (InstallableMetrics::IsReportableInstallSource(install_source_) &&

chrome/browser/extensions/bookmark_app_helper.h

@@ -92,6 +92,11 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   bool is_default_app() { return is_default_app_; }
 
+  // If called, the installed extension will be considered system installed.
+  void set_is_system_app() { is_system_app_ = true; }
+
+  bool is_system_app() { return is_system_app_; }
+
   // If called, desktop shortcuts will not be created.
   void set_skip_shortcut_creation() { create_shortcuts_ = false; }
 
@@ -168,6 +173,8 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   bool is_default_app_ = false;
 
+  bool is_system_app_ = false;
+
   bool create_shortcuts_ = true;
 
   // The mechanism via which the app creation was triggered.
@@ -185,7 +192,7 @@ void CreateOrUpdateBookmarkApp(ExtensionService* service,
                                WebApplicationInfo* web_app_info,
                                bool is_locally_installed);
 
-// Returns whether the given |url| is a valid bookmark app url.
+// Returns whether the given |url| is a valid user bookmark app url.
 bool IsValidBookmarkAppUrl(const GURL& url);
 
 }  // namespace extensions

chrome/browser/web_applications/bookmark_apps/BUILD.gn

@@ -14,6 +14,8 @@ source_set("bookmark_apps") {
     "policy/web_app_policy_constants.h",
     "policy/web_app_policy_manager.cc",
     "policy/web_app_policy_manager.h",
+    "system_web_app_manager.cc",
+    "system_web_app_manager.h",
   ]
 
   deps = [
@@ -34,6 +36,7 @@ source_set("unit_tests") {
   sources = [
     "external_web_apps_unittest.cc",
     "policy/web_app_policy_manager_unittest.cc",
+    "system_web_app_manager_unittest.cc",
   ]
 
   deps = [

chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h

@@ -30,8 +30,7 @@ class WebAppPolicyManager {
   // Constructs a WebAppPolicyManager instance that uses
   // |pending_app_manager| to manage apps. |pending_app_manager| should outlive
   // this class.
-  explicit WebAppPolicyManager(Profile* profile,
-                               PendingAppManager* pending_app_manager);
+  WebAppPolicyManager(Profile* profile, PendingAppManager* pending_app_manager);
   ~WebAppPolicyManager();
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

chrome/browser/web_applications/bookmark_apps/system_web_app_manager.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h"
+
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "base/task/post_task.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
+#include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/webui_url_constants.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#endif  // OS_CHROMEOS
+
+namespace web_app {
+
+SystemWebAppManager::SystemWebAppManager(Profile* profile,
+                                         PendingAppManager* pending_app_manager)
+    : profile_(profile), pending_app_manager_(pending_app_manager) {
+  content::BrowserThread::PostAfterStartupTask(
+      FROM_HERE,
+      base::CreateSingleThreadTaskRunnerWithTraits(
+          {content::BrowserThread::UI}),
+      base::BindOnce(&SystemWebAppManager::StartAppInstallation,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+SystemWebAppManager::~SystemWebAppManager() = default;
+
+// static
+bool SystemWebAppManager::ShouldEnableForProfile(Profile* profile) {
+  bool is_enabled = base::FeatureList::IsEnabled(features::kSystemWebApps);
+#if defined(OS_CHROMEOS)
+  // System Apps should not be installed to the signin profile.
+  is_enabled = is_enabled && !chromeos::ProfileHelper::IsSigninProfile(profile);
+#endif
+  return is_enabled;
+}
+
+void SystemWebAppManager::StartAppInstallation() {
+  std::vector<PendingAppManager::AppInfo> apps_to_install;
+
+  if (ShouldEnableForProfile(profile_)) {
+    // Skipping this will uninstall all System Apps currently installed.
+    apps_to_install = CreateSystemWebApps();
+  }
+
+  pending_app_manager_->SynchronizeInstalledApps(
+      std::move(apps_to_install),
+      PendingAppManager::InstallSource::kSystemInstalled);
+}
+
+std::vector<PendingAppManager::AppInfo>
+SystemWebAppManager::CreateSystemWebApps() {
+  std::vector<web_app::PendingAppManager::AppInfo> app_infos;
+
+// TODO(calamity): Split this into per-platform functions.
+#if defined(OS_CHROMEOS)
+  app_infos.emplace_back(
+      GURL(chrome::kChromeUIDiscoverURL),
+      web_app::PendingAppManager::LaunchContainer::kWindow,
+      web_app::PendingAppManager::InstallSource::kSystemInstalled,
+      false /* create_shortcuts */);
+  app_infos.emplace_back(
+      GURL(chrome::kChromeUISettingsURL),
+      web_app::PendingAppManager::LaunchContainer::kWindow,
+      web_app::PendingAppManager::InstallSource::kSystemInstalled,
+      false /* create_shortcuts */);
+#endif  // OS_CHROMEOS
+
+  return app_infos;
+}
+
+}  // namespace web_app

chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/web_applications/components/pending_app_manager.h"
+#include "url/gurl.h"
+
+class Profile;
+
+namespace web_app {
+
+// Installs, uninstalls, and updates System Web Apps.
+class SystemWebAppManager {
+ public:
+  // Constructs a SystemWebAppManager instance that uses
+  // |pending_app_manager| to manage apps. |pending_app_manager| should outlive
+  // this class.
+  SystemWebAppManager(Profile* profile, PendingAppManager* pending_app_manager);
+  virtual ~SystemWebAppManager();
+
+  static bool ShouldEnableForProfile(Profile* profile);
+
+ protected:
+  // Overridden in tests.
+  virtual std::vector<web_app::PendingAppManager::AppInfo>
+  CreateSystemWebApps();
+
+ private:
+  void StartAppInstallation();
+
+  Profile* profile_;
+
+  // Used to install, uninstall, and update apps. Should outlive this class.
+  PendingAppManager* pending_app_manager_;
+
+  base::WeakPtrFactory<SystemWebAppManager> weak_ptr_factory_{this};
+
+  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManager);
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_SYSTEM_WEB_APP_MANAGER_H_

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h"
+
+#include <memory>
+#include <utility>
+#include <vector>
+
+#include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/web_applications/components/pending_app_manager.h"
+#include "chrome/browser/web_applications/components/test_pending_app_manager.h"
+#include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/crx_file/id_util.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_builder.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace web_app {
+
+namespace {
+
+const char kWindowedUrl[] = "https://windowed.example";
+const char kTabbedUrl[] = "https://tabbed.example";
+const char kDefaultContainerUrl[] = "https://default-container.example";
+
+PendingAppManager::AppInfo GetWindowedAppInfo() {
+  return PendingAppManager::AppInfo(
+      GURL(kWindowedUrl), PendingAppManager::LaunchContainer::kWindow,
+      PendingAppManager::InstallSource::kSystemInstalled,
+      false /* create_shortcuts */);
+}
+
+PendingAppManager::AppInfo GetTabbedAppInfo() {
+  return PendingAppManager::AppInfo(
+      GURL(kTabbedUrl), PendingAppManager::LaunchContainer::kTab,
+      PendingAppManager::InstallSource::kSystemInstalled,
+      false /* create_shortcuts */);
+}
+
+}  // namespace
+
+class TestSystemWebAppManager : public SystemWebAppManager {
+ public:
+  TestSystemWebAppManager(Profile* profile,
+                          PendingAppManager* pending_app_manager,
+                          std::vector<PendingAppManager::AppInfo> system_apps)
+      : SystemWebAppManager(profile, pending_app_manager),
+        system_apps_(std::move(system_apps)) {}
+  ~TestSystemWebAppManager() override {}
+
+  std::vector<web_app::PendingAppManager::AppInfo> CreateSystemWebApps()
+      override {
+    return std::move(system_apps_);
+  }
+
+ private:
+  std::vector<PendingAppManager::AppInfo> system_apps_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSystemWebAppManager);
+};
+
+class SystemWebAppManagerTest : public ChromeRenderViewHostTestHarness {
+ public:
+  SystemWebAppManagerTest() = default;
+  ~SystemWebAppManagerTest() override = default;
+
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    scoped_feature_list_.InitWithFeatures({features::kSystemWebApps}, {});
+
+    // Reset WebAppProvider so that its SystemWebAppManager doesn't interfere
+    // with tests.
+    web_app::WebAppProvider::Get(profile())->Reset();
+  }
+
+  void SimulatePreviouslyInstalledApp(
+      TestPendingAppManager* pending_app_manager,
+      GURL url,
+      PendingAppManager::InstallSource install_source) {
+    std::string id =
+        crx_file::id_util::GenerateId("fake_app_id_for:" + url.spec());
+    extensions::ExtensionRegistry::Get(profile())->AddEnabled(
+        extensions::ExtensionBuilder("Dummy Name").SetID(id).Build());
+
+    ExtensionIdsMap extension_ids_map(profile()->GetPrefs());
+    extension_ids_map.Insert(url, id, install_source);
+
+    pending_app_manager->SimulatePreviouslyInstalledApp(url, install_source);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerTest);
+};
+
+// Test that System Apps are uninstalled with the feature disabled.
+TEST_F(SystemWebAppManagerTest, Disabled) {
+  base::test::ScopedFeatureList disable_feature_list;
+  disable_feature_list.InitWithFeatures({}, {features::kSystemWebApps});
+
+  auto pending_app_manager = std::make_unique<TestPendingAppManager>();
+
+  SimulatePreviouslyInstalledApp(
+      pending_app_manager.get(), GURL(kWindowedUrl),
+      PendingAppManager::InstallSource::kSystemInstalled);
+
+  std::vector<PendingAppManager::AppInfo> system_apps;
+  system_apps.push_back(GetWindowedAppInfo());
+
+  TestSystemWebAppManager system_web_app_manager(
+      profile(), pending_app_manager.get(), std::move(system_apps));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(pending_app_manager->install_requests().empty());
+
+  // We should try to uninstall the app that is no longer in the System App
+  // list.
+  EXPECT_EQ(std::vector<GURL>({GURL(kWindowedUrl)}),
+            pending_app_manager->uninstall_requests());
+}
+
+// Test that System Apps do install with the feature enabled.
+TEST_F(SystemWebAppManagerTest, Enabled) {
+  auto pending_app_manager = std::make_unique<TestPendingAppManager>();
+
+  std::vector<PendingAppManager::AppInfo> system_apps;
+  system_apps.push_back(GetWindowedAppInfo());
+  system_apps.push_back(GetTabbedAppInfo());
+
+  TestSystemWebAppManager system_web_app_manager(
+      profile(), pending_app_manager.get(), std::move(system_apps));
+  base::RunLoop().RunUntilIdle();
+
+  const auto& apps_to_install = pending_app_manager->install_requests();
+  EXPECT_FALSE(apps_to_install.empty());
+}
+
+// Test that changing the set of System Apps uninstalls apps.
+TEST_F(SystemWebAppManagerTest, UninstallAppInstalledInPreviousSession) {
+  auto pending_app_manager = std::make_unique<TestPendingAppManager>();
+
+  // Simulate System Apps and a regular app that were installed in the
+  // previous session.
+  SimulatePreviouslyInstalledApp(
+      pending_app_manager.get(), GURL(kWindowedUrl),
+      PendingAppManager::InstallSource::kSystemInstalled);
+  SimulatePreviouslyInstalledApp(
+      pending_app_manager.get(), GURL(kTabbedUrl),
+      PendingAppManager::InstallSource::kSystemInstalled);
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(),
+                                 GURL(kDefaultContainerUrl),
+                                 PendingAppManager::InstallSource::kInternal);
+  std::vector<PendingAppManager::AppInfo> system_apps;
+  system_apps.push_back(GetWindowedAppInfo());
+
+  TestSystemWebAppManager system_web_app_manager(
+      profile(), pending_app_manager.get(), std::move(system_apps));
+  base::RunLoop().RunUntilIdle();
+
+  // We should only try to install the app in the System App list.
+  std::vector<PendingAppManager::AppInfo> expected_apps_to_install;
+  expected_apps_to_install.push_back(GetWindowedAppInfo());
+  EXPECT_EQ(pending_app_manager->install_requests(), expected_apps_to_install);
+
+  // We should try to uninstall the app that is no longer in the System App
+  // list.
+  EXPECT_EQ(std::vector<GURL>({GURL(kTabbedUrl)}),
+            pending_app_manager->uninstall_requests());
+}
+
+}  // namespace web_app

chrome/browser/web_applications/components/pending_app_manager.h

@@ -88,6 +88,11 @@ class PendingAppManager {
     // The corresponding SynchronizeInstalledApps call site is in
     // WebAppPolicyManager::RefreshPolicyInstalledApps.
     kExternalPolicy = 2,
+    // Installed as a Chrome component, such as a help app, or a settings app.
+    //
+    // The corresponding SynchronizeInstalledApps call site is in
+    // SystemWebAppManager::RefreshPolicyInstalledApps.
+    kSystemInstalled = 3,
   };
 
   struct AppInfo {

chrome/browser/web_applications/extensions/bookmark_app_installation_task.cc

@@ -127,6 +127,9 @@ void BookmarkAppInstallationTask::OnGetWebApplicationInfo(
     case web_app::PendingAppManager::InstallSource::kExternalPolicy:
       helper_->set_is_policy_installed_app();
       break;
+    case web_app::PendingAppManager::InstallSource::kSystemInstalled:
+      helper_->set_is_system_app();
+      break;
   }
 
   if (!app_info_.create_shortcuts)

chrome/browser/web_applications/web_app_provider.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/bookmark_apps/external_web_apps.h"
 #include "chrome/browser/web_applications/bookmark_apps/policy/web_app_policy_manager.h"
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h"
 #include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
 #include "chrome/browser/web_applications/extensions/web_app_extension_ids_map.h"
 #include "chrome/browser/web_applications/web_app_provider_factory.h"
@@ -31,6 +32,9 @@ WebAppProvider::WebAppProvider(Profile* profile)
         profile, pending_app_manager_.get());
   }
 
+  system_web_app_manager_ = std::make_unique<SystemWebAppManager>(
+      profile, pending_app_manager_.get());
+
   registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
                  content::Source<Profile>(profile));
 
@@ -52,6 +56,7 @@ void WebAppProvider::Reset() {
   // PendingAppManager is used by WebAppPolicyManager and therefore should be
   // deleted after it.
   web_app_policy_manager_.reset();
+  system_web_app_manager_.reset();
   pending_app_manager_.reset();
 }
 

chrome/browser/web_applications/web_app_provider.h

@@ -25,6 +25,7 @@ namespace web_app {
 
 class PendingAppManager;
 class WebAppPolicyManager;
+class SystemWebAppManager;
 
 // Connects Web App features, such as the installation of default and
 // policy-managed web apps, with Profiles (as WebAppProvider is a
@@ -57,6 +58,7 @@ class WebAppProvider : public KeyedService,
 
   std::unique_ptr<PendingAppManager> pending_app_manager_;
   std::unique_ptr<WebAppPolicyManager> web_app_policy_manager_;
+  std::unique_ptr<SystemWebAppManager> system_web_app_manager_;
 
   content::NotificationRegistrar registrar_;
 

chrome/common/chrome_features.cc

@@ -566,6 +566,10 @@ const base::Feature kSysInternals{"SysInternals",
                                   base::FEATURE_DISABLED_BY_DEFAULT};
 #endif
 
+// Enables or disables the System Web App manager.
+const base::Feature kSystemWebApps{"SystemWebApps",
+                                   base::FEATURE_DISABLED_BY_DEFAULT};
+
 // Enable TopSites to source and sort its site data using site engagement.
 const base::Feature kTopSitesFromSiteEngagement{
     "TopSitesFromSiteEngagement", base::FEATURE_DISABLED_BY_DEFAULT};

chrome/common/chrome_features.h

@@ -364,6 +364,9 @@ COMPONENT_EXPORT(CHROME_FEATURES)
 extern const base::Feature kSysInternals;
 #endif
 
+COMPONENT_EXPORT(CHROME_FEATURES)
+extern const base::Feature kSystemWebApps;
+
 #if !defined(OS_ANDROID)
 COMPONENT_EXPORT(CHROME_FEATURES) extern const base::Feature kTabMetricsLogging;
 #endif

chrome/common/extensions/manifest_handlers/app_launch_info.cc

@@ -161,8 +161,11 @@ bool AppLaunchInfo::LoadLaunchURL(Extension* extension, base::string16* error) {
     // Ensure the launch web URL is a valid absolute URL and web extent scheme.
     GURL url(launch_url);
     URLPattern pattern(Extension::kValidWebExtentSchemes);
-    if (extension->from_bookmark())
-      pattern.SetValidSchemes(Extension::kValidBookmarkAppSchemes);
+    if (extension->from_bookmark()) {
+      // System Web Apps are bookmark apps that point to chrome:// URLs.
+      pattern.SetValidSchemes(Extension::kValidBookmarkAppSchemes |
+                              URLPattern::SCHEME_CHROMEUI);
+    }
     if ((!url.is_valid() || !pattern.SetScheme(url.scheme()))) {
       *error = ErrorUtils::FormatErrorMessageUTF16(
           errors::kInvalidLaunchValue,

extensions/common/extension.h

@@ -158,7 +158,7 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   // Valid schemes for web extent URLPatterns.
   static const int kValidWebExtentSchemes;
 
-  // Valid schemes for bookmark app installs.
+  // Valid schemes for bookmark app installs by the user.
   static const int kValidBookmarkAppSchemes;
 
   // Valid schemes for host permission URLPatterns.