[System PWAs] Allow System Web Apps to install as PWAs.

This CL alters the System App installation pipeline in order to allow
the full PWA setup experience to succeed for System Apps (mostly
adding exceptions for chrome:// URLs), allowing System Apps
to be installed with a scope. It also adds a browser test which
installs a System PWA end to end.

Bug: 836128
Change-Id: Ida32fa45484661d32a9607b44e06f28f272d4cc3
Reviewed-on: https://chromium-review.googlesource.com/c/1272636
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601837}

chrome/browser/extensions/bookmark_app_helper.h

@@ -108,7 +108,7 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   // If called, the installability check won't test for a service worker.
   void set_bypass_service_worker_check() {
-    DCHECK(is_default_app());
+    DCHECK(is_default_app() || is_system_app());
     bypass_service_worker_check_ = true;
   }
 

chrome/browser/extensions/extension_browsertest.cc

@@ -306,7 +306,7 @@ Browser* ExtensionBrowserTest::LaunchAppBrowser(const Extension* extension) {
 
 base::FilePath ExtensionBrowserTest::PackExtension(
     const base::FilePath& dir_path,
-    ExtensionCreator::RunFlags extra_run_flags) {
+    int extra_run_flags) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   base::FilePath crx_path = temp_dir_.GetPath().AppendASCII("temp.crx");
   if (!base::DeleteFile(crx_path, false)) {
@@ -337,7 +337,7 @@ base::FilePath ExtensionBrowserTest::PackExtensionWithOptions(
     const base::FilePath& crx_path,
     const base::FilePath& pem_path,
     const base::FilePath& pem_out_path,
-    ExtensionCreator::RunFlags extra_run_flags) {
+    int extra_run_flags) {
   base::ScopedAllowBlockingForTesting allow_blocking;
   if (!base::PathExists(dir_path)) {
     ADD_FAILURE() << "Extension dir not found: " << dir_path.value();
@@ -451,9 +451,12 @@ const Extension* ExtensionBrowserTest::InstallOrUpdateExtension(
     //                 and then always pack the extension here.
     base::FilePath crx_path = path;
     if (crx_path.Extension() != FILE_PATH_LITERAL(".crx")) {
-      ExtensionCreator::RunFlags run_flags = ExtensionCreator::kNoRunFlags;
-      if (creation_flags & Extension::FROM_BOOKMARK)
+      int run_flags = ExtensionCreator::kNoRunFlags;
+      if (creation_flags & Extension::FROM_BOOKMARK) {
         run_flags = ExtensionCreator::kBookmarkApp;
+        if (install_source == Manifest::EXTERNAL_COMPONENT)
+          run_flags |= ExtensionCreator::kSystemApp;
+      }
 
       crx_path = PackExtension(path, run_flags);
     }

chrome/browser/extensions/extension_browsertest.h

@@ -135,9 +135,9 @@ class ExtensionBrowserTest : virtual public InProcessBrowserTest {
 
   // Pack the extension in |dir_path| into a crx file and return its path.
   // Return an empty FilePath if there were errors.
-  base::FilePath PackExtension(const base::FilePath& dir_path,
-                               ExtensionCreator::RunFlags extra_run_flags =
-                                   ExtensionCreator::kNoRunFlags);
+  base::FilePath PackExtension(
+      const base::FilePath& dir_path,
+      int extra_run_flags = ExtensionCreator::kNoRunFlags);
 
   // Pack the extension in |dir_path| into a crx file at |crx_path|, using the
   // key |pem_path|. If |pem_path| does not exist, create a new key at
@@ -148,8 +148,7 @@ class ExtensionBrowserTest : virtual public InProcessBrowserTest {
       const base::FilePath& crx_path,
       const base::FilePath& pem_path,
       const base::FilePath& pem_out_path,
-      ExtensionCreator::RunFlags extra_run_flags =
-          ExtensionCreator::kNoRunFlags);
+      int extra_run_flags = ExtensionCreator::kNoRunFlags);
 
   // |expected_change| indicates how many extensions should be installed (or
   // disabled, if negative).

chrome/browser/installable/installable_logging.h

@@ -17,41 +17,41 @@ class WebContents;
 // and utilise LogErrorToConsole to write a message to the devtools console.
 // This enum backs an UMA histogram, so it must be treated as append-only.
 enum InstallableStatusCode {
-  NO_ERROR_DETECTED,
-  RENDERER_EXITING,
-  RENDERER_CANCELLED,
-  USER_NAVIGATED,
-  NOT_IN_MAIN_FRAME,
-  NOT_FROM_SECURE_ORIGIN,
-  NO_MANIFEST,
-  MANIFEST_EMPTY,
-  START_URL_NOT_VALID,
-  MANIFEST_MISSING_NAME_OR_SHORT_NAME,
-  MANIFEST_DISPLAY_NOT_SUPPORTED,
-  MANIFEST_MISSING_SUITABLE_ICON,
-  NO_MATCHING_SERVICE_WORKER,
-  NO_ACCEPTABLE_ICON,
-  CANNOT_DOWNLOAD_ICON,
-  NO_ICON_AVAILABLE,
-  PLATFORM_NOT_SUPPORTED_ON_ANDROID,
-  NO_ID_SPECIFIED,
-  IDS_DO_NOT_MATCH,
-  ALREADY_INSTALLED,
-  INSUFFICIENT_ENGAGEMENT,
-  PACKAGE_NAME_OR_START_URL_EMPTY,
-  PREVIOUSLY_BLOCKED,
-  PREVIOUSLY_IGNORED,
-  SHOWING_NATIVE_APP_BANNER,
-  SHOWING_WEB_APP_BANNER,
-  FAILED_TO_CREATE_BANNER,
-  URL_NOT_SUPPORTED_FOR_WEBAPK,
-  IN_INCOGNITO,
-  NOT_OFFLINE_CAPABLE,
-  WAITING_FOR_MANIFEST,
-  WAITING_FOR_INSTALLABLE_CHECK,
-  NO_GESTURE,
-  WAITING_FOR_NATIVE_DATA,
-  SHOWING_APP_INSTALLATION_DIALOG,
+  NO_ERROR_DETECTED = 0,
+  RENDERER_EXITING = 1,
+  RENDERER_CANCELLED = 2,
+  USER_NAVIGATED = 3,
+  NOT_IN_MAIN_FRAME = 4,
+  NOT_FROM_SECURE_ORIGIN = 5,
+  NO_MANIFEST = 6,
+  MANIFEST_EMPTY = 7,
+  START_URL_NOT_VALID = 8,
+  MANIFEST_MISSING_NAME_OR_SHORT_NAME = 9,
+  MANIFEST_DISPLAY_NOT_SUPPORTED = 10,
+  MANIFEST_MISSING_SUITABLE_ICON = 11,
+  NO_MATCHING_SERVICE_WORKER = 12,
+  NO_ACCEPTABLE_ICON = 13,
+  CANNOT_DOWNLOAD_ICON = 14,
+  NO_ICON_AVAILABLE = 15,
+  PLATFORM_NOT_SUPPORTED_ON_ANDROID = 16,
+  NO_ID_SPECIFIED = 17,
+  IDS_DO_NOT_MATCH = 18,
+  ALREADY_INSTALLED = 19,
+  INSUFFICIENT_ENGAGEMENT = 20,
+  PACKAGE_NAME_OR_START_URL_EMPTY = 21,
+  PREVIOUSLY_BLOCKED = 22,
+  PREVIOUSLY_IGNORED = 23,
+  SHOWING_NATIVE_APP_BANNER = 24,
+  SHOWING_WEB_APP_BANNER = 25,
+  FAILED_TO_CREATE_BANNER = 26,
+  URL_NOT_SUPPORTED_FOR_WEBAPK = 27,
+  IN_INCOGNITO = 28,
+  NOT_OFFLINE_CAPABLE = 29,
+  WAITING_FOR_MANIFEST = 30,
+  WAITING_FOR_INSTALLABLE_CHECK = 31,
+  NO_GESTURE = 32,
+  WAITING_FOR_NATIVE_DATA = 33,
+  SHOWING_APP_INSTALLATION_DIALOG = 34,
   MAX_ERROR_CODE,
 };
 

chrome/browser/installable/installable_manager.cc

@@ -17,6 +17,7 @@
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/storage_partition.h"
+#include "content/public/common/url_constants.h"
 #include "net/base/url_util.h"
 #include "third_party/blink/public/common/manifest/manifest_icon_selector.h"
 #include "third_party/blink/public/common/manifest/web_display_mode.h"
@@ -68,6 +69,10 @@ bool IsContentSecure(content::WebContents* web_contents) {
   if (!web_contents)
     return false;
 
+  // chrome:// URLs are considered secure.
+  if (web_contents->GetVisibleURL().scheme() == content::kChromeUIScheme)
+    return true;
+
   // Whitelist localhost. Check the VisibleURL to match what the
   // SecurityStateTabHelper looks at.
   if (net::IsLocalhost(web_contents->GetVisibleURL()))

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -351,7 +351,25 @@ class HostedAppTest
             : extensions::Extension::NO_FLAGS);
     ASSERT_TRUE(app_);
 
-    // Launch it in a window.
+    LaunchApp();
+  }
+
+  void SetupSystemAppWithURL(const GURL& app_url) {
+    extensions::TestExtensionDir test_app_dir;
+    test_app_dir.WriteManifest(
+        base::StringPrintf(kAppDotComManifest, app_url.spec().c_str()));
+
+    app_ = InstallExtensionWithSourceAndFlags(
+        test_app_dir.UnpackedPath(), 1,
+        extensions::Manifest::EXTERNAL_COMPONENT,
+        extensions::Extension::FROM_BOOKMARK);
+    ASSERT_TRUE(app_);
+
+    LaunchApp();
+  }
+
+  void LaunchApp() {
+    // Launch app in a window.
     app_browser_ = LaunchAppBrowser(app_);
     ASSERT_TRUE(app_browser_);
     ASSERT_TRUE(app_browser_ != browser());
@@ -1424,7 +1442,7 @@ IN_PROC_BROWSER_TEST_P(HostedAppPWAOnlyTest,
                        ShouldShowLocationBarForSystemApp) {
   const GURL app_url(chrome::kChromeUISettingsURL);
 
-  SetupAppWithURL(app_url);
+  SetupSystemAppWithURL(app_url);
 
   // Navigate to the app's launch page; the location bar should be hidden.
   NavigateAndCheckForLocationBar(app_browser_, app_url, false);

chrome/browser/web_applications/BUILD.gn

@@ -108,6 +108,7 @@ source_set("browser_tests") {
 
   deps = [
     ":web_app_group",
+    "//chrome/browser/web_applications/bookmark_apps:browser_tests",
     "//chrome/browser/web_applications/extensions:browser_tests",
   ]
 }

chrome/browser/web_applications/bookmark_apps/BUILD.gn

@@ -32,6 +32,21 @@ source_set("bookmark_apps") {
   ]
 }
 
+source_set("test_support") {
+  testonly = true
+
+  sources = [
+    "test_system_web_app_manager.cc",
+    "test_system_web_app_manager.h",
+  ]
+
+  deps = [
+    ":bookmark_apps",
+    "//base",
+    "//url",
+  ]
+}
+
 source_set("unit_tests") {
   testonly = true
 
@@ -43,6 +58,7 @@ source_set("unit_tests") {
 
   deps = [
     ":bookmark_apps",
+    ":test_support",
     "//base",
     "//chrome/browser",
     "//chrome/browser/web_applications:web_app_group",
@@ -62,3 +78,28 @@ source_set("unit_tests") {
     "//url",
   ]
 }
+
+source_set("browser_tests") {
+  testonly = true
+
+  defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]
+
+  sources = [
+    "system_web_app_manager_browsertest.cc",
+  ]
+
+  deps = [
+    ":bookmark_apps",
+    ":test_support",
+    "//base",
+    "//base/test:test_support",
+    "//chrome/app/theme:chrome_unscaled_resources_grit",
+    "//chrome/browser/web_applications:web_applications_on_extensions",
+    "//chrome/browser/web_applications/components",
+    "//chrome/browser/web_applications/extensions",
+    "//chrome/common:constants",
+    "//chrome/test:test_support",
+    "//extensions/browser",
+    "//url",
+  ]
+}

chrome/browser/web_applications/bookmark_apps/system_web_app_manager.cc

@@ -25,6 +25,22 @@
 
 namespace web_app {
 
+namespace {
+
+PendingAppManager::AppInfo CreateAppInfoForSystemApp(const GURL& url) {
+  DCHECK_EQ(content::kChromeUIScheme, url.scheme());
+  return {
+      url,
+      LaunchContainer::kWindow,
+      InstallSource::kSystemInstalled,
+      false /* create_shortcuts */,
+      PendingAppManager::AppInfo::kDefaultOverridePreviousUserUninstall,
+      true /* bypass_service_worker_check */,
+  };
+}
+
+}  // namespace
+
 SystemWebAppManager::SystemWebAppManager(Profile* profile,
                                          PendingAppManager* pending_app_manager)
     : profile_(profile), pending_app_manager_(pending_app_manager) {
@@ -49,32 +65,30 @@ bool SystemWebAppManager::ShouldEnableForProfile(Profile* profile) {
 }
 
 void SystemWebAppManager::StartAppInstallation() {
-  std::vector<PendingAppManager::AppInfo> apps_to_install;
-
+  std::vector<GURL> urls_to_install;
   if (ShouldEnableForProfile(profile_)) {
     // Skipping this will uninstall all System Apps currently installed.
-    apps_to_install = CreateSystemWebApps();
+    urls_to_install = CreateSystemWebApps();
   }
 
+  std::vector<PendingAppManager::AppInfo> apps_to_install;
+  for (const auto& url : urls_to_install)
+    apps_to_install.emplace_back(CreateAppInfoForSystemApp(url));
+
   pending_app_manager_->SynchronizeInstalledApps(
       std::move(apps_to_install), InstallSource::kSystemInstalled);
 }
 
-std::vector<PendingAppManager::AppInfo>
-SystemWebAppManager::CreateSystemWebApps() {
-  std::vector<PendingAppManager::AppInfo> app_infos;
+std::vector<GURL> SystemWebAppManager::CreateSystemWebApps() {
+  std::vector<GURL> urls;
 
 // TODO(calamity): Split this into per-platform functions.
 #if defined(OS_CHROMEOS)
-  app_infos.emplace_back(
-      GURL(chrome::kChromeUIDiscoverURL), LaunchContainer::kWindow,
-      InstallSource::kSystemInstalled, false /* create_shortcuts */);
-  app_infos.emplace_back(
-      GURL(chrome::kChromeUISettingsURL), LaunchContainer::kWindow,
-      InstallSource::kSystemInstalled, false /* create_shortcuts */);
+  urls.emplace_back(chrome::kChromeUIDiscoverURL);
+  urls.emplace_back(chrome::kChromeUISettingsURL);
 #endif  // OS_CHROMEOS
 
-  return app_infos;
+  return urls;
 }
 
 }  // namespace web_app

chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h

@@ -29,8 +29,7 @@ class SystemWebAppManager {
 
  protected:
   // Overridden in tests.
-  virtual std::vector<web_app::PendingAppManager::AppInfo>
-  CreateSystemWebApps();
+  virtual std::vector<GURL> CreateSystemWebApps();
 
  private:
   void StartAppInstallation();

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_browsertest.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 <vector>
+
+#include "base/memory/ref_counted_memory.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/web_applications/bookmark_apps/test_system_web_app_manager.h"
+#include "chrome/browser/web_applications/components/web_app_constants.h"
+#include "chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/browser/web_applications/web_app_provider_factory.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/extensions/manifest_handlers/app_theme_color_info.h"
+#include "chrome/grit/chrome_unscaled_resources.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_ui.h"
+#include "content/public/browser/web_ui_controller.h"
+#include "content/public/browser/web_ui_controller_factory.h"
+#include "content/public/browser/web_ui_data_source.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/test_extension_registry_observer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace web_app {
+
+constexpr char kSystemAppManifestText[] =
+    R"({
+      "name": "Test System App",
+      "display": "standalone",
+      "icons": [
+        {
+          "src": "icon-256.png",
+          "sizes": "256x256",
+          "type": "image/png"
+        }
+      ],
+      "start_url": "/",
+      "theme_color": "#00FF00"
+    })";
+
+constexpr char kSystemAppHTMLText[] =
+    R"(<html><head><link rel="manifest" href="manifest.json"></head></html>)";
+
+// WebUIController that serves a System PWA.
+class TestWebUIController : public content::WebUIController {
+ public:
+  explicit TestWebUIController(content::WebUI* web_ui)
+      : WebUIController(web_ui) {
+    content::WebUIDataSource* data_source =
+        content::WebUIDataSource::Create("test-system-app");
+    data_source->AddResourcePath("icon-256.png", IDR_PRODUCT_LOGO_256);
+    data_source->SetRequestFilter(base::BindRepeating(
+        [](const std::string& id,
+           const content::WebUIDataSource::GotDataCallback& callback) {
+          scoped_refptr<base::RefCountedString> ref_contents(
+              new base::RefCountedString);
+          if (id == "manifest.json")
+            ref_contents->data() = kSystemAppManifestText;
+
+          if (id == "pwa.html")
+            ref_contents->data() = kSystemAppHTMLText;
+
+          if (ref_contents->data().empty())
+            return false;
+
+          callback.Run(ref_contents);
+          return true;
+        }));
+    content::WebUIDataSource::Add(web_ui->GetWebContents()->GetBrowserContext(),
+                                  data_source);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestWebUIController);
+};
+
+// WebUIControllerFactory that serves our TestWebUIController.
+class TestWebUIControllerFactory : public content::WebUIControllerFactory {
+ public:
+  TestWebUIControllerFactory() {}
+
+  std::unique_ptr<content::WebUIController> CreateWebUIControllerForURL(
+      content::WebUI* web_ui,
+      const GURL& url) const override {
+    return std::make_unique<TestWebUIController>(web_ui);
+  }
+
+  content::WebUI::TypeID GetWebUIType(content::BrowserContext* browser_context,
+                                      const GURL& url) const override {
+    return reinterpret_cast<content::WebUI::TypeID>(1);
+  }
+
+  bool UseWebUIForURL(content::BrowserContext* browser_context,
+                      const GURL& url) const override {
+    return true;
+  }
+  bool UseWebUIBindingsForURL(content::BrowserContext* browser_context,
+                              const GURL& url) const override {
+    return true;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestWebUIControllerFactory);
+};
+
+class SystemWebAppManagerIntegrationTest
+    : public extensions::ExtensionBrowserTest {
+ public:
+  SystemWebAppManagerIntegrationTest() {
+    scoped_feature_list_.InitWithFeatures({features::kSystemWebApps}, {});
+    content::WebUIControllerFactory::RegisterFactory(&factory_);
+  }
+  ~SystemWebAppManagerIntegrationTest() override {
+    content::WebUIControllerFactory::UnregisterFactoryForTesting(&factory_);
+  }
+
+  void SetUpOnMainThread() override {
+    extensions::ExtensionBrowserTest::SetUpOnMainThread();
+
+    // Reset WebAppProvider so that its SystemWebAppManager doesn't interfere
+    // with tests.
+    WebAppProvider::Get(profile())->Reset();
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  TestWebUIControllerFactory factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemWebAppManagerIntegrationTest);
+};
+
+// Test that System Apps install correctly with a manifest.
+IN_PROC_BROWSER_TEST_F(SystemWebAppManagerIntegrationTest, WithManifest) {
+  std::vector<GURL> system_apps;
+  system_apps.emplace_back(GURL("chrome://test-system-app/pwa.html"));
+  extensions::PendingBookmarkAppManager pending_app_manager(profile());
+  TestSystemWebAppManager system_web_app_manager(
+      profile(), &pending_app_manager, std::move(system_apps));
+  const extensions::Extension* app =
+      extensions::TestExtensionRegistryObserver(
+          extensions::ExtensionRegistry::Get(profile()))
+          .WaitForExtensionInstalled();
+  EXPECT_EQ("Test System App", app->name());
+  EXPECT_EQ(SkColorSetRGB(0, 0xFF, 0),
+            extensions::AppThemeColorInfo::GetThemeColor(app));
+  EXPECT_TRUE(app->from_bookmark());
+  EXPECT_EQ(extensions::Manifest::EXTERNAL_COMPONENT, app->location());
+
+  // The app should be a PWA.
+  EXPECT_EQ(extensions::util::GetInstalledPwaForUrl(
+                profile(), GURL("chrome://test-system-app/")),
+            app);
+}
+
+}  // namespace web_app

chrome/browser/web_applications/bookmark_apps/system_web_app_manager_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/web_applications/bookmark_apps/test_system_web_app_manager.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/components/web_app_constants.h"
@@ -32,43 +33,20 @@ namespace web_app {
 
 namespace {
 
-const char kWindowedUrl[] = "https://windowed.example";
-const char kTabbedUrl[] = "https://tabbed.example";
-const char kDefaultContainerUrl[] = "https://default-container.example";
+const char kAppUrl1[] = "chrome://system-app1";
+const char kAppUrl2[] = "chrome://system-app2";
+const char kAppUrl3[] = "chrome://system-app3";
 
 PendingAppManager::AppInfo GetWindowedAppInfo() {
   return PendingAppManager::AppInfo(
-      GURL(kWindowedUrl), LaunchContainer::kWindow,
-      InstallSource::kSystemInstalled, false /* create_shortcuts */);
-}
-
-PendingAppManager::AppInfo GetTabbedAppInfo() {
-  return PendingAppManager::AppInfo(GURL(kTabbedUrl), LaunchContainer::kTab,
-                                    InstallSource::kSystemInstalled,
-                                    false /* create_shortcuts */);
+      GURL(kAppUrl1), LaunchContainer::kWindow, InstallSource::kSystemInstalled,
+      false /* create_shortcuts */,
+      PendingAppManager::AppInfo::kDefaultOverridePreviousUserUninstall,
+      true /* bypass_service_worker_check */);
 }
 
 }  // 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<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;
@@ -111,11 +89,11 @@ TEST_F(SystemWebAppManagerTest, Disabled) {
 
   auto pending_app_manager = std::make_unique<TestPendingAppManager>();
 
-  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kWindowedUrl),
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kAppUrl1),
                                  InstallSource::kSystemInstalled);
 
-  std::vector<PendingAppManager::AppInfo> system_apps;
-  system_apps.push_back(GetWindowedAppInfo());
+  std::vector<GURL> system_apps;
+  system_apps.push_back(GURL(kAppUrl1));
 
   TestSystemWebAppManager system_web_app_manager(
       profile(), pending_app_manager.get(), std::move(system_apps));
@@ -125,7 +103,7 @@ TEST_F(SystemWebAppManagerTest, Disabled) {
 
   // We should try to uninstall the app that is no longer in the System App
   // list.
-  EXPECT_EQ(std::vector<GURL>({GURL(kWindowedUrl)}),
+  EXPECT_EQ(std::vector<GURL>({GURL(kAppUrl1)}),
             pending_app_manager->uninstall_requests());
 }
 
@@ -133,9 +111,9 @@ TEST_F(SystemWebAppManagerTest, Disabled) {
 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());
+  std::vector<GURL> system_apps;
+  system_apps.push_back(GURL(kAppUrl1));
+  system_apps.push_back(GURL(kAppUrl2));
 
   TestSystemWebAppManager system_web_app_manager(
       profile(), pending_app_manager.get(), std::move(system_apps));
@@ -151,15 +129,14 @@ TEST_F(SystemWebAppManagerTest, UninstallAppInstalledInPreviousSession) {
 
   // Simulate System Apps and a regular app that were installed in the
   // previous session.
-  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kWindowedUrl),
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kAppUrl1),
                                  InstallSource::kSystemInstalled);
-  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kTabbedUrl),
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kAppUrl2),
                                  InstallSource::kSystemInstalled);
-  SimulatePreviouslyInstalledApp(pending_app_manager.get(),
-                                 GURL(kDefaultContainerUrl),
+  SimulatePreviouslyInstalledApp(pending_app_manager.get(), GURL(kAppUrl3),
                                  InstallSource::kInternal);
-  std::vector<PendingAppManager::AppInfo> system_apps;
-  system_apps.push_back(GetWindowedAppInfo());
+  std::vector<GURL> system_apps;
+  system_apps.push_back(GURL(kAppUrl1));
 
   TestSystemWebAppManager system_web_app_manager(
       profile(), pending_app_manager.get(), std::move(system_apps));
@@ -172,7 +149,7 @@ TEST_F(SystemWebAppManagerTest, UninstallAppInstalledInPreviousSession) {
 
   // We should try to uninstall the app that is no longer in the System App
   // list.
-  EXPECT_EQ(std::vector<GURL>({GURL(kTabbedUrl)}),
+  EXPECT_EQ(std::vector<GURL>({GURL(kAppUrl2)}),
             pending_app_manager->uninstall_requests());
 }
 

chrome/browser/web_applications/bookmark_apps/test_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/test_system_web_app_manager.h"
+
+namespace web_app {
+
+TestSystemWebAppManager::TestSystemWebAppManager(
+    Profile* profile,
+    PendingAppManager* pending_app_manager,
+    std::vector<GURL> system_apps)
+    : SystemWebAppManager(profile, pending_app_manager),
+      system_apps_(std::move(system_apps)) {}
+
+TestSystemWebAppManager::~TestSystemWebAppManager() = default;
+
+std::vector<GURL> TestSystemWebAppManager::CreateSystemWebApps() {
+  return std::move(system_apps_);
+}
+
+}  // namespace web_app

chrome/browser/web_applications/bookmark_apps/test_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_TEST_SYSTEM_WEB_APP_MANAGER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_TEST_SYSTEM_WEB_APP_MANAGER_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "chrome/browser/web_applications/bookmark_apps/system_web_app_manager.h"
+#include "url/gurl.h"
+
+class Profile;
+
+namespace web_app {
+
+class TestSystemWebAppManager : public SystemWebAppManager {
+ public:
+  TestSystemWebAppManager(Profile* profile,
+                          PendingAppManager* pending_app_manager,
+                          std::vector<GURL> system_apps);
+  ~TestSystemWebAppManager() override;
+
+  // Overridden from SystemWebAppManager:
+  std::vector<GURL> CreateSystemWebApps() override;
+
+ private:
+  std::vector<GURL> system_apps_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSystemWebAppManager);
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_BOOKMARK_APPS_TEST_SYSTEM_WEB_APP_MANAGER_H_

chrome/browser/web_applications/components/pending_app_manager.h

@@ -62,8 +62,8 @@ class PendingAppManager {
     const bool create_shortcuts;
     const bool override_previous_user_uninstall;
 
-    // This must only be used by pre-installed default apps that are valid PWAs
-    // if loading the real service worker is too costly to verify
+    // This must only be used by pre-installed default or system apps that are
+    // valid PWAs if loading the real service worker is too costly to verify
     // programmatically.
     const bool bypass_service_worker_check;
 

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_browsertest.cc

@@ -16,7 +16,10 @@
 #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/extensions/api/url_handlers/url_handlers_parser.h"
+#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/common/url_constants.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -117,4 +120,28 @@ IN_PROC_BROWSER_TEST_F(PendingBookmarkAppManagerBrowserTest,
   EXPECT_FALSE(app);
 }
 
+// Test that adding a manifest that points to a chrome:// URL does not actually
+// install a bookmark app that points to a chrome:// URL.
+IN_PROC_BROWSER_TEST_F(PendingBookmarkAppManagerBrowserTest,
+                       InstallChromeURLFails) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url(embedded_test_server()->GetURL(
+      "/banners/manifest_test_page.html?manifest=manifest_chrome_url.json"));
+  InstallApp(url);
+  EXPECT_EQ(web_app::InstallResultCode::kSuccess, result_code_.value());
+  base::Optional<std::string> id =
+      web_app::ExtensionIdsMap(browser()->profile()->GetPrefs())
+          .LookupExtensionId(url);
+  ASSERT_TRUE(id.has_value());
+  const Extension* app = ExtensionRegistry::Get(browser()->profile())
+                             ->enabled_extensions()
+                             .GetByID(id.value());
+  ASSERT_TRUE(app);
+
+  // The installer falls back to installing a bookmark app of the original URL.
+  EXPECT_EQ(url, extensions::AppLaunchInfo::GetLaunchWebURL(app));
+  EXPECT_FALSE(extensions::UrlHandlers::CanBookmarkAppHandleUrl(
+      app, GURL("chrome://settings")));
+}
+
 }  // namespace extensions

chrome/common/extensions/api/url_handlers/url_handlers_parser.cc

@@ -109,7 +109,8 @@ UrlHandlersParser::~UrlHandlersParser() {
 bool ParseUrlHandler(const std::string& handler_id,
                      const base::DictionaryValue& handler_info,
                      std::vector<UrlHandlerInfo>* url_handlers,
-                     base::string16* error) {
+                     base::string16* error,
+                     Extension* extension) {
   DCHECK(error);
 
   UrlHandlerInfo handler;
@@ -134,8 +135,14 @@ bool ParseUrlHandler(const std::string& handler_id,
     // TODO(sergeygs): Limit this to non-top-level domains.
     // TODO(sergeygs): Also add a verification to the CWS installer that the
     // URL patterns claimed here belong to the app's author verified sites.
-    URLPattern pattern(URLPattern::SCHEME_HTTP |
-                       URLPattern::SCHEME_HTTPS);
+    URLPattern pattern(URLPattern::SCHEME_HTTP | URLPattern::SCHEME_HTTPS);
+    // System Web Apps are bookmark apps that point to chrome:// URLs.
+    // TODO(calamity): Remove once Bookmark Apps are no longer on Extensions.
+    if (extension->location() == Manifest::EXTERNAL_COMPONENT &&
+        extension->from_bookmark()) {
+      pattern = URLPattern(URLPattern::SCHEME_CHROMEUI);
+    }
+
     if (pattern.Parse(str_pattern) != URLPattern::ParseResult::kSuccess) {
       *error = ErrorUtils::FormatErrorMessageUTF16(
           merrors::kInvalidURLHandlerPatternElement, handler_id);
@@ -174,7 +181,8 @@ bool UrlHandlersParser::Parse(Extension* extension, base::string16* error) {
       return false;
     }
 
-    if (!ParseUrlHandler(iter.key(), *handler, &info->handlers, error)) {
+    if (!ParseUrlHandler(iter.key(), *handler, &info->handlers, error,
+                         extension)) {
       // Text in |error| is set by ParseUrlHandler.
       return false;
     }

chrome/common/extensions/manifest_handlers/app_launch_info.cc

@@ -163,8 +163,10 @@ bool AppLaunchInfo::LoadLaunchURL(Extension* extension, base::string16* error) {
     URLPattern pattern(Extension::kValidWebExtentSchemes);
     if (extension->from_bookmark()) {
       // System Web Apps are bookmark apps that point to chrome:// URLs.
-      pattern.SetValidSchemes(Extension::kValidBookmarkAppSchemes |
-                              URLPattern::SCHEME_CHROMEUI);
+      int valid_schemes = Extension::kValidBookmarkAppSchemes;
+      if (extension->location() == Manifest::EXTERNAL_COMPONENT)
+        valid_schemes |= URLPattern::SCHEME_CHROMEUI;
+      pattern.SetValidSchemes(valid_schemes);
     }
     if ((!url.is_valid() || !pattern.SetScheme(url.scheme()))) {
       *error = ErrorUtils::FormatErrorMessageUTF16(

chrome/test/BUILD.gn

@@ -295,6 +295,11 @@ static_library("test_support") {
   }
 
   if (enable_extensions) {
+    sources += [
+      "../browser/extensions/extension_browsertest.cc",
+      "../browser/extensions/extension_browsertest.h",
+    ]
+
     public_deps += [
       "//apps:test_support",
       "//chrome/common/extensions/api",
@@ -1343,8 +1348,6 @@ test("browser_tests") {
         "../browser/extensions/extension_apitest.h",
         "../browser/extensions/extension_bindings_apitest.cc",
         "../browser/extensions/extension_bookmarklet_browsertest.cc",
-        "../browser/extensions/extension_browsertest.cc",
-        "../browser/extensions/extension_browsertest.h",
         "../browser/extensions/extension_context_menu_browsertest.cc",
         "../browser/extensions/extension_csp_bypass_browsertest.cc",
         "../browser/extensions/extension_disabled_ui_browsertest.cc",

chrome/test/data/banners/manifest_chrome_url.json

+{
+  "short_name": "Manifest",
+  "name": "",
+  "icons": [
+    {
+      "src": "image-512px.png",
+      "sizes": "512x512",
+      "type": "image/png"
+    }
+  ],
+  "start_url": "chrome://settings",
+  "display": "standalone",
+  "orientation": "landscape"
+}

extensions/browser/extension_creator.cc

@@ -110,9 +110,11 @@ bool ExtensionCreator::ValidateManifest(const base::FilePath& extension_dir,
   if (run_flags & kBookmarkApp)
     create_flags |= Extension::FROM_BOOKMARK;
 
-  scoped_refptr<Extension> extension(
-      file_util::LoadExtension(extension_dir, extension_id, Manifest::INTERNAL,
-                               create_flags, &error_message_));
+  scoped_refptr<Extension> extension(file_util::LoadExtension(
+      extension_dir, extension_id,
+      run_flags & kSystemApp ? Manifest::EXTERNAL_COMPONENT
+                             : Manifest::INTERNAL,
+      create_flags, &error_message_));
   return !!extension.get();
 }
 

extensions/browser/extension_creator.h

@@ -38,6 +38,7 @@ class ExtensionCreator {
     kOverwriteCRX = 1 << 0,
     kRequireModernManifestVersion = 1 << 1,
     kBookmarkApp = 1 << 2,
+    kSystemApp = 1 << 3,
   };
 
   // Categories of error that may need special handling on the UI end.