[PWA] Remove ExternallyManagedAppInstallTask dependencies [2/N]

This CL is the 2nd step in the chain to remove any dependency on
ExternallyManagedAppInstallTask. This is done on production by updating
the struct that stored an instance of the task to only store an instance
of the ExternalInstallOption and the corresponding callback. When an
installation needs to happen, the WebAppCommandScheduler is used to
invoke an ExternalAppResolutionCommand directly with the callback, thus
completely bypassing the requirement of an intermediary task like state.

This CL also updates existing documentation that mentions the
ExternallyManagedAppInstallTask to instead refer to the command that is
called.

A future CL will remove all traces of the task code altogether.

Change-Id: Iaf88ae4c9c709a0a1ee72c63163f0f4bdd3dd313
Bug: 328431086
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6357998
Commit-Queue: Dibyajyoti Pal <dibyapal@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1437102}

chrome/browser/web_applications/externally_managed_app_manager.cc

@@ -22,7 +22,6 @@
 #include "base/values.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/external_install_options.h"
-#include "chrome/browser/web_applications/externally_managed_app_install_task.h"
 #include "chrome/browser/web_applications/externally_managed_app_registration_task.h"
 #include "chrome/browser/web_applications/locks/all_apps_lock.h"
 #include "chrome/browser/web_applications/proto/web_app_install_state.pb.h"
