[Extensions Callback Cleanup] Miscellaneous Installation Util

Do the following clean-ups in some miscellaneous installation utility
classes/files in extensions code:
- 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: Ia58a8467b6956bdf2bd15557d51f312e9cc0a4cc
Reviewed-on: https://chromium-review.googlesource.com/c/1391866
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619835}

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

@@ -1157,8 +1157,8 @@ void DeveloperPrivateLoadUnpackedFunction::FileSelected(
   scoped_refptr<UnpackedInstaller> installer(
       UnpackedInstaller::Create(GetExtensionService(browser_context())));
   installer->set_be_noisy_on_failure(!fail_quietly_);
-  installer->set_completion_callback(
-      base::Bind(&DeveloperPrivateLoadUnpackedFunction::OnLoadComplete, this));
+  installer->set_completion_callback(base::BindOnce(
+      &DeveloperPrivateLoadUnpackedFunction::OnLoadComplete, this));
   installer->Load(path);
 
   retry_guid_ = DeveloperPrivateAPI::Get(browser_context())

chrome/browser/extensions/content_verifier_test_utils.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/extensions/content_verifier_test_utils.h"
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/run_loop.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/external_install_info.h"
@@ -86,9 +90,9 @@ const std::vector<base::TimeDelta>& DelayTracker::calls() {
   return calls_;
 }
 
-void DelayTracker::ReinstallAction(const base::RepeatingClosure& callback,
+void DelayTracker::ReinstallAction(base::OnceClosure callback,
                                    base::TimeDelta delay) {
-  saved_callback_ = callback;
+  saved_callback_ = std::move(callback);
   calls_.push_back(delay);
 }
 
@@ -96,9 +100,9 @@ void DelayTracker::Proceed() {
   ASSERT_TRUE(saved_callback_);
   ASSERT_TRUE(!saved_callback_->is_null());
   // Run() will set |saved_callback_| again, so use a temporary: |callback|.
-  base::RepeatingClosure callback = saved_callback_.value();
+  base::OnceClosure callback = std::move(saved_callback_.value());
   saved_callback_.reset();
-  callback.Run();
+  std::move(callback).Run();
 }
 
 void DelayTracker::StopWatching() {

chrome/browser/extensions/content_verifier_test_utils.h

@@ -93,14 +93,13 @@ class DelayTracker {
   ~DelayTracker();
 
   const std::vector<base::TimeDelta>& calls();
-  void ReinstallAction(const base::RepeatingClosure& callback,
-                       base::TimeDelta delay);
+  void ReinstallAction(base::OnceClosure callback, base::TimeDelta delay);
   void Proceed();
   void StopWatching();
 
  private:
   std::vector<base::TimeDelta> calls_;
-  base::Optional<base::RepeatingClosure> saved_callback_;
+  base::Optional<base::OnceClosure> saved_callback_;
   PolicyExtensionReinstaller::ReinstallCallback action_;
 
   DISALLOW_COPY_AND_ASSIGN(DelayTracker);

chrome/browser/extensions/install_signer.cc

@@ -316,17 +316,17 @@ void LogRequestStartHistograms() {
 
 }  // namespace
 
-void InstallSigner::GetSignature(const SignatureCallback& callback) {
+void InstallSigner::GetSignature(SignatureCallback callback) {
   CHECK(!simple_loader_.get());
   CHECK(callback_.is_null());
   CHECK(salt_.empty());
-  callback_ = callback;
+  callback_ = std::move(callback);
 
   // If the set of ids is empty, just return an empty signature and skip the
   // call to the server.
   if (ids_.empty()) {
     if (!callback_.is_null())
-      callback_.Run(std::unique_ptr<InstallSignature>(new InstallSignature()));
+      std::move(callback_).Run(std::make_unique<InstallSignature>());
     return;
   }
 
@@ -417,9 +417,8 @@ void InstallSigner::GetSignature(const SignatureCallback& callback) {
 }
 
 void InstallSigner::ReportErrorViaCallback() {
-  InstallSignature* null_signature = NULL;
   if (!callback_.is_null())
-    callback_.Run(std::unique_ptr<InstallSignature>(null_signature));
+    std::move(callback_).Run(nullptr);
 }
 
 void InstallSigner::ParseFetchResponse(
@@ -513,7 +512,7 @@ void InstallSigner::HandleSignatureResult(const std::string& signature,
   }
 
   if (!callback_.is_null())
-    callback_.Run(std::move(result));
+    std::move(callback_).Run(std::move(result));
 }
 
 

chrome/browser/extensions/install_signer.h

@@ -61,8 +61,8 @@ struct InstallSignature {
 // that a set of ids are hosted in the webstore.
 class InstallSigner {
  public:
-  typedef base::Callback<void(std::unique_ptr<InstallSignature>)>
-      SignatureCallback;
+  using SignatureCallback =
+      base::OnceCallback<void(std::unique_ptr<InstallSignature>)>;
 
   // IMPORTANT NOTE: It is possible that only some, but not all, of the entries
   // in |ids| will be successfully signed by the backend. Callers should always
@@ -80,7 +80,7 @@ class InstallSigner {
   // Begins the process of fetching a signature from the backend. This should
   // only be called once! If you want to get another signature, make another
   // instance of this class.
-  void GetSignature(const SignatureCallback& callback);
+  void GetSignature(SignatureCallback callback);
 
   // Returns whether the signature in InstallSignature is properly signed with a
   // known public key.

chrome/browser/extensions/install_verifier.cc

@@ -583,8 +583,8 @@ void InstallVerifier::BeginFetch() {
       content::BrowserContext::GetDefaultStoragePartition(context_)
           ->GetURLLoaderFactoryForBrowserProcess();
   signer_ = std::make_unique<InstallSigner>(url_loader_factory, ids_to_sign);
-  signer_->GetSignature(base::Bind(&InstallVerifier::SignatureCallback,
-                                   weak_factory_.GetWeakPtr()));
+  signer_->GetSignature(base::BindOnce(&InstallVerifier::SignatureCallback,
+                                       weak_factory_.GetWeakPtr()));
 }
 
 void InstallVerifier::SaveToPrefs() {

chrome/browser/extensions/policy_extension_reinstaller.cc

@@ -86,13 +86,13 @@ void PolicyExtensionReinstaller::ScheduleNextReinstallAttempt() {
 
   scheduled_fire_pending_ = true;
   base::TimeDelta reinstall_delay = GetNextFireDelay();
-  base::Closure callback =
-      base::Bind(&PolicyExtensionReinstaller::Fire, weak_factory_.GetWeakPtr());
+  base::OnceClosure callback = base::BindOnce(&PolicyExtensionReinstaller::Fire,
+                                              weak_factory_.GetWeakPtr());
   if (g_reinstall_action_for_test) {
-    g_reinstall_action_for_test->Run(callback, reinstall_delay);
+    g_reinstall_action_for_test->Run(std::move(callback), reinstall_delay);
   } else {
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(FROM_HERE, callback,
-                                                         reinstall_delay);
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, std::move(callback), reinstall_delay);
   }
 }
 

chrome/browser/extensions/policy_extension_reinstaller.h

@@ -24,8 +24,9 @@ namespace extensions {
 // it will retry reinstallation with backoff.
 class PolicyExtensionReinstaller {
  public:
-  using ReinstallCallback = base::Callback<void(const base::Closure& callback,
-                                                base::TimeDelta delay)>;
+  using ReinstallCallback =
+      base::RepeatingCallback<void(base::OnceClosure callback,
+                                   base::TimeDelta delay)>;
 
   explicit PolicyExtensionReinstaller(content::BrowserContext* context);
   ~PolicyExtensionReinstaller();

chrome/browser/extensions/policy_extension_reinstaller_unittest.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/extensions/policy_extension_reinstaller.h"
 
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -19,31 +23,31 @@ const char kDummyExtensionId[] = "whatever";
 class TestReinstallerTracker {
  public:
   TestReinstallerTracker()
-      : action_(base::Bind(&TestReinstallerTracker::ReinstallAction,
-                           base::Unretained(this))) {
+      : action_(base::BindRepeating(&TestReinstallerTracker::ReinstallAction,
+                                    base::Unretained(this))) {
     PolicyExtensionReinstaller::set_policy_reinstall_action_for_test(&action_);
   }
   ~TestReinstallerTracker() {
     PolicyExtensionReinstaller::set_policy_reinstall_action_for_test(nullptr);
   }
-  void ReinstallAction(const base::Closure& callback,
+  void ReinstallAction(base::OnceClosure callback,
                        base::TimeDelta reinstall_delay) {
     ++call_count_;
-    saved_callback_ = callback;
+    saved_callback_ = std::move(callback);
   }
   void Proceed() {
     DCHECK(saved_callback_);
     DCHECK(!saved_callback_->is_null());
     // Run() will set |saved_callback_| again, so use a temporary.
-    base::Closure callback = saved_callback_.value();
+    base::OnceClosure callback = std::move(saved_callback_.value());
     saved_callback_.reset();
-    callback.Run();
+    std::move(callback).Run();
   }
   int call_count() { return call_count_; }
 
  private:
   int call_count_ = 0;
-  base::Optional<base::Closure> saved_callback_;
+  base::Optional<base::OnceClosure> saved_callback_;
   PolicyExtensionReinstaller::ReinstallCallback action_;
 
   DISALLOW_COPY_AND_ASSIGN(TestReinstallerTracker);

chrome/browser/extensions/unpacked_installer.cc

@@ -342,10 +342,8 @@ void UnpackedInstaller::ReportExtensionLoadError(const std::string &error) {
         extension_path_, error, service_weak_->profile(), be_noisy_on_failure_);
   }
 
-  if (!callback_.is_null()) {
-    callback_.Run(nullptr, extension_path_, error);
-    callback_.Reset();
-  }
+  if (!callback_.is_null())
+    std::move(callback_).Run(nullptr, extension_path_, error);
 }
 
 void UnpackedInstaller::InstallExtension() {
@@ -364,10 +362,8 @@ void UnpackedInstaller::InstallExtension() {
                                       kInstallFlagInstallImmediately,
                                       dnr_ruleset_checksum_);
 
-  if (!callback_.is_null()) {
-    callback_.Run(extension(), extension_path_, std::string());
-    callback_.Reset();
-  }
+  if (!callback_.is_null())
+    std::move(callback_).Run(extension(), extension_path_, std::string());
 }
 
 }  // namespace extensions

chrome/browser/extensions/unpacked_installer.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
@@ -34,9 +35,9 @@ class PreloadCheckGroup;
 class UnpackedInstaller
     : public base::RefCountedThreadSafe<UnpackedInstaller> {
  public:
-  using CompletionCallback = base::Callback<void(const Extension* extension,
-                                                 const base::FilePath&,
-                                                 const std::string&)>;
+  using CompletionCallback = base::OnceCallback<void(const Extension* extension,
+                                                     const base::FilePath&,
+                                                     const std::string&)>;
 
   static scoped_refptr<UnpackedInstaller> Create(
       ExtensionService* extension_service);
@@ -72,8 +73,8 @@ class UnpackedInstaller
     be_noisy_on_failure_ = be_noisy_on_failure;
   }
 
-  void set_completion_callback(const CompletionCallback& callback) {
-    callback_ = callback;
+  void set_completion_callback(CompletionCallback callback) {
+    callback_ = std::move(callback);
   }
 
  private: