dpwas: Move app filtering from ParseConfig() to PostProcessConfigs()

This CL moves the filtering of apps by:
 - feature_name
 - user_type,
 - disable_if_arc_supported
 - disable_if_tablet_form_factor
from ParseConfig() into PostProcessConfigs().
This is to be able to share that logic with apps added to
GetPreinstalledWebApps().

There are no behavioural changes in this CL aside from minor fixups
in ExternalInstallOptions::operator==() and its operator<<().

Bug: 1058265
Change-Id: I6a2f3d89f076269e612d0806fb9e3abd37536d24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2520559
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Auto-Submit: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825264}

chrome/browser/web_applications/BUILD.gn

@@ -187,7 +187,6 @@ source_set("web_applications_unit_tests") {
     "external_web_app_manager_unittest.cc",
     "external_web_app_utils_unittest.cc",
     "pending_app_manager_impl_unittest.cc",
-    "preinstalled_web_apps_unittest.cc",
     "system_web_app_manager_unittest.cc",
     "web_app_database_unittest.cc",
     "web_app_icon_manager_unittest.cc",

chrome/browser/web_applications/components/external_install_options.cc

@@ -37,65 +37,111 @@ bool ExternalInstallOptions::operator==(
   return std::tie(install_url, user_display_mode, install_source,
                   add_to_applications_menu, add_to_desktop,
                   add_to_quick_launch_bar, add_to_search, add_to_management,
-                  is_disabled, override_previous_user_uninstall,
+                  run_on_os_login, is_disabled,
+                  override_previous_user_uninstall, only_for_new_users,
+                  user_type_allowlist, gate_on_feature,
+#if defined(OS_CHROMEOS)
+                  disable_if_arc_supported, disable_if_tablet_form_factor,
+#endif  // defined(OS_CHROMEOS)
                   bypass_service_worker_check, require_manifest,
                   force_reinstall, wait_for_windows_closed, install_placeholder,
-                  reinstall_placeholder,
+                  reinstall_placeholder, launch_query_params,
                   load_and_await_service_worker_registration,
                   service_worker_registration_url, uninstall_and_replace,
                   additional_search_terms, only_use_app_info_factory) ==
-         std::tie(other.install_url, other.user_display_mode,
-                  other.install_source, other.add_to_applications_menu,
-                  other.add_to_desktop, other.add_to_quick_launch_bar,
-                  other.add_to_search, other.add_to_management,
-                  other.is_disabled, other.override_previous_user_uninstall,
-                  other.bypass_service_worker_check, other.require_manifest,
-                  other.force_reinstall, other.wait_for_windows_closed,
-                  other.install_placeholder, other.reinstall_placeholder,
-                  other.load_and_await_service_worker_registration,
-                  other.service_worker_registration_url,
-                  other.uninstall_and_replace, other.additional_search_terms,
-                  other.only_use_app_info_factory);
+         std::tie(
+             other.install_url, other.user_display_mode, other.install_source,
+             other.add_to_applications_menu, other.add_to_desktop,
+             other.add_to_quick_launch_bar, other.add_to_search,
+             other.add_to_management, other.run_on_os_login, other.is_disabled,
+             other.override_previous_user_uninstall, other.only_for_new_users,
+             other.user_type_allowlist, other.gate_on_feature,
+#if defined(OS_CHROMEOS)
+             other.disable_if_arc_supported,
+             other.disable_if_tablet_form_factor,
+#endif  // defined(OS_CHROMEOS)
+             other.bypass_service_worker_check, other.require_manifest,
+             other.force_reinstall, other.wait_for_windows_closed,
+             other.install_placeholder, other.reinstall_placeholder,
+             other.launch_query_params,
+             other.load_and_await_service_worker_registration,
+             other.service_worker_registration_url, other.uninstall_and_replace,
+             other.additional_search_terms, other.only_use_app_info_factory);
 }
 
+namespace {
+
+template <typename T>
+std::ostream& operator<<(std::ostream& out, const base::Optional<T>& optional) {
+  if (optional)
+    out << *optional;
+  else
+    out << "nullopt";
+  return out;
+}
+
+template <typename T>
+std::ostream& operator<<(std::ostream& out, const std::vector<T>& list) {
+  out << '[';
+  for (size_t i = 0; i < list.size(); ++i) {
+    if (i > 0)
+      out << ", ";
+    out << list[i];
+  }
+  out << ']';
+  return out;
+}
+
+}  // namespace
+
 std::ostream& operator<<(std::ostream& out,
                          const ExternalInstallOptions& install_options) {
-  return out << "install_url: " << install_options.install_url
-             << "\n user_display_mode: "
-             << static_cast<int32_t>(install_options.user_display_mode)
-             << "\n install_source: "
-             << static_cast<int32_t>(install_options.install_source)
-             << "\n add_to_applications_menu: "
-             << install_options.add_to_applications_menu
-             << "\n add_to_desktop: " << install_options.add_to_desktop
-             << "\n add_to_quick_launch_bar: "
-             << install_options.add_to_quick_launch_bar
-             << "\n add_to_search: " << install_options.add_to_search
-             << "\n add_to_management: " << install_options.add_to_management
-             << "\n is_disabled: " << install_options.is_disabled
-             << "\n override_previous_user_uninstall: "
-             << install_options.override_previous_user_uninstall
-             << "\n bypass_service_worker_check: "
-             << install_options.bypass_service_worker_check
-             << "\n require_manifest: " << install_options.require_manifest
-             << "\n force_reinstall: " << install_options.force_reinstall
-             << "\n wait_for_windows_closed: "
-             << install_options.wait_for_windows_closed
-             << "\n install_placeholder: "
-             << install_options.install_placeholder
-             << "\n reinstall_placeholder: "
-             << install_options.reinstall_placeholder
-             << "\n load_and_await_service_worker_registration: "
-             << install_options.load_and_await_service_worker_registration
-             << "\n service_worker_registration_url: "
-             << install_options.service_worker_registration_url.value_or(GURL())
-             << "\n uninstall_and_replace:\n  "
-             << base::JoinString(install_options.uninstall_and_replace, "\n  ")
-             << "\n only_use_app_info_factory:\n "
-             << install_options.only_use_app_info_factory
-             << "\n additional_search_terms:\n "
-             << base::JoinString(install_options.additional_search_terms,
-                                 "\n ");
+  return out
+         << "install_url: " << install_options.install_url
+         << "\n user_display_mode: "
+         << static_cast<int32_t>(install_options.user_display_mode)
+         << "\n install_source: "
+         << static_cast<int32_t>(install_options.install_source)
+         << "\n add_to_applications_menu: "
+         << install_options.add_to_applications_menu
+         << "\n add_to_desktop: " << install_options.add_to_desktop
+         << "\n add_to_quick_launch_bar: "
+         << install_options.add_to_quick_launch_bar
+         << "\n add_to_search: " << install_options.add_to_search
+         << "\n add_to_management: " << install_options.add_to_management
+         << "\n run_on_os_login: " << install_options.run_on_os_login
+         << "\n is_disabled: " << install_options.is_disabled
+         << "\n override_previous_user_uninstall: "
+         << install_options.override_previous_user_uninstall
+         << "\n only_for_new_users: " << install_options.only_for_new_users
+         << "\n user_type_allowlist: " << install_options.user_type_allowlist
+         << "\n gate_on_feature: " << install_options.gate_on_feature
+#if defined(OS_CHROMEOS)
+         << "\n disable_if_arc_supported: "
+         << install_options.disable_if_arc_supported
+         << "\n disable_if_tablet_form_factor: "
+         << install_options.disable_if_tablet_form_factor
+#endif  // defined(OS_CHROMEOS)
+         << "\n bypass_service_worker_check: "
+         << install_options.bypass_service_worker_check
+         << "\n require_manifest: " << install_options.require_manifest
+         << "\n force_reinstall: " << install_options.force_reinstall
+         << "\n wait_for_windows_closed: "
+         << install_options.wait_for_windows_closed
+         << "\n install_placeholder: " << install_options.install_placeholder
+         << "\n reinstall_placeholder: "
+         << install_options.reinstall_placeholder
+         << "\n launch_query_params: " << install_options.launch_query_params
+         << "\n load_and_await_service_worker_registration: "
+         << install_options.load_and_await_service_worker_registration
+         << "\n service_worker_registration_url: "
+         << install_options.service_worker_registration_url.value_or(GURL())
+         << "\n uninstall_and_replace:\n  "
+         << base::JoinString(install_options.uninstall_and_replace, "\n  ")
+         << "\n only_use_app_info_factory:\n "
+         << install_options.only_use_app_info_factory
+         << "\n additional_search_terms:\n "
+         << base::JoinString(install_options.additional_search_terms, "\n ");
 }
 
 InstallManager::InstallParams ConvertExternalInstallOptionsToParams(

chrome/browser/web_applications/components/external_install_options.h

@@ -80,6 +80,21 @@ struct ExternalInstallOptions {
   // the first time.
   bool only_for_new_users = false;
 
+  // Which user types this app should be installed for.
+  // See apps::DetermineUserType() for relevant string constants.
+  std::vector<std::string> user_type_allowlist;
+
+  // Which feature flag should be enabled to install this app. See
+  // chrome/browser/web_applications/components/external_app_install_features.h
+  // for available features to gate on.
+  base::Optional<std::string> gate_on_feature;
+
+  // Whether this should not be installed for devices that support ARC.
+  bool disable_if_arc_supported = false;
+
+  // Whether this should not be installed for tablet devices.
+  bool disable_if_tablet_form_factor = false;
+
   // 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.

chrome/browser/web_applications/external_web_app_manager.cc

@@ -28,6 +28,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/apps/user_type_filter.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/components/external_app_install_features.h"
 #include "chrome/browser/web_applications/components/externally_installed_web_app_prefs.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
@@ -46,7 +47,9 @@
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
-#endif
+#include "chromeos/constants/chromeos_switches.h"
+#include "components/arc/arc_util.h"
+#endif  // defined(OS_CHROMEOS)
 
 namespace web_app {
 
@@ -106,12 +109,10 @@ LoadedConfigs LoadConfigsBlocking(const base::FilePath& config_dir) {
 
 struct ParsedConfigs {
   std::vector<ExternalInstallOptions> options_list;
-  int disabled_count = 0;
   int error_count = 0;
 };
 
 ParsedConfigs ParseConfigsBlocking(const base::FilePath& config_dir,
-                                   const std::string& user_type,
                                    LoadedConfigs loaded_configs) {
   ParsedConfigs result;
   result.error_count = loaded_configs.error_count;
@@ -121,20 +122,12 @@ ParsedConfigs ParseConfigsBlocking(const base::FilePath& config_dir,
                         : std::make_unique<FileUtilsWrapper>();
 
   for (const LoadedConfig& loaded_config : loaded_configs.configs) {
-    ExternalConfigParseResult parse_result =
-        ParseConfig(*file_utils, config_dir, loaded_config.file, user_type,
-                    loaded_config.contents);
-    switch (parse_result.type) {
-      case ExternalConfigParseResult::kEnabled:
-        result.options_list.push_back(std::move(parse_result.options.value()));
-        break;
-      case ExternalConfigParseResult::kDisabled:
-        ++result.disabled_count;
-        break;
-      case ExternalConfigParseResult::kError:
-        ++result.error_count;
-        break;
-    }
+    base::Optional<ExternalInstallOptions> parse_result = ParseConfig(
+        *file_utils, config_dir, loaded_config.file, loaded_config.contents);
+    if (parse_result)
+      result.options_list.push_back(std::move(*parse_result));
+    else
+      ++result.error_count;
   }
 
   return result;
@@ -242,7 +235,6 @@ void ExternalWebAppManager::ParseConfigs(ConsumeParsedConfigs callback,
       {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
       base::BindOnce(&ParseConfigsBlocking, GetConfigDir(),
-                     apps::DetermineUserType(profile_),
                      std::move(loaded_configs)),
       std::move(callback));
 }
@@ -250,19 +242,44 @@ void ExternalWebAppManager::ParseConfigs(ConsumeParsedConfigs callback,
 void ExternalWebAppManager::PostProcessConfigs(ConsumeInstallOptions callback,
                                                ParsedConfigs parsed_configs) {
   // Add hard coded configs.
-  PreinstalledWebApps preinstalled_web_apps = GetPreinstalledWebApps();
-  for (ExternalInstallOptions& options : preinstalled_web_apps.options)
+  for (ExternalInstallOptions& options : GetPreinstalledWebApps())
     parsed_configs.options_list.push_back(std::move(options));
-  parsed_configs.disabled_count += preinstalled_web_apps.disabled_count;
-
-  // Track this separately as we may remove apps due to user uninstall (not the
-  // same as being disabled).
-  int enabled_count = parsed_configs.options_list.size();
 
+  const int total_count = parsed_configs.options_list.size();
+  int disabled_count = 0;
   bool is_new_user = IsNewUser();
-
+  std::string user_type = apps::DetermineUserType(profile_);
   base::EraseIf(
       parsed_configs.options_list, [&](const ExternalInstallOptions& options) {
+        // Remove if not applicable to current user type.
+        DCHECK_GT(options.user_type_allowlist.size(), 0u);
+        if (!base::Contains(options.user_type_allowlist, user_type)) {
+          ++disabled_count;
+          return true;
+        }
+
+        // Remove if gated on a disabled feature.
+        if (options.gate_on_feature &&
+            !IsExternalAppInstallFeatureEnabled(*options.gate_on_feature)) {
+          ++disabled_count;
+          return true;
+        }
+
+#if defined(OS_CHROMEOS)
+        // Remove if ARC is supported and app should be disabled.
+        if (options.disable_if_arc_supported && arc::IsArcAvailable()) {
+          ++disabled_count;
+          return true;
+        }
+
+        // Remove if device is tablet and app should be disabled.
+        if (options.disable_if_tablet_form_factor &&
+            chromeos::switches::IsTabletFormFactor()) {
+          ++disabled_count;
+          return true;
+        }
+#endif  // defined(OS_CHROMEOS)
+
         // Remove if only for new users, user isn't new and app was not
         // installed previously.
         if (options.only_for_new_users && !is_new_user) {
@@ -277,8 +294,7 @@ void ExternalWebAppManager::PostProcessConfigs(ConsumeInstallOptions callback,
         // Remove if any apps to replace are blocked by admin policy.
         for (const AppId& app_id : options.uninstall_and_replace) {
           if (extensions::IsExtensionBlockedByPolicy(profile_, app_id)) {
-            ++parsed_configs.disabled_count;
-            --enabled_count;
+            ++disabled_count;
             return true;
           }
         }
@@ -299,9 +315,9 @@ void ExternalWebAppManager::PostProcessConfigs(ConsumeInstallOptions callback,
       });
 
   base::UmaHistogramCounts100(ExternalWebAppManager::kHistogramEnabledCount,
-                              enabled_count);
+                              total_count - disabled_count);
   base::UmaHistogramCounts100(ExternalWebAppManager::kHistogramDisabledCount,
-                              parsed_configs.disabled_count);
+                              disabled_count);
   base::UmaHistogramCounts100(ExternalWebAppManager::kHistogramConfigErrorCount,
                               parsed_configs.error_count);
 

chrome/browser/web_applications/external_web_app_manager_unittest.cc

@@ -200,11 +200,11 @@ TEST_F(ExternalWebAppManagerTest, ReplacementExtensionBlockedByPolicy) {
   ScopedTestingPreinstalledAppData scoped_preinstalled_apps;
   GURL install_url("https://test.app");
   constexpr char kExtensionId[] = "abcdefghijklmnopabcdefghijklmnop";
-  scoped_preinstalled_apps.apps.push_back({
-      .install_url = install_url,
-      .feature_name = nullptr,
-      .app_id_to_replace = kExtensionId,
-  });
+  ExternalInstallOptions options(install_url, DisplayMode::kBrowser,
+                                 ExternalInstallSource::kExternalDefault);
+  options.user_type_allowlist = {"unmanaged"};
+  options.uninstall_and_replace = {kExtensionId};
+  scoped_preinstalled_apps.apps.push_back(std::move(options));
 
   auto expect_present = [&]() {
     std::vector<ExternalInstallOptions> options_list =
@@ -251,6 +251,7 @@ TEST_F(ExternalWebAppManagerTest, GoodJson) {
     ExternalInstallOptions install_options(
         GURL("https://www.chromestatus.com/features"), DisplayMode::kBrowser,
         ExternalInstallSource::kExternalDefault);
+    install_options.user_type_allowlist = {"unmanaged"};
     install_options.add_to_applications_menu = true;
     install_options.add_to_search = true;
     install_options.add_to_management = true;
@@ -263,6 +264,7 @@ TEST_F(ExternalWebAppManagerTest, GoodJson) {
     ExternalInstallOptions install_options(
         GURL("https://events.google.com/io2016/?utm_source=web_app_manifest"),
         DisplayMode::kStandalone, ExternalInstallSource::kExternalDefault);
+    install_options.user_type_allowlist = {"unmanaged"};
     install_options.add_to_applications_menu = true;
     install_options.add_to_search = true;
     install_options.add_to_management = true;

chrome/browser/web_applications/external_web_app_migration_browsertest.cc

@@ -424,10 +424,13 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest,
 IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest,
                        MigrateToPreinstalledWebApp) {
   ScopedTestingPreinstalledAppData preinstalled_apps;
-  preinstalled_apps.apps.push_back(
-      {GetWebAppUrl(), kMigrationFlag, kExtensionId});
-  EXPECT_EQ(1, GetPreinstalledWebApps().disabled_count);
-
+  ExternalInstallOptions options(GetWebAppUrl(), DisplayMode::kBrowser,
+                                 ExternalInstallSource::kExternalDefault);
+  options.gate_on_feature = kMigrationFlag;
+  options.user_type_allowlist = {"unmanaged"};
+  options.uninstall_and_replace.push_back(kExtensionId);
+  preinstalled_apps.apps.push_back(std::move(options));
+  EXPECT_EQ(1u, GetPreinstalledWebApps().size());
   // Set up pre-migration state.
   {
     base::HistogramTester histograms;

chrome/browser/web_applications/external_web_app_utils.h

@@ -19,41 +19,11 @@ namespace web_app {
 
 class FileUtilsWrapper;
 
-struct ExternalConfigParseResult {
-  enum Type {
-    kEnabled,
-    kDisabled,
-    kError,
-  };
-
-  static ExternalConfigParseResult Enabled(ExternalInstallOptions options);
-  static ExternalConfigParseResult Disabled();
-  static ExternalConfigParseResult Error();
-
-  ~ExternalConfigParseResult();
-  ExternalConfigParseResult(const ExternalConfigParseResult&) = delete;
-  ExternalConfigParseResult(ExternalConfigParseResult&&);
-  ExternalConfigParseResult& operator=(const ExternalConfigParseResult&) =
-      delete;
-
-  const Type type;
-
-  // Set iff kEnabled.
-  const base::Optional<ExternalInstallOptions> options;
-
- private:
-  ExternalConfigParseResult(Type type,
-                            base::Optional<ExternalInstallOptions> options);
-};
-
-// TODO(https://crbug.com/1128801): Record and log parsing errors more
-// effectively. At the moment they're indistinguishable from disabled apps to
-// the caller.
-ExternalConfigParseResult ParseConfig(FileUtilsWrapper& file_utils,
-                                      const base::FilePath& dir,
-                                      const base::FilePath& file,
-                                      const std::string& user_type,
-                                      const base::Value& app_config);
+base::Optional<ExternalInstallOptions> ParseConfig(
+    FileUtilsWrapper& file_utils,
+    const base::FilePath& dir,
+    const base::FilePath& file,
+    const base::Value& app_config);
 
 base::Optional<WebApplicationInfoFactory> ParseOfflineManifest(
     FileUtilsWrapper& file_utils,

chrome/browser/web_applications/external_web_app_utils_unittest.cc

@@ -38,14 +38,15 @@ class ExternalWebAppUtilsTest : public testing::Test {
     });
   }
 
-  ExternalConfigParseResult ParseConfig(const char* app_config_string) {
+  base::Optional<ExternalInstallOptions> ParseConfig(
+      const char* app_config_string) {
     base::Optional<base::Value> app_config =
         base::JSONReader::Read(app_config_string);
     DCHECK(app_config);
     FileUtilsWrapper file_utils;
     return ::web_app::ParseConfig(file_utils, /*dir=*/base::FilePath(),
                                   /*file=*/base::FilePath(),
-                                  /*user_type=*/"test", app_config.value());
+                                  app_config.value());
   }
 
   base::Optional<WebApplicationInfoFactory> ParseOfflineManifest(
@@ -95,7 +96,7 @@ class ExternalWebAppUtilsTabletTest
 };
 
 TEST_P(ExternalWebAppUtilsTabletTest, DisableIfTabletFormFactor) {
-  ExternalConfigParseResult disable_true_result = ParseConfig(R"(
+  base::Optional<ExternalInstallOptions> disable_true_options = ParseConfig(R"(
     {
       "app_url": "https://test.org",
       "launch_container": "window",
@@ -103,11 +104,9 @@ TEST_P(ExternalWebAppUtilsTabletTest, DisableIfTabletFormFactor) {
       "user_type": ["test"]
     }
   )");
-  EXPECT_EQ(disable_true_result.type,
-            is_tablet() ? ExternalConfigParseResult::kDisabled
-                        : ExternalConfigParseResult::kEnabled);
+  EXPECT_TRUE(disable_true_options->disable_if_tablet_form_factor);
 
-  ExternalConfigParseResult disable_false_result = ParseConfig(R"(
+  base::Optional<ExternalInstallOptions> disable_false_options = ParseConfig(R"(
     {
       "app_url": "https://test.org",
       "launch_container": "window",
@@ -115,8 +114,7 @@ TEST_P(ExternalWebAppUtilsTabletTest, DisableIfTabletFormFactor) {
       "user_type": ["test"]
     }
   )");
-  EXPECT_EQ(disable_false_result.type, ExternalConfigParseResult::kEnabled);
-  EXPECT_TRUE(disable_false_result.options.has_value());
+  EXPECT_FALSE(disable_false_options->disable_if_tablet_form_factor);
 }
 
 INSTANTIATE_TEST_SUITE_P(All,
@@ -140,7 +138,7 @@ class ExternalWebAppUtilsArcTest
 };
 
 TEST_P(ExternalWebAppUtilsArcTest, DisableIfArcSupported) {
-  ExternalConfigParseResult disable_true_result = ParseConfig(R"(
+  base::Optional<ExternalInstallOptions> disable_true_options = ParseConfig(R"(
     {
       "app_url": "https://test.org",
       "launch_container": "window",
@@ -148,11 +146,9 @@ TEST_P(ExternalWebAppUtilsArcTest, DisableIfArcSupported) {
       "user_type": ["test"]
     }
   )");
-  EXPECT_EQ(disable_true_result.type,
-            is_arc_supported() ? ExternalConfigParseResult::kDisabled
-                               : ExternalConfigParseResult::kEnabled);
+  EXPECT_TRUE(disable_true_options->disable_if_arc_supported);
 
-  ExternalConfigParseResult disable_false_result = ParseConfig(R"(
+  base::Optional<ExternalInstallOptions> disable_false_options = ParseConfig(R"(
     {
       "app_url": "https://test.org",
       "launch_container": "window",
@@ -160,8 +156,7 @@ TEST_P(ExternalWebAppUtilsArcTest, DisableIfArcSupported) {
       "user_type": ["test"]
     }
   )");
-  EXPECT_EQ(disable_false_result.type, ExternalConfigParseResult::kEnabled);
-  EXPECT_TRUE(disable_false_result.options.has_value());
+  EXPECT_FALSE(disable_false_options->disable_if_arc_supported);
 }
 
 INSTANTIATE_TEST_SUITE_P(All,

chrome/browser/web_applications/preinstalled_web_apps.cc

@@ -11,18 +11,17 @@
 namespace web_app {
 namespace {
 
-std::vector<PreinstalledAppData>* g_preinstalled_app_data_for_testing = nullptr;
+std::vector<ExternalInstallOptions>* g_preinstalled_app_data_for_testing =
+    nullptr;
 
-std::vector<PreinstalledAppData> GetPreinstalledAppData() {
+std::vector<ExternalInstallOptions> GetPreinstalledAppData() {
   if (g_preinstalled_app_data_for_testing)
     return *g_preinstalled_app_data_for_testing;
 
-  std::vector<PreinstalledAppData> preinstalled_app_data = {
-      // TODO(devlin): Add the web apps that should come preinstalled, gated
-      // by OS.
+  return {
+      // TODO(devlin): Add the web apps that should come preinstalled, gated by
+      // OS.
   };
-
-  return preinstalled_app_data;
 }
 
 }  // namespace
@@ -37,31 +36,20 @@ ScopedTestingPreinstalledAppData::~ScopedTestingPreinstalledAppData() {
   g_preinstalled_app_data_for_testing = nullptr;
 }
 
-PreinstalledWebApps::PreinstalledWebApps() = default;
-PreinstalledWebApps::PreinstalledWebApps(PreinstalledWebApps&&) = default;
-PreinstalledWebApps::~PreinstalledWebApps() = default;
-
-PreinstalledWebApps GetPreinstalledWebApps() {
-  PreinstalledWebApps result;
+std::vector<ExternalInstallOptions> GetPreinstalledWebApps() {
+  std::vector<ExternalInstallOptions> result;
 
-  for (const PreinstalledAppData& app_data : GetPreinstalledAppData()) {
-    if (app_data.feature_name &&
-        !IsExternalAppInstallFeatureEnabled(app_data.feature_name)) {
-      ++result.disabled_count;
-      continue;
-    }
+  for (ExternalInstallOptions& app_data : GetPreinstalledAppData()) {
+    DCHECK_EQ(app_data.install_source, ExternalInstallSource::kExternalDefault);
 
-    ExternalInstallOptions options(app_data.install_url, DisplayMode::kBrowser,
-                                   ExternalInstallSource::kExternalDefault);
     // Preinstalled web apps should not have OS shortcuts of any kind.
-    options.add_to_applications_menu = false;
-    options.add_to_desktop = false;
-    options.add_to_quick_launch_bar = false;
-    options.add_to_search = false;
-    options.add_to_management = false;
-    options.require_manifest = true;
-    options.uninstall_and_replace = {app_data.app_id_to_replace};
-    result.options.push_back(std::move(options));
+    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;

chrome/browser/web_applications/preinstalled_web_apps.h

@@ -5,26 +5,11 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_PREINSTALLED_WEB_APPS_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_PREINSTALLED_WEB_APPS_H_
 
-#include <string>
 #include <vector>
 
 #include "chrome/browser/web_applications/components/external_install_options.h"
-#include "url/gurl.h"
 
 namespace web_app {
-struct ExternalInstallOptions;
-
-// A simple struct that contains the relevant data for web apps that come
-// preinstalled. These are used to generate the ExternalInstallOptions, which
-// in turn are used to install the apps.
-struct PreinstalledAppData {
-  // The install url for the app.
-  GURL install_url;
-  // The name of a feature which must be enabled for the app to be installed.
-  const char* feature_name = nullptr;
-  // The ID of an existing app to uninstall when this app is installed.
-  const char* app_id_to_replace = nullptr;
-};
 
 // A scoped helper to provide a testing set of preinstalled app data. This will
 // replace the default set.
@@ -36,20 +21,11 @@ struct ScopedTestingPreinstalledAppData {
       const ScopedTestingPreinstalledAppData&) = delete;
   ~ScopedTestingPreinstalledAppData();
 
-  std::vector<PreinstalledAppData> apps;
-};
-
-struct PreinstalledWebApps {
-  PreinstalledWebApps();
-  PreinstalledWebApps(PreinstalledWebApps&&);
-  ~PreinstalledWebApps();
-
-  std::vector<ExternalInstallOptions> options;
-  int disabled_count = 0;
+  std::vector<ExternalInstallOptions> apps;
 };
 
 // Returns the list of web apps that should be pre-installed on new profiles.
-PreinstalledWebApps GetPreinstalledWebApps();
+std::vector<ExternalInstallOptions> GetPreinstalledWebApps();
 
 }  // namespace web_app
 

chrome/browser/web_applications/preinstalled_web_apps_unittest.cc

-// Copyright 2020 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/preinstalled_web_apps.h"
-
-#include <memory>
-#include <vector>
-
-#include "base/test/scoped_feature_list.h"
-#include "chrome/browser/web_applications/components/external_app_install_features.h"
-#include "chrome/common/chrome_features.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace web_app {
-namespace {
-
-constexpr char kTestAppId1[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
-constexpr char kTestAppId2[] = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb";
-
-std::unique_ptr<ScopedTestingPreinstalledAppData> CreateStubPreinstalledApps() {
-  auto app_data = std::make_unique<ScopedTestingPreinstalledAppData>();
-  app_data->apps.push_back({GURL("https://one.example"),
-                            kMigrateDefaultChromeAppToWebAppsGSuite.name,
-                            kTestAppId1});
-  app_data->apps.push_back({GURL("https://two.example"),
-                            kMigrateDefaultChromeAppToWebAppsNonGSuite.name,
-                            kTestAppId2});
-  return app_data;
-}
-
-}  // namespace
-
-using PreinstalledWebAppsTest = testing::Test;
-
-TEST(PreinstalledWebAppsTest, AppsOnlyReturnedIfSpecificFeatureEnabled) {
-  auto scoped_preinstalled_apps = CreateStubPreinstalledApps();
-
-  // The preinstalled apps depend on two different features (in addition to
-  // default web app installation).
-  {
-    // With both features disabled, no apps are included.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitWithFeatures({features::kDefaultWebAppInstallation},
-                                  {kMigrateDefaultChromeAppToWebAppsGSuite,
-                                   kMigrateDefaultChromeAppToWebAppsNonGSuite});
-    PreinstalledWebApps preinstalled_web_apps = GetPreinstalledWebApps();
-    EXPECT_EQ(2, preinstalled_web_apps.disabled_count);
-    EXPECT_EQ(0u, preinstalled_web_apps.options.size());
-  }
-
-  {
-    // Enable a single feature; only the corresponding app should be returned.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitWithFeatures({features::kDefaultWebAppInstallation,
-                                   kMigrateDefaultChromeAppToWebAppsGSuite},
-                                  {kMigrateDefaultChromeAppToWebAppsNonGSuite});
-    PreinstalledWebApps preinstalled_web_apps = GetPreinstalledWebApps();
-    EXPECT_EQ(1, preinstalled_web_apps.disabled_count);
-    ASSERT_EQ(1u, preinstalled_web_apps.options.size());
-    const ExternalInstallOptions& options = preinstalled_web_apps.options[0];
-    ASSERT_EQ(1u, options.uninstall_and_replace.size());
-    EXPECT_EQ(kTestAppId1, options.uninstall_and_replace[0]);
-  }
-
-  {
-    // Enable the second feature; the corresponding app should be returned.
-    base::test::ScopedFeatureList feature_list;
-    feature_list.InitWithFeatures({features::kDefaultWebAppInstallation,
-                                   kMigrateDefaultChromeAppToWebAppsNonGSuite},
-                                  {kMigrateDefaultChromeAppToWebAppsGSuite});
-    PreinstalledWebApps preinstalled_web_apps = GetPreinstalledWebApps();
-    EXPECT_EQ(1, preinstalled_web_apps.disabled_count);
-    ASSERT_EQ(1u, preinstalled_web_apps.options.size());
-    const ExternalInstallOptions& options = preinstalled_web_apps.options[0];
-    ASSERT_EQ(1u, options.uninstall_and_replace.size());
-    EXPECT_EQ(kTestAppId2, options.uninstall_and_replace[0]);
-  }
-}
-
-}  // namespace web_app