[Extensions Callback Cleanup] Webstore Installers

Do the following clean-ups in the webstore installer classes and its
callers:
- Convert base::Callback to base::RepeatingCallback/base::OnceCallback
- Convert base::Bind to base::BindRepeating/base::BindOnce
- Pass callbacks by value when an instance is retained

Bug: 714018

Change-Id: Ia55a04264ac528625c3f8664fd30607036945c24
Reviewed-on: https://chromium-review.googlesource.com/c/1391867Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619844}

chrome/browser/apps/platform_apps/api/webstore_widget_private/app_installer.cc

@@ -36,10 +36,11 @@ AppInstaller::AppInstaller(content::WebContents* web_contents,
                            const std::string& item_id,
                            Profile* profile,
                            bool silent_installation,
-                           const Callback& callback)
-    : extensions::WebstoreStandaloneInstaller(item_id, profile, callback),
+                           Callback callback)
+    : extensions::WebstoreStandaloneInstaller(item_id,
+                                              profile,
+                                              std::move(callback)),
       silent_installation_(silent_installation),
-      callback_(callback),
       web_contents_(web_contents),
       web_contents_observer_(new WebContentsObserver(web_contents, this)) {
   DCHECK(web_contents_);
@@ -79,8 +80,8 @@ content::WebContents* AppInstaller::GetWebContents() const {
 }
 
 void AppInstaller::OnWebContentsDestroyed(content::WebContents* web_contents) {
-  callback_.Run(false, kWebContentsDestroyedError,
-                extensions::webstore_install::OTHER_ERROR);
+  RunCallback(false, kWebContentsDestroyedError,
+              extensions::webstore_install::OTHER_ERROR);
   AbortInstall();
 }
 

chrome/browser/apps/platform_apps/api/webstore_widget_private/app_installer.h

@@ -26,7 +26,7 @@ class AppInstaller : public extensions::WebstoreStandaloneInstaller {
                const std::string& item_id,
                Profile* profile,
                bool silent_installation,
-               const Callback& callback);
+               Callback callback);
 
  protected:
   friend class base::RefCountedThreadSafe<AppInstaller>;
@@ -47,7 +47,6 @@ class AppInstaller : public extensions::WebstoreStandaloneInstaller {
   class WebContentsObserver;
 
   bool silent_installation_;
-  Callback callback_;
   content::WebContents* web_contents_;
   std::unique_ptr<WebContentsObserver> web_contents_observer_;
 

chrome/browser/apps/platform_apps/api/webstore_widget_private/webstore_widget_private_api.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
 #include "chrome/browser/apps/platform_apps/api/webstore_widget_private/app_installer.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/profiles/profile.h"
@@ -44,20 +45,19 @@ WebstoreWidgetPrivateInstallWebstoreItemFunction::Run() {
   if (params->silent_installation && !allow_silent_install)
     return RespondNow(Error("Silent installation not allowed."));
 
-  const extensions::WebstoreStandaloneInstaller::Callback callback = base::Bind(
-      &WebstoreWidgetPrivateInstallWebstoreItemFunction::OnInstallComplete,
-      this);
-
   content::WebContents* web_contents = GetSenderWebContents();
   if (!web_contents) {
     return RespondNow(
         Error(extensions::function_constants::kCouldNotFindSenderWebContents));
   }
-  scoped_refptr<webstore_widget::AppInstaller> installer(
-      new webstore_widget::AppInstaller(
-          web_contents, params->item_id,
-          Profile::FromBrowserContext(browser_context()),
-          params->silent_installation, callback));
+
+  auto installer = base::MakeRefCounted<webstore_widget::AppInstaller>(
+      web_contents, params->item_id,
+      Profile::FromBrowserContext(browser_context()),
+      params->silent_installation,
+      base::BindOnce(
+          &WebstoreWidgetPrivateInstallWebstoreItemFunction::OnInstallComplete,
+          this));
   // installer will be AddRef()'d in BeginInstall().
   installer->BeginInstall();
 

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -1889,10 +1889,10 @@ DeveloperPrivateRepairExtensionFunction::Run() {
     return RespondNow(Error(kCouldNotFindWebContentsError));
 
   scoped_refptr<WebstoreReinstaller> reinstaller(new WebstoreReinstaller(
-      web_contents,
-      params->extension_id,
-      base::Bind(&DeveloperPrivateRepairExtensionFunction::OnReinstallComplete,
-                 this)));
+      web_contents, params->extension_id,
+      base::BindOnce(
+          &DeveloperPrivateRepairExtensionFunction::OnReinstallComplete,
+          this)));
   reinstaller->BeginReinstall();
 
   return RespondLater();

chrome/browser/extensions/webstore_install_with_prompt.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/webstore_install_with_prompt.h"
 
+#include <utility>
+
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/web_contents.h"
@@ -15,8 +17,10 @@ namespace extensions {
 WebstoreInstallWithPrompt::WebstoreInstallWithPrompt(
     const std::string& webstore_item_id,
     Profile* profile,
-    const Callback& callback)
-    : WebstoreStandaloneInstaller(webstore_item_id, profile, callback),
+    Callback callback)
+    : WebstoreStandaloneInstaller(webstore_item_id,
+                                  profile,
+                                  std::move(callback)),
       show_post_install_ui_(true),
       dummy_web_contents_(
           WebContents::Create(WebContents::CreateParams(profile))),
@@ -28,8 +32,10 @@ WebstoreInstallWithPrompt::WebstoreInstallWithPrompt(
     const std::string& webstore_item_id,
     Profile* profile,
     gfx::NativeWindow parent_window,
-    const Callback& callback)
-    : WebstoreStandaloneInstaller(webstore_item_id, profile, callback),
+    Callback callback)
+    : WebstoreStandaloneInstaller(webstore_item_id,
+                                  profile,
+                                  std::move(callback)),
       show_post_install_ui_(true),
       dummy_web_contents_(
           WebContents::Create(WebContents::CreateParams(profile))),

