Refactor ExternalWebAppManager config loading flow

This CL splits ExternalWebAppManager's config loading flow into discrete
steps and removes the various testing entry points that omit or
duplicate logic used by the real thing.

Tests can now inject data (file_utils, config_dir, configs) into the
config loading pipeline instead of having separate mini pipelines just
for them.

This change is in preparation for adding a new "FilterExtensionIds" step
to the config loading flow. Without this refactor there would be
duplication of logic between real and test code code paths that would be
prone to bitrot and missing real coverage.

Bug: 1128801
Change-Id: Ie5e87b26ab47782fab6589a4127df3ea5e876fec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423823Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810025}

chrome/browser/web_applications/external_web_app_manager.h

@@ -10,6 +10,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/values.h"
 #include "chrome/browser/web_applications/components/external_install_options.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/file_utils_wrapper.h"
@@ -22,47 +23,65 @@ class Profile;
 
 namespace web_app {
 
+namespace {
+struct LoadedConfigs;
+struct ParsedConfigs;
+}  // namespace
+
 class PendingAppManager;
 
+// Installs web apps to be preinstalled on the device (AKA default apps) during
+// start up. Will keep the apps installed on the device in sync with the set of
+// apps configured for preinstall, adding or removing as necessary. Works very
+// similar to WebAppPolicyManager.
 class ExternalWebAppManager {
  public:
+  using ConsumeLoadedConfigs = base::OnceCallback<void(LoadedConfigs)>;
+  using ConsumeParsedConfigs = base::OnceCallback<void(ParsedConfigs)>;
+  using ConsumeInstallOptions =
+      base::OnceCallback<void(std::vector<ExternalInstallOptions>)>;
+  using SynchronizeCallback = PendingAppManager::SynchronizeCallback;
+
   static const char* kHistogramEnabledCount;
   static const char* kHistogramDisabledCount;
   static const char* kHistogramConfigErrorCount;
 
+  static void SkipStartupForTesting();
+  static void SetConfigDirForTesting(const base::FilePath* config_dir);
+  static void SetConfigsForTesting(const std::vector<base::Value>* configs);
+  static void SetFileUtilsForTesting(const FileUtilsWrapper* file_utils);
+
   explicit ExternalWebAppManager(Profile* profile);
   ~ExternalWebAppManager();
 
   void SetSubsystems(PendingAppManager* pending_app_manager);
 
+  // Loads the preinstalled app configs and synchronizes them with the device's
+  // installed apps.
   void Start();
 
-  // Scans the given directory (non-recursively) for *.json files that define
-  // "external web apps", the Web App analogs of "external extensions",
-  // described at https://developer.chrome.com/apps/external_extensions
-  //
-  // This function performs file I/O, and must not be scheduled on UI threads.
-  static std::vector<ExternalInstallOptions> ReloadInstallOptionsForTesting(
-      std::unique_ptr<FileUtilsWrapper> file_utils,
-      const base::FilePath& dir,
-      Profile* profile);
-
-  using LoadCallback =
-      base::OnceCallback<void(std::vector<ExternalInstallOptions>)>;
-
-  void LoadInstallOptions(LoadCallback callback);
+  void LoadAndSynchronizeForTesting(SynchronizeCallback callback);
 
-  static void SkipStartupForTesting();
-
-  void SynchronizeAppsForTesting(
-      std::unique_ptr<FileUtilsWrapper> file_utils,
-      std::vector<std::string> app_configs,
-      PendingAppManager::SynchronizeCallback callback);
+  void LoadForTesting(ConsumeInstallOptions callback);
 
  private:
-  void SynchronizeExternalInstallOptions(
+  void LoadAndSynchronize(SynchronizeCallback callback);
+
+  void Load(ConsumeInstallOptions callback);
+  void LoadConfigs(ConsumeLoadedConfigs callback);
+  void ParseConfigs(ConsumeParsedConfigs callback,
+                    LoadedConfigs loaded_configs);
+  void PostProcessConfigs(ConsumeInstallOptions callback,
+                          ParsedConfigs parsed_configs);
+
+  void Synchronize(SynchronizeCallback callback,
+                   std::vector<ExternalInstallOptions>);
+  void OnExternalWebAppsSynchronized(
       PendingAppManager::SynchronizeCallback callback,
-      std::vector<ExternalInstallOptions>);
+      std::map<GURL, InstallResultCode> install_results,
+      std::map<GURL, bool> uninstall_results);
+
+  base::FilePath GetConfigDir();
 
   PendingAppManager* pending_app_manager_ = nullptr;
   Profile* const profile_;

chrome/browser/web_applications/external_web_app_manager_browsertest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/web_applications/external_web_app_manager.h"
 
 #include "base/files/file_path.h"
+#include "base/json/json_reader.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/test/bind_test_util.h"
@@ -41,34 +42,48 @@ class ExternalWebAppManagerBrowserTest
   // Mocks "icon.png" as available in the config's directory.
   InstallResultCode SyncDefaultAppConfig(const GURL& install_url,
                                          std::string app_config_string) {
+    base::FilePath test_config_dir(FILE_PATH_LITERAL("test_dir"));
+    ExternalWebAppManager::SetConfigDirForTesting(&test_config_dir);
+
     base::FilePath source_root_dir;
     CHECK(base::PathService::Get(base::DIR_SOURCE_ROOT, &source_root_dir));
     base::FilePath test_icon_path =
         source_root_dir.Append(GetChromeTestDataDir())
             .AppendASCII("web_apps/blue-192.png");
+    TestFileUtils file_utils(
+        {{base::FilePath(FILE_PATH_LITERAL("test_dir/icon.png")),
+          test_icon_path}});
+    ExternalWebAppManager::SetFileUtilsForTesting(&file_utils);
+
+    std::vector<base::Value> app_configs;
+    app_configs.push_back(*base::JSONReader::Read(app_config_string));
+    ExternalWebAppManager::SetConfigsForTesting(&app_configs);
 
     base::Optional<InstallResultCode> code;
     base::RunLoop sync_run_loop;
     WebAppProvider::Get(browser()->profile())
         ->external_web_app_manager_for_testing()
-        .SynchronizeAppsForTesting(
-            TestFileUtils::Create(
-                {{base::FilePath(FILE_PATH_LITERAL("test_dir/icon.png")),
-                  test_icon_path}}),
-            {app_config_string},
-            base::BindLambdaForTesting(
-                [&](std::map<GURL, InstallResultCode> install_results,
-                    std::map<GURL, bool> uninstall_results) {
-                  code = install_results.at(install_url);
-                  sync_run_loop.Quit();
-                }));
+        .LoadAndSynchronizeForTesting(base::BindLambdaForTesting(
+            [&](std::map<GURL, InstallResultCode> install_results,
+                std::map<GURL, bool> uninstall_results) {
+              code = install_results.at(install_url);
+              sync_run_loop.Quit();
+            }));
     sync_run_loop.Run();
+
+    ExternalWebAppManager::SetConfigDirForTesting(nullptr);
+    ExternalWebAppManager::SetFileUtilsForTesting(nullptr);
+    ExternalWebAppManager::SetConfigsForTesting(nullptr);
+
     return *code;
   }
 
   ~ExternalWebAppManagerBrowserTest() override = default;
 };
 
+// This JSON config functionality is only available on Chrome OS.
+#if defined(OS_CHROMEOS)
+
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest, UninstallAndReplace) {
   ASSERT_TRUE(embedded_test_server()->Start());
   Profile* profile = browser()->profile();
@@ -102,10 +117,6 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest, UninstallAndReplace) {
   EXPECT_EQ(app, uninstalled_app.get());
 }
 
-// TODO(crbug.com/1119710): Loading icon.png fails on Windows.
-// This JSON config functionality is only used on Chrome OS.
-#if !defined(OS_WIN)
-
 // Check that offline fallback installs work offline.
 IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
                        OfflineFallbackManifestSiteOffline) {
@@ -286,6 +297,6 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest,
             SK_ColorBLUE);
 }
 
-#endif  // !defined(OS_WIN)
+#endif  // defined(OS_CHROMEOS)
 
 }  // namespace web_app

chrome/browser/web_applications/external_web_app_migration_browsertest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/files/file_path.h"
+#include "base/json/json_reader.h"
 #include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/test/bind_test_util.h"
@@ -176,9 +177,9 @@ class ExternalWebAppMigrationBrowserTest : public InProcessBrowserTest {
           run_loop.Quit();
         });
 
-    std::vector<std::string> configs;
+    std::vector<base::Value> app_configs;
     if (pass_config) {
-      std::string external_web_app_config = base::ReplaceStringPlaceholders(
+      std::string app_config_string = base::ReplaceStringPlaceholders(
           R"({
             "app_url": "$1",
             "launch_container": "window",
@@ -187,15 +188,17 @@ class ExternalWebAppMigrationBrowserTest : public InProcessBrowserTest {
             "uninstall_and_replace": ["$3"]
           })",
           {GetWebAppUrl().spec(), kMigrationFlag, kExtensionId}, nullptr);
-      configs.push_back(std::move(external_web_app_config));
+      app_configs.push_back(*base::JSONReader::Read(app_config_string));
     }
