Reland "Refactor ExternalWebAppManager config loading flow"

This is a reland of 02f9f701

The original change uncovered an incompatibility with the test profile
generated by AccountManagerUIHandlerTest and
chromeos::ProfileHelper::IsPrimaryProfile(). This change uncovered it
by changing the timing of when it gets called resulting in flaky
failures.

The AccountManagerUIHandlerTest tests were disabled in the meantime
due to flakiness. A follow up CL re-enables the tests and disables the
ExternalWebAppManager start up flow such that it doesn't trigger the
incompatibility with the custom test profile:
https://chromium-review.googlesource.com/c/chromium/src/+/2434412

Original change's description:
> 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/+/2423823
> Reviewed-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}

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

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;