WebApp: Support concurrent installations in BookmarkAppInstallFinalizer.

BookmarkAppHelper supports concurrent installs:
many instances of BookmarkAppHelper can be created at call sites.

Example: You can open 3 browser windows, initiate an install in each of them.
And you see 3 modal dialogs to accept/dismiss.

Bug: 915043
Change-Id: Ib7c1422bf341a43f4b45a82aa3204eb29c572369
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1498795Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638005}

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.cc

@@ -4,9 +4,11 @@
 
 #include "chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/bind.h"
+#include "base/optional.h"
 #include "chrome/browser/extensions/bookmark_app_extension_util.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/launch_util.h"
@@ -17,6 +19,7 @@
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/web_application_info.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/install/crx_install_error.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/extension_set.h"
@@ -38,6 +41,34 @@ const Extension* GetExtensionById(Profile* profile,
   return app;
 }
 
+void OnExtensionInstalled(
+    const GURL& app_url,
+    LaunchType launch_type,
+    web_app::InstallFinalizer::InstallFinalizedCallback callback,
+    scoped_refptr<CrxInstaller> crx_installer,
+    const base::Optional<CrxInstallError>& error) {
+  if (error) {
+    std::move(callback).Run(web_app::AppId(),
+                            web_app::InstallResultCode::kFailedUnknownReason);
+    return;
+  }
+
+  const Extension* extension = crx_installer->extension();
+  DCHECK(extension);
+
+  DCHECK_EQ(AppLaunchInfo::GetLaunchWebURL(extension), app_url);
+
+  // Set the launcher type for the app.
+  SetLaunchType(crx_installer->profile(), extension->id(), launch_type);
+
+  // Set this app to be locally installed, as it was installed from this
+  // machine.
+  SetBookmarkAppIsLocallyInstalled(crx_installer->profile(), extension, true);
+
+  std::move(callback).Run(extension->id(),
+                          web_app::InstallResultCode::kSuccess);
+}
+
 }  // namespace
 
 BookmarkAppInstallFinalizer::BookmarkAppInstallFinalizer(Profile* profile)
@@ -55,45 +86,14 @@ BookmarkAppInstallFinalizer::~BookmarkAppInstallFinalizer() = default;
 void BookmarkAppInstallFinalizer::FinalizeInstall(
     const WebApplicationInfo& web_app_info,
     InstallFinalizedCallback callback) {
-  DCHECK(!crx_installer_);
+  scoped_refptr<CrxInstaller> crx_installer =
+      crx_installer_factory_.Run(profile_);
 
-  crx_installer_ = crx_installer_factory_.Run(profile_);
-  DCHECK(crx_installer_);
+  crx_installer->set_installer_callback(base::BindOnce(
+      OnExtensionInstalled, web_app_info.app_url, GetLaunchType(web_app_info),
+      std::move(callback), crx_installer));
 
-  crx_installer_->set_installer_callback(base::BindOnce(
-      &BookmarkAppInstallFinalizer::OnExtensionInstalled,
-      weak_ptr_factory_.GetWeakPtr(),
-      std::make_unique<WebApplicationInfo>(web_app_info), std::move(callback)));
-  crx_installer_->InstallWebApp(web_app_info);
-}
-
-void BookmarkAppInstallFinalizer::OnExtensionInstalled(
-    std::unique_ptr<WebApplicationInfo> web_app_info,
-    InstallFinalizedCallback callback,
-    const base::Optional<CrxInstallError>& error) {
-  const Extension* extension = crx_installer_->extension();
-  crx_installer_.reset();
-
-  if (error) {
-    std::move(callback).Run(web_app::AppId(),
-                            web_app::InstallResultCode::kFailedUnknownReason);
-    return;
-  }
-
-  DCHECK(extension);
-  DCHECK_EQ(AppLaunchInfo::GetLaunchWebURL(extension), web_app_info->app_url);
-
-  const LaunchType launch_type = GetLaunchType(*web_app_info);
-
-  // Set the launcher type for the app.
-  SetLaunchType(profile_, extension->id(), launch_type);
-
-  // Set this app to be locally installed, as it was installed from this
-  // machine.
-  SetBookmarkAppIsLocallyInstalled(profile_, extension, true);
-
-  std::move(callback).Run(extension->id(),
-                          web_app::InstallResultCode::kSuccess);
+  crx_installer->InstallWebApp(web_app_info);
 }
 
 bool BookmarkAppInstallFinalizer::CanCreateOsShortcuts() const {

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer.h

@@ -8,24 +8,16 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
-#include "base/optional.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
 
 class Profile;
 
-struct WebApplicationInfo;
-
-namespace content {
-class WebContents;
-}
-
 namespace extensions {
 
 class CrxInstaller;
-class CrxInstallError;
 
 // Class used to actually install the Bookmark App in the system.
+// TODO(loyso): Erase this subclass once crbug.com/877898 fixed.
 class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
  public:
   // Constructs a BookmarkAppInstallFinalizer that will install the Bookmark App
@@ -53,18 +45,9 @@ class BookmarkAppInstallFinalizer : public web_app::InstallFinalizer {
       CrxInstallerFactory crx_installer_factory);
 
  private:
-  void OnExtensionInstalled(std::unique_ptr<WebApplicationInfo> web_app_info,
-                            InstallFinalizedCallback callback,
-                            const base::Optional<CrxInstallError>& error);
-
-  scoped_refptr<CrxInstaller> crx_installer_;
   CrxInstallerFactory crx_installer_factory_;
   Profile* profile_;
 
-  // We need a WeakPtr because CrxInstaller is refcounted and it can run its
-  // callback after this class has been destroyed.
-  base::WeakPtrFactory<BookmarkAppInstallFinalizer> weak_ptr_factory_{this};
-
   DISALLOW_COPY_AND_ASSIGN(BookmarkAppInstallFinalizer);
 };
 

chrome/browser/web_applications/extensions/bookmark_app_install_finalizer_unittest.cc

@@ -151,4 +151,55 @@ TEST_F(BookmarkAppInstallFinalizerTest, BasicInstallFails) {
   EXPECT_TRUE(callback_called);
 }
 
+TEST_F(BookmarkAppInstallFinalizerTest, ConcurrentInstallSucceeds) {
+  BookmarkAppInstallFinalizer finalizer(profile());
+
+  base::RunLoop run_loop;
+
+  const GURL url1("https://foo1.example");
+  const GURL url2("https://foo2.example");
+
+  bool callback1_called = false;
+  bool callback2_called = false;
+
+  // Start install finalization for the 1st app
+  {
+    WebApplicationInfo web_application_info;
+    web_application_info.app_url = url1;
+
+    finalizer.FinalizeInstall(
+        web_application_info,
+        base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
+                                       web_app::InstallResultCode code) {
+          EXPECT_EQ(web_app::InstallResultCode::kSuccess, code);
+          EXPECT_EQ(installed_app_id, web_app::GenerateAppIdFromURL(url1));
+          callback1_called = true;
+          if (callback2_called)
+            run_loop.Quit();
+        }));
+  }
+
+  // Start install finalization for the 2nd app
+  {
+    WebApplicationInfo web_application_info;
+    web_application_info.app_url = url2;
+
+    finalizer.FinalizeInstall(
+        web_application_info,
+        base::BindLambdaForTesting([&](const web_app::AppId& installed_app_id,
+                                       web_app::InstallResultCode code) {
+          EXPECT_EQ(web_app::InstallResultCode::kSuccess, code);
+          EXPECT_EQ(installed_app_id, web_app::GenerateAppIdFromURL(url2));
+          callback2_called = true;
+          if (callback1_called)
+            run_loop.Quit();
+        }));
+  }
+
+  run_loop.Run();
+
+  EXPECT_TRUE(callback1_called);
+  EXPECT_TRUE(callback2_called);
+}
+
 }  // namespace extensions