chrome/browser/extensions/webstore_install_with_prompt.h

@@ -35,14 +35,14 @@ class WebstoreInstallWithPrompt : public WebstoreStandaloneInstaller {
   // will be centered on the screen.
   WebstoreInstallWithPrompt(const std::string& webstore_item_id,
                             Profile* profile,
-                            const Callback& callback);
+                            Callback callback);
 
   // If this constructor is used, the parent of the install dialog will be
   // |parent_window|.
   WebstoreInstallWithPrompt(const std::string& webstore_item_id,
                             Profile* profile,
                             gfx::NativeWindow parent_window,
-                            const Callback& callback);
+                            Callback callback);
 
  protected:
   friend class base::RefCountedThreadSafe<WebstoreInstallWithPrompt>;

chrome/browser/extensions/webstore_reinstaller.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/webstore_reinstaller.h"
 
+#include <utility>
+
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/extension_install_prompt.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -21,11 +23,11 @@ const char kTabClosed[] = "Tab was closed.";
 WebstoreReinstaller::WebstoreReinstaller(
     content::WebContents* web_contents,
     const std::string& extension_id,
-    const WebstoreStandaloneInstaller::Callback& callback)
+    WebstoreStandaloneInstaller::Callback callback)
     : WebstoreStandaloneInstaller(
           extension_id,
           Profile::FromBrowserContext(web_contents->GetBrowserContext()),
-          callback),
+          std::move(callback)),
       content::WebContentsObserver(web_contents) {
   DCHECK(
       ExtensionPrefs::Get(web_contents->GetBrowserContext())

chrome/browser/extensions/webstore_reinstaller.h

@@ -19,7 +19,7 @@ class WebstoreReinstaller : public WebstoreStandaloneInstaller,
  public:
   WebstoreReinstaller(content::WebContents* web_contents,
                       const std::string& extension_id,
-                      const WebstoreStandaloneInstaller::Callback& callback);
+                      WebstoreStandaloneInstaller::Callback callback);
 
   // Begin the reinstall process. |callback| (from the constructor) will be
   // called upon completion.

chrome/browser/extensions/webstore_reinstaller_browsertest.cc

@@ -89,13 +89,10 @@ IN_PROC_BROWSER_TEST_F(WebstoreReinstallerBrowserTest, TestWebstoreReinstall) {
 
   // Create and run a WebstoreReinstaller.
   base::RunLoop run_loop;
-  scoped_refptr<WebstoreReinstaller> reinstaller(
-      new WebstoreReinstaller(
-          active_web_contents,
-          kTestExtensionId,
-          base::Bind(&WebstoreReinstallerBrowserTest::OnInstallCompletion,
-                     base::Unretained(this),
-                     run_loop.QuitClosure())));
+  scoped_refptr<WebstoreReinstaller> reinstaller(new WebstoreReinstaller(
+      active_web_contents, kTestExtensionId,
+      base::BindOnce(&WebstoreReinstallerBrowserTest::OnInstallCompletion,
+                     base::Unretained(this), run_loop.QuitClosure())));
   reinstaller->BeginReinstall();
   run_loop.Run();
 
@@ -108,13 +105,10 @@ IN_PROC_BROWSER_TEST_F(WebstoreReinstallerBrowserTest, TestWebstoreReinstall) {
   // Now accept the repair prompt.
   AutoAcceptInstall();
   base::RunLoop run_loop2;
-  reinstaller =
-      new WebstoreReinstaller(
-          active_web_contents,
-          kTestExtensionId,
-          base::Bind(&WebstoreReinstallerBrowserTest::OnInstallCompletion,
-                     base::Unretained(this),
-                     run_loop2.QuitClosure()));
+  reinstaller = new WebstoreReinstaller(
+      active_web_contents, kTestExtensionId,
+      base::BindOnce(&WebstoreReinstallerBrowserTest::OnInstallCompletion,
+                     base::Unretained(this), run_loop2.QuitClosure()));
   reinstaller->BeginReinstall();
   run_loop2.Run();
 

chrome/browser/extensions/webstore_standalone_installer.cc

@@ -31,9 +31,9 @@ namespace extensions {
 WebstoreStandaloneInstaller::WebstoreStandaloneInstaller(
     const std::string& webstore_item_id,
     Profile* profile,
-    const Callback& callback)
+    Callback callback)
     : id_(webstore_item_id),
-      callback_(callback),
+      callback_(std::move(callback)),
       profile_(profile),
       install_source_(WebstoreInstaller::INSTALL_SOURCE_INLINE),
       show_user_count_(true),
@@ -80,7 +80,8 @@ WebstoreStandaloneInstaller::~WebstoreStandaloneInstaller() {
 void WebstoreStandaloneInstaller::RunCallback(bool success,
                                               const std::string& error,
                                               webstore_install::Result result) {
-  callback_.Run(success, error, result);
+  DCHECK(callback_);
+  std::move(callback_).Run(success, error, result);
 }
 
 void WebstoreStandaloneInstaller::AbortInstall() {
@@ -117,7 +118,7 @@ void WebstoreStandaloneInstaller::CompleteInstall(
     const std::string& error) {
   scoped_active_install_.reset();
   if (!callback_.is_null())
-    callback_.Run(result == webstore_install::SUCCESS, error, result);
+    RunCallback(result == webstore_install::SUCCESS, error, result);
   Release();  // Matches the AddRef in BeginInstall.
 }
 

chrome/browser/extensions/webstore_standalone_installer.h

@@ -46,20 +46,21 @@ class WebstoreStandaloneInstaller
   // A callback for when the install process completes, successfully or not. If
   // there was a failure, |success| will be false and |error| may contain a
   // developer-readable error message about why it failed.
-  typedef base::Callback<void(bool success,
-                              const std::string& error,
-                              webstore_install::Result result)> Callback;
+  using Callback = base::OnceCallback<void(bool success,
+                                           const std::string& error,
+                                           webstore_install::Result result)>;
 
   WebstoreStandaloneInstaller(const std::string& webstore_item_id,
                               Profile* profile,
-                              const Callback& callback);
+                              Callback callback);
   void BeginInstall();
 
  protected:
   ~WebstoreStandaloneInstaller() override;
 
   // Runs the callback; primarily used for running a callback before it is
-  // cleared in AbortInstall().
+  // cleared in AbortInstall(). This should only be called once for the lifetime
+  // of the class.
   void RunCallback(
       bool success, const std::string& error, webstore_install::Result result);