+    ExternalWebAppManager::SetConfigsForTesting(&app_configs);
 
     WebAppProvider::Get(profile())
         ->external_web_app_manager_for_testing()
-        .SynchronizeAppsForTesting(std::make_unique<FileUtilsWrapper>(),
-                                   configs, std::move(callback));
+        .LoadAndSynchronizeForTesting(std::move(callback));
 
     run_loop.Run();
+
+    ExternalWebAppManager::SetConfigsForTesting(nullptr);
   }
 
   bool IsWebAppInstalled() {
@@ -444,15 +447,12 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest,
     EXPECT_FALSE(IsWebAppInstalled());
     EXPECT_TRUE(IsExtensionAppInstalled());
 
-    // TODO(crbug.com/1128801): Use the normal LoadInstallOptions() code path
-    // such that metrics are recorded.
-    // histograms.ExpectUniqueSample(
-    //     ExternalWebAppManager::kHistogramEnabledCount,
-    //     0, 1);
-    // histograms.ExpectUniqueSample(
-    //     ExternalWebAppManager::kHistogramDisabledCount, 1, 1);
-    // histograms.ExpectUniqueSample(
-    //     ExternalWebAppManager::kHistogramConfigErrorCount, 0, 1);
+    histograms.ExpectUniqueSample(ExternalWebAppManager::kHistogramEnabledCount,
+                                  0, 1);
+    histograms.ExpectUniqueSample(
+        ExternalWebAppManager::kHistogramDisabledCount, 1, 1);
+    histograms.ExpectUniqueSample(
+        ExternalWebAppManager::kHistogramConfigErrorCount, 0, 1);
   }
 
   // Migrate extension app to web app.
@@ -481,14 +481,12 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest,
       EXPECT_EQ(uninstalled_app->id(), kExtensionId);
       EXPECT_FALSE(IsExtensionAppInstalled());
 
-      // TODO(crbug.com/1128801): Use the normal LoadInstallOptions() code path
-      // such that metrics are recorded.
-      // histograms.ExpectUniqueSample(
-      //     ExternalWebAppManager::kHistogramEnabledCount, 1, 1);
-      // histograms.ExpectUniqueSample(
-      //     ExternalWebAppManager::kHistogramDisabledCount, 0, 1);
-      // histograms.ExpectUniqueSample(
-      //     ExternalWebAppManager::kHistogramConfigErrorCount, 0, 1);
+      histograms.ExpectUniqueSample(
+          ExternalWebAppManager::kHistogramEnabledCount, 1, 1);
+      histograms.ExpectUniqueSample(
+          ExternalWebAppManager::kHistogramDisabledCount, 0, 1);
+      histograms.ExpectUniqueSample(
+          ExternalWebAppManager::kHistogramConfigErrorCount, 0, 1);
     }
   }
 }

chrome/browser/web_applications/file_utils_wrapper.cc

@@ -9,7 +9,7 @@
 
 namespace web_app {
 
-std::unique_ptr<FileUtilsWrapper> FileUtilsWrapper::Clone() {
+std::unique_ptr<FileUtilsWrapper> FileUtilsWrapper::Clone() const {
   return std::make_unique<FileUtilsWrapper>();
 }
 

chrome/browser/web_applications/file_utils_wrapper.h

@@ -34,7 +34,7 @@ class FileUtilsWrapper {
   virtual ~FileUtilsWrapper() = default;
 
   // Create a copy to use in IO task.
-  virtual std::unique_ptr<FileUtilsWrapper> Clone();
+  virtual std::unique_ptr<FileUtilsWrapper> Clone() const;
 
   bool PathExists(const base::FilePath& path);
 

chrome/browser/web_applications/test/test_file_utils.cc

@@ -23,7 +23,7 @@ TestFileUtils::TestFileUtils(const TestFileUtils&) = default;
 
 TestFileUtils::~TestFileUtils() = default;
 
-std::unique_ptr<FileUtilsWrapper> TestFileUtils::Clone() {
+std::unique_ptr<FileUtilsWrapper> TestFileUtils::Clone() const {
   return std::make_unique<TestFileUtils>(*this);
 }
 

chrome/browser/web_applications/test/test_file_utils.h

@@ -28,7 +28,7 @@ class TestFileUtils : public FileUtilsWrapper {
   ~TestFileUtils() override;
 
   // FileUtilsWrapper:
-  std::unique_ptr<FileUtilsWrapper> Clone() override;
+  std::unique_ptr<FileUtilsWrapper> Clone() const override;
   int WriteFile(const base::FilePath& filename,
                 const char* data,
                 int size) override;