@@ -91,13 +90,13 @@ ExternallyManagedAppManager::SynchronizeRequest::operator=(
 ExternallyManagedAppManager::SynchronizeRequest::SynchronizeRequest(
     SynchronizeRequest&& other) = default;
 
-struct ExternallyManagedAppManager::TaskAndCallback {
-  TaskAndCallback(std::unique_ptr<ExternallyManagedAppInstallTask> task,
-                  OnceInstallCallback callback)
-      : task(std::move(task)), callback(std::move(callback)) {}
-  ~TaskAndCallback() = default;
+struct ExternallyManagedAppManager::ExternalInstallMetadata {
+  ExternalInstallMetadata(ExternalInstallOptions options,
+                          OnceInstallCallback callback)
+      : options(std::move(options)), callback(std::move(callback)) {}
+  ~ExternalInstallMetadata() = default;
 
-  std::unique_ptr<ExternallyManagedAppInstallTask> task;
+  ExternalInstallOptions options;
   OnceInstallCallback callback;
 };
 
@@ -133,8 +132,9 @@ void ExternallyManagedAppManager::SetProvider(base::PassKey<WebAppProvider>,
 void ExternallyManagedAppManager::InstallNow(
     ExternalInstallOptions install_options,
     OnceInstallCallback callback) {
-  pending_installs_.push_front(std::make_unique<TaskAndCallback>(
-      CreateInstallationTask(std::move(install_options)), std::move(callback)));
+  pending_installs_metadata_.push_front(
+      std::make_unique<ExternalInstallMetadata>(std::move(install_options),
+                                                std::move(callback)));
 
   PostMaybeStartNext();
 }
@@ -142,8 +142,9 @@ void ExternallyManagedAppManager::InstallNow(
 void ExternallyManagedAppManager::Install(
     ExternalInstallOptions install_options,
     OnceInstallCallback callback) {
-  pending_installs_.push_back(std::make_unique<TaskAndCallback>(
-      CreateInstallationTask(std::move(install_options)), std::move(callback)));
+  pending_installs_metadata_.push_back(
+      std::make_unique<ExternalInstallMetadata>(std::move(install_options),
+                                                std::move(callback)));
 
   PostMaybeStartNext();
 }
@@ -152,8 +153,9 @@ void ExternallyManagedAppManager::InstallApps(
     std::vector<ExternalInstallOptions> install_options_list,
     const RepeatingInstallCallback& callback) {
   for (auto& install_options : install_options_list) {
-    pending_installs_.push_back(std::make_unique<TaskAndCallback>(
-        CreateInstallationTask(std::move(install_options)), callback));
+    pending_installs_metadata_.push_back(
+        std::make_unique<ExternalInstallMetadata>(std::move(install_options),
+                                                  callback));
   }
 
   PostMaybeStartNext();
@@ -205,11 +207,11 @@ void ExternallyManagedAppManager::Shutdown() {
   is_in_shutdown_ = true;
   pending_registrations_.clear();
   current_registration_.reset();
-  pending_installs_.clear();
+  pending_installs_metadata_.clear();
   url_loader_.reset();
   // `current_install_` keeps a pointer to `web_contents_` so destroy it before
   // releasing the WebContents.
-  current_install_.reset();
+  current_install_metadata_.reset();
   ReleaseWebContents();
 }
 
@@ -228,21 +230,12 @@ void ExternallyManagedAppManager::SetDataRetrieverFactoryForTesting(
 void ExternallyManagedAppManager::ReleaseWebContents() {
   DCHECK(pending_registrations_.empty());
   DCHECK(!current_registration_);
-  DCHECK(pending_installs_.empty());
-  DCHECK(!current_install_);
+  DCHECK(pending_installs_metadata_.empty());
+  DCHECK(!current_install_metadata_);
 
   web_contents_.reset();
 }
 
-std::unique_ptr<ExternallyManagedAppInstallTask>
-ExternallyManagedAppManager::CreateInstallationTask(
-    ExternalInstallOptions install_options) {
-  std::unique_ptr<ExternallyManagedAppInstallTask> install_task =
-      std::make_unique<ExternallyManagedAppInstallTask>(
-          *provider_, std::move(install_options));
-  return install_task;
-}
-
 std::unique_ptr<ExternallyManagedAppRegistrationTaskBase>
 ExternallyManagedAppManager::CreateRegistration(
     GURL install_url,
@@ -281,7 +274,7 @@ void ExternallyManagedAppManager::PostMaybeStartNext() {
 }
 
 void ExternallyManagedAppManager::MaybeStartNext() {
-  if (current_install_ || IsShuttingDown()) {
+  if (current_install_metadata_ || IsShuttingDown()) {
     return;
   }
   provider_->scheduler().ScheduleCallback(
@@ -294,17 +287,16 @@ void ExternallyManagedAppManager::MaybeStartNext() {
 void ExternallyManagedAppManager::MaybeStartNextOnLockAcquired(
     AllAppsLock& lock,
     base::Value::Dict& debug_value) {
-  if (current_install_ || IsShuttingDown()) {
+  if (current_install_metadata_ || IsShuttingDown()) {
     return;
   }
 
-  while (!pending_installs_.empty()) {
-    std::unique_ptr<TaskAndCallback> front =
-        std::move(pending_installs_.front());
-    pending_installs_.pop_front();
+  while (!pending_installs_metadata_.empty()) {
+    std::unique_ptr<ExternalInstallMetadata> front =
+        std::move(pending_installs_metadata_.front());
+    pending_installs_metadata_.pop_front();
 
-    const ExternalInstallOptions& install_options =
-        front->task->install_options();
+    const ExternalInstallOptions& install_options = front->options;
 
     CHECK(install_options.install_url.is_valid(), base::NotFatalUntil::M130);
     std::optional<webapps::AppId> app_id =
@@ -398,7 +390,7 @@ void ExternallyManagedAppManager::MaybeStartNextOnLockAcquired(
                           /*installed_placeholder_app_id=*/std::nullopt);
     return;
   }
-  DCHECK(!current_install_);
+  DCHECK(!current_install_metadata_);
 
   if (current_registration_ || RunNextRegistration()) {
     return;
@@ -408,12 +400,12 @@ void ExternallyManagedAppManager::MaybeStartNextOnLockAcquired(
 }
 
 void ExternallyManagedAppManager::StartInstallationTask(
-    std::unique_ptr<TaskAndCallback> task,
+    std::unique_ptr<ExternalInstallMetadata> external_install_metadata,
     std::optional<webapps::AppId> installed_placeholder_app_id) {
   if (IsShuttingDown()) {
     return;
   }
-  DCHECK(!current_install_);
+  DCHECK(!current_install_metadata_);
   DCHECK(!is_in_shutdown_);
   if (current_registration_) {
     // Preempt current registration.
@@ -423,9 +415,10 @@ void ExternallyManagedAppManager::StartInstallationTask(
     current_registration_.reset();
   }
 
-  current_install_ = std::move(task);
+  current_install_metadata_ = std::move(external_install_metadata);
   CreateWebContentsIfNecessary();
-  current_install_->task->Install(
+  provider_->scheduler().InstallExternallyManagedApp(
+      current_install_metadata_->options,
       std::move(installed_placeholder_app_id),
       base::BindOnce(&ExternallyManagedAppManager::OnInstalled,
                      weak_ptr_factory_.GetWeakPtr()));
@@ -462,8 +455,7 @@ void ExternallyManagedAppManager::CreateWebContentsIfNecessary() {
 void ExternallyManagedAppManager::OnInstalled(
     ExternallyManagedAppManagerInstallResult result) {
   if (result.app_id && IsSuccess(result.code)) {
-    MaybeEnqueueServiceWorkerRegistration(
-        current_install_->task->install_options());
+    MaybeEnqueueServiceWorkerRegistration(current_install_metadata_->options);
   }
 
   // Post a task to avoid webapps::InstallableManager crashing and do so before
@@ -471,10 +463,10 @@ void ExternallyManagedAppManager::OnInstalled(
   // app.
   PostMaybeStartNext();
 
-  std::unique_ptr<TaskAndCallback> task_and_callback;
-  task_and_callback.swap(current_install_);
-  std::move(task_and_callback->callback)
-      .Run(task_and_callback->task->install_options().install_url, result);
+  std::unique_ptr<ExternalInstallMetadata> metadata_and_callback;
+  metadata_and_callback.swap(current_install_metadata_);
+  std::move(metadata_and_callback->callback)
+      .Run(metadata_and_callback->options.install_url, result);
 }
 
 void ExternallyManagedAppManager::MaybeEnqueueServiceWorkerRegistration(

chrome/browser/web_applications/externally_managed_app_manager.h

@@ -38,7 +38,6 @@ class WebContents;
 namespace web_app {
 
 class AllAppsLock;
-class ExternallyManagedAppInstallTask;
 class ExternallyManagedAppRegistrationTaskBase;
 class WebAppDataRetriever;
 class WebAppProvider;
@@ -185,9 +184,6 @@ class ExternallyManagedAppManager {
  protected:
   virtual void ReleaseWebContents();
 
-  virtual std::unique_ptr<ExternallyManagedAppInstallTask>
-  CreateInstallationTask(ExternalInstallOptions install_options);
-
   virtual std::unique_ptr<ExternallyManagedAppRegistrationTaskBase>
   CreateRegistration(GURL install_url,
                      const base::TimeDelta registration_timeout);
@@ -200,7 +196,7 @@ class ExternallyManagedAppManager {
   raw_ptr<WebAppProvider> provider_ = nullptr;
 
  private:
-  struct TaskAndCallback;
+  struct ExternalInstallMetadata;
 
   struct SynchronizeRequest {
     SynchronizeRequest(SynchronizeCallback callback,
@@ -245,7 +241,7 @@ class ExternallyManagedAppManager {
                                     base::Value::Dict& debug_value);
 
   void StartInstallationTask(
-      std::unique_ptr<TaskAndCallback> task,
+      std::unique_ptr<ExternalInstallMetadata> external_install_metadata,
       std::optional<webapps::AppId> installed_placeholder_app_id);
 
   bool RunNextRegistration();
@@ -276,9 +272,10 @@ class ExternallyManagedAppManager {
 
   std::unique_ptr<content::WebContents> web_contents_;
 
-  std::unique_ptr<TaskAndCallback> current_install_;
+  std::unique_ptr<ExternalInstallMetadata> current_install_metadata_;
 
-  base::circular_deque<std::unique_ptr<TaskAndCallback>> pending_installs_;
+  base::circular_deque<std::unique_ptr<ExternalInstallMetadata>>
+      pending_installs_metadata_;
 
   std::unique_ptr<ExternallyManagedAppRegistrationTaskBase>
       current_registration_;

docs/webapps/installation_pipeline.md

@@ -74,7 +74,8 @@ The general installation flow of an externally managed app is:
 
 1. A call to [`ExternallyManagedAppProvider::SynchronizeInstalledApps`][16]
 1. Finding all apps that need to be uninstalled and uninstalling them, find all apps that need to be installed and:
-1. Queue an `ExternallyManagedAppInstallTask` to install each app sequentially.
+1. Enqueue an `ExternalAppResolutionCommand` for each app to start resolving what the final behavior
+should be.
 1. Each task loads the url for the app.
 1. If the url is successfully loaded, then use [`ExternallyManagedInstallCommand`][14], and continue installation on the normal pipeline (described above, and [flowchart][17] above).
 1. If the url fails to fully load (usually a redirect if the user needs to sign in or corp credentials are not installed), and the external app manager specified a [placeholder app was required][18] then: