[dPWA] Pulling out the 'lock' functionality into a separate manager.

This change:
* pulls out the 'lock' functionality into a separate manager, owned
  by the command manager,
* makes the locks independent types, allowing for type constrained
  methods, and
* introduces the UpgradeLock method for only the SharedWebContentsLock
  to SharedWebContentsWithAppLock upgrade.

This change also fixes a lot of includes, especially for the sub app
install command.

This CL relies on existing testing to verify lock behavior (which
there is plenty in the command manager unittest), and introduces but
does not test the Upgrade functionality. This will be tested when it
used by commands.

Bug: b/2546057
Change-Id: Id58b6bd2053e462fdc74ae25fda86d4432100155
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3811413
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Phillis Tang <phillis@chromium.org>
Commit-Queue: Phillis Tang <phillis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1034285}

chrome/browser/ui/web_applications/web_app_browser_controller.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/web_applications/web_app_browser_controller.h"
 
 #include "base/callback_helpers.h"
+#include "base/containers/flat_set.h"
 #include "base/memory/raw_ptr.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
@@ -22,10 +23,12 @@
 #include "chrome/browser/ui/web_applications/web_app_launch_utils.h"
 #include "chrome/browser/ui/web_applications/web_app_ui_manager_impl.h"
 #include "chrome/browser/web_applications/commands/callback_command.h"
+#include "chrome/browser/web_applications/locks/app_lock.h"
 #include "chrome/browser/web_applications/web_app_command_manager.h"
 #include "chrome/browser/web_applications/web_app_constants.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_icon_manager.h"
+#include "chrome/browser/web_applications/web_app_id.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/browser/web_applications/web_app_sync_bridge.h"
 #include "chrome/browser/web_applications/web_app_tab_helper.h"
@@ -190,7 +193,7 @@ void WebAppBrowserController::ToggleAlwaysShowToolbarInFullscreen() {
   // base::Unretained is safe as the command manager won't execute the command
   // if the provider no longer exists.
   provider_.command_manager().ScheduleCommand(std::make_unique<CallbackCommand>(
-      WebAppCommandLock::CreateForAppLock({app_id()}),
+      std::make_unique<AppLock, base::flat_set<AppId>>({app_id()}),
       base::BindOnce(&WebAppSyncBridge::SetAlwaysShowToolbarInFullscreen,
                      base::Unretained(&provider_.sync_bridge()), app_id(),
                      !registrar().AlwaysShowToolbarInFullscreen(app_id()))));

chrome/browser/web_applications/BUILD.gn

@@ -55,6 +55,20 @@ source_set("web_applications") {
     "install_bounce_metric.h",
     "isolation_prefs_utils.cc",
     "isolation_prefs_utils.h",
+    "locks/app_lock.cc",
+    "locks/app_lock.h",
+    "locks/full_system_lock.cc",
+    "locks/full_system_lock.h",
+    "locks/lock.cc",
+    "locks/lock.h",
+    "locks/noop_lock.cc",
+    "locks/noop_lock.h",
+    "locks/shared_web_contents_lock.cc",
+    "locks/shared_web_contents_lock.h",
+    "locks/shared_web_contents_with_app_lock.cc",
+    "locks/shared_web_contents_with_app_lock.h",
+    "locks/web_app_lock_manager.cc",
+    "locks/web_app_lock_manager.h",
     "manifest_update_manager.cc",
     "manifest_update_manager.h",
     "manifest_update_task.cc",

chrome/browser/web_applications/commands/callback_command.cc

@@ -4,14 +4,20 @@
 
 #include "chrome/browser/web_applications/commands/callback_command.h"
 
+#include <memory>
+#include <utility>
+
+#include "base/callback.h"
 #include "base/strings/stringprintf.h"
-#include "chrome/browser/web_applications/web_app_command_manager.h"
+#include "chrome/browser/web_applications/locks/lock.h"
 
 namespace web_app {
 
-CallbackCommand::CallbackCommand(WebAppCommandLock command_lock,
+CallbackCommand::CallbackCommand(std::unique_ptr<Lock> lock,
                                  base::OnceClosure callback)
-    : WebAppCommand(std::move(command_lock)), callback_(std::move(callback)) {}
+    : lock_(std::move(lock)), callback_(std::move(callback)) {
+  DCHECK(lock_);
+}
 
 CallbackCommand::~CallbackCommand() = default;
 
@@ -20,6 +26,10 @@ void CallbackCommand::Start() {
                                          base::BindOnce(std::move(callback_)));
 }
 
+Lock& CallbackCommand::lock() const {
+  return *lock_;
+}
+
 base::Value CallbackCommand::ToDebugValue() const {
   return base::Value(base::StringPrintf("CallbackCommand %d", id()));
 }

chrome/browser/web_applications/commands/callback_command.h

@@ -5,26 +5,32 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_CALLBACK_COMMAND_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_CALLBACK_COMMAND_H_
 
+#include "base/callback.h"
 #include "chrome/browser/web_applications/commands/web_app_command.h"
 
 namespace web_app {
 
+class Lock;
+
 // CallbackCommand simply runs the callback being passed. This is handy for
 // small operations to web app system to avoid defining a new command class but
 // still providing isolation for the work done in the callback.
 class CallbackCommand : public WebAppCommand {
  public:
-  CallbackCommand(WebAppCommandLock command_lock, base::OnceClosure callback);
+  CallbackCommand(std::unique_ptr<Lock> lock, base::OnceClosure callback);
   ~CallbackCommand() override;
 
   void Start() override;
 
-  void OnSyncSourceRemoved() override {}
+  Lock& lock() const override;
 
-  void OnShutdown() override {}
   base::Value ToDebugValue() const override;
 
+  void OnSyncSourceRemoved() override {}
+  void OnShutdown() override {}
+
  private:
+  std::unique_ptr<Lock> lock_;
   base::OnceClosure callback_;
 };
 

chrome/browser/web_applications/commands/fetch_manifest_and_install_command.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/commands/web_app_command.h"
 #include "chrome/browser/web_applications/commands/web_app_install_command.h"
+#include "chrome/browser/web_applications/locks/noop_lock.h"
 #include "chrome/browser/web_applications/web_app_command_manager.h"
 #include "chrome/browser/web_applications/web_app_data_retriever.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
@@ -32,7 +33,7 @@ FetchManifestAndInstallCommand::FetchManifestAndInstallCommand(
     bool bypass_service_worker_check,
     WebAppInstallDialogCallback dialog_callback,
     OnceInstallCallback callback)
-    : WebAppCommand(WebAppCommandLock::CreateForNoOpLock()),
+    : lock_(std::make_unique<NoopLock>()),
       install_finalizer_(install_finalizer),
       registrar_(registrar),
       install_surface_(install_surface),
@@ -52,7 +53,7 @@ FetchManifestAndInstallCommand::FetchManifestAndInstallCommand(
     OnceInstallCallback callback,
     bool use_fallback,
     WebAppInstallFlow flow)
-    : WebAppCommand(WebAppCommandLock::CreateForNoOpLock()),
+    : lock_(std::make_unique<NoopLock>()),
       install_finalizer_(install_finalizer),
       registrar_(registrar),
       install_surface_(install_surface),
@@ -66,6 +67,10 @@ FetchManifestAndInstallCommand::FetchManifestAndInstallCommand(
 
 FetchManifestAndInstallCommand::~FetchManifestAndInstallCommand() = default;
 
+Lock& FetchManifestAndInstallCommand::lock() const {
+  return *lock_;
+}
+
 void FetchManifestAndInstallCommand::Start() {
   if (IsWebContentsDestroyed()) {
     Abort(webapps::InstallResultCode::kWebContentsDestroyed);

chrome/browser/web_applications/commands/fetch_manifest_and_install_command.h

@@ -23,6 +23,7 @@ class WebContents;
 
 namespace web_app {
 
+class NoopLock;
 class WebAppDataRetriever;
 class WebAppInstallFinalizer;
 class WebAppRegistrar;
@@ -59,6 +60,8 @@ class FetchManifestAndInstallCommand : public WebAppCommand {
 
   ~FetchManifestAndInstallCommand() override;
 
+  Lock& lock() const override;
+
   void Start() override;
   void OnSyncSourceRemoved() override;
   void OnShutdown() override;
@@ -81,6 +84,8 @@ class FetchManifestAndInstallCommand : public WebAppCommand {
                                     bool is_installable);
   void LogInstallInfo();
 
+  std::unique_ptr<NoopLock> lock_;
+
   raw_ptr<WebAppInstallFinalizer> install_finalizer_;
   raw_ptr<WebAppRegistrar> registrar_;
   webapps::WebappInstallSource install_surface_;

chrome/browser/web_applications/commands/install_from_info_command.cc

@@ -4,11 +4,14 @@
 
 #include "chrome/browser/web_applications/commands/install_from_info_command.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/containers/flat_set.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/stringprintf.h"
+#include "chrome/browser/web_applications/locks/app_lock.h"
 #include "chrome/browser/web_applications/os_integration/os_integration_manager.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_id.h"
@@ -25,7 +28,7 @@ InstallFromInfoCommand::InstallFromInfoCommand(
     bool overwrite_existing_manifest_fields,
     webapps::WebappInstallSource install_surface,
     OnceInstallCallback install_callback)
-    : WebAppCommand(WebAppCommandLock::CreateForAppLock(
+    : lock_(std::make_unique<AppLock, base::flat_set<AppId>>(
           {GenerateAppId(install_info->manifest_id, install_info->start_url)})),
       app_id_(
           GenerateAppId(install_info->manifest_id, install_info->start_url)),
@@ -42,7 +45,7 @@ InstallFromInfoCommand::InstallFromInfoCommand(
     webapps::WebappInstallSource install_surface,
     OnceInstallCallback install_callback,
     const WebAppInstallParams& install_params)
-    : WebAppCommand(WebAppCommandLock::CreateForAppLock(
+    : lock_(std::make_unique<AppLock, base::flat_set<AppId>>(
           {GenerateAppId(install_info->manifest_id, install_info->start_url)})),
       app_id_(
           GenerateAppId(install_info->manifest_id, install_info->start_url)),
@@ -61,6 +64,10 @@ InstallFromInfoCommand::InstallFromInfoCommand(
 }
 InstallFromInfoCommand::~InstallFromInfoCommand() = default;
 
+Lock& InstallFromInfoCommand::lock() const {
+  return *lock_;
+}
+
 void InstallFromInfoCommand::Start() {
   PopulateProductIcons(install_info_.get(),
                        /*icons_map=*/nullptr);

chrome/browser/web_applications/commands/install_from_info_command.h

@@ -20,6 +20,7 @@
 
 namespace web_app {
 
+class AppLock;
 class WebAppInstallFinalizer;
 
 // Starts a web app installation process using prefilled
@@ -52,6 +53,8 @@ class InstallFromInfoCommand : public WebAppCommand {
 
   ~InstallFromInfoCommand() override;
 
+  Lock& lock() const override;
+
   void Start() override;
   void OnSyncSourceRemoved() override;
   void OnShutdown() override;
@@ -65,6 +68,7 @@ class InstallFromInfoCommand : public WebAppCommand {
                           webapps::InstallResultCode code,
                           OsHooksErrors os_hooks_errors);
 
+  std::unique_ptr<AppLock> lock_;
   AppId app_id_;
   std::unique_ptr<WebAppInstallInfo> install_info_;
   raw_ptr<WebAppInstallFinalizer> install_finalizer_;

chrome/browser/web_applications/commands/install_from_sync_command.cc

@@ -7,22 +7,23 @@
 #include <memory>
 #include <utility>
 
-#include "base/callback_helpers.h"
+#include "base/containers/flat_set.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/commands/web_app_command.h"
 #include "chrome/browser/web_applications/install_bounce_metric.h"
+#include "chrome/browser/web_applications/locks/shared_web_contents_with_app_lock.h"
 #include "chrome/browser/web_applications/web_app_command_manager.h"
+#include "chrome/browser/web_applications/web_app_data_retriever.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_install_utils.h"
 #include "chrome/browser/web_applications/web_app_utils.h"
 #include "components/webapps/browser/install_result_code.h"
 #include "components/webapps/browser/installable/installable_metrics.h"
 #include "content/public/browser/web_contents.h"
-#include "net/http/http_status_code.h"
 
 namespace web_app {
 
@@ -78,8 +79,9 @@ InstallFromSyncCommand::InstallFromSyncCommand(
     std::unique_ptr<WebAppDataRetriever> data_retriever,
     const Params& params,
     OnceInstallCallback install_callback)
-    : WebAppCommand(
-          WebAppCommandLock::CreateForAppAndWebContentsLock({params.app_id})),
+    : lock_(
+          std::make_unique<SharedWebContentsWithAppLock, base::flat_set<AppId>>(
+              {params.app_id})),
       url_loader_(url_loader),
       profile_(profile),
       finalizer_(finalizer),
@@ -127,6 +129,10 @@ void InstallFromSyncCommand::OnSyncSourceRemoved() {
                          webapps::InstallResultCode::kHaltedBySyncUninstall);
 }
 
+Lock& InstallFromSyncCommand::lock() const {
+  return *lock_;
+}
+
 void InstallFromSyncCommand::Start() {
   url_loader_->LoadUrl(
       params_.start_url, shared_web_contents(),

chrome/browser/web_applications/commands/install_from_sync_command.h

@@ -13,22 +13,25 @@
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/commands/web_app_command.h"
+#include "chrome/browser/web_applications/os_integration/os_integration_manager.h"
 #include "chrome/browser/web_applications/user_display_mode.h"
 #include "chrome/browser/web_applications/web_app_constants.h"
-#include "chrome/browser/web_applications/web_app_data_retriever.h"
-#include "chrome/browser/web_applications/web_app_install_finalizer.h"
-#include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_install_params.h"
 #include "chrome/browser/web_applications/web_app_logging.h"
-#include "chrome/browser/web_applications/web_app_registrar.h"
-#include "chrome/browser/web_applications/web_app_url_loader.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "url/gurl.h"
 
 class Profile;
+struct WebAppInstallInfo;
 
 namespace web_app {
 
+enum class WebAppUrlLoaderResult;
+class SharedWebContentsWithAppLock;
+class WebAppInstallFinalizer;
+class WebAppDataRetriever;
+class WebAppRegistrar;
+
 class InstallFromSyncCommand : public WebAppCommand {
  public:
   struct Params {
@@ -65,6 +68,8 @@ class InstallFromSyncCommand : public WebAppCommand {
       OnceInstallCallback install_callback);
   ~InstallFromSyncCommand() override;
 
+  Lock& lock() const override;
+
   base::Value ToDebugValue() const override;
 
   void OnSyncSourceRemoved() override;
@@ -77,7 +82,7 @@ class InstallFromSyncCommand : public WebAppCommand {
       base::OnceCallback<void(webapps::InstallResultCode code)> callback);
 
  private:
-  void OnWebAppUrlLoadedGetWebAppInstallInfo(WebAppUrlLoader::Result result);
+  void OnWebAppUrlLoadedGetWebAppInstallInfo(WebAppUrlLoaderResult result);
 
   void OnGetWebAppInstallInfo(std::unique_ptr<WebAppInstallInfo> web_app_info);
 
@@ -104,6 +109,7 @@ class InstallFromSyncCommand : public WebAppCommand {
   void ReportResultAndDestroy(const AppId& app_id,
                               webapps::InstallResultCode code);
 
+  std::unique_ptr<SharedWebContentsWithAppLock> lock_;
   const base::raw_ptr<WebAppUrlLoader> url_loader_;
   const base::raw_ptr<Profile> profile_;
   const base::raw_ptr<WebAppInstallFinalizer> finalizer_;

chrome/browser/web_applications/commands/install_from_sync_command_browsertest.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/web_applications/test/web_app_icon_test_utils.h"
 #include "chrome/browser/web_applications/user_display_mode.h"
 #include "chrome/browser/web_applications/web_app_command_manager.h"
+#include "chrome/browser/web_applications/web_app_data_retriever.h"
 #include "chrome/browser/web_applications/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_install_info.h"
 #include "chrome/browser/web_applications/web_app_provider.h"