Simplify ExtensionInstallChecker into a single-use class

chrome/browser/extensions/extension_install_checker.cc

Index: chrome/browser/extensions/extension_install_checker.cc
diff --git a/chrome/browser/extensions/extension_install_checker.cc b/chrome/browser/extensions/extension_install_checker.cc
index 313225df59060d09c967dd4cf239bc78eb43db8f..08177ab6ef34f5b0679fcc05bb78d14387d13f47 100644
--- a/chrome/browser/extensions/extension_install_checker.cc
+++ b/chrome/browser/extensions/extension_install_checker.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/extension_install_checker.h"
 
+#include "base/callback_helpers.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/blacklist.h"
 #include "chrome/browser/extensions/chrome_requirements_checker.h"
@@ -14,22 +15,24 @@
 
 namespace extensions {
 
-ExtensionInstallChecker::ExtensionInstallChecker(Profile* profile)
+ExtensionInstallChecker::ExtensionInstallChecker(
+    Profile* profile,
+    scoped_refptr<const Extension> extension,
+    int enabled_checks,
+    bool fail_fast)
     : profile_(profile),
+      extension_(extension),
       blacklist_state_(NOT_BLACKLISTED),
       policy_allows_load_(true),
-      current_sequence_number_(0),
+      enabled_checks_(enabled_checks),
       running_checks_(0),

[Devlin, 2017/03/20 15:36:46]

We could combine running_checks_ and enabled_checks_ into e.g.
remaining_checks_.

[michaelpg, 2017/03/22 00:17:12]

Added a TODO. running_checks_ has a use currently (detects if Start() is called
more than once). We can probably simplify that into a bool or maybe remove the
NOTREACHED altogether, but i'll save that for a possible future CL.

[Devlin, 2017/03/22 15:37:13]

TODO sounds fine, but I don't see one added - am I just missing it?

[michaelpg, 2017/03/22 20:46:10]

Weird. Uploaded again and it's there (in the header)

-      fail_fast_(false),
-      weak_ptr_factory_(this) {
-}
+      fail_fast_(fail_fast),
+      weak_ptr_factory_(this) {}
 
 ExtensionInstallChecker::~ExtensionInstallChecker() {
 }
 
-void ExtensionInstallChecker::Start(int enabled_checks,
-                                    bool fail_fast,
-                                    const Callback& callback) {
+void ExtensionInstallChecker::Start(const Callback& callback) {
   // Profile is null in tests.
   if (profile_) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -39,30 +42,28 @@ void ExtensionInstallChecker::Start(int enabled_checks,
     }
   }
 
-  if (is_running() || !enabled_checks || callback.is_null()) {
+  if (is_running() || !enabled_checks_ || callback.is_null()) {
     NOTREACHED();

[Devlin, 2017/03/20 15:36:46]

Unrelated to this CL, but usually we only want to use a NOTREACHED(); return;
pattern when it's possible to happen through e.g. untrusted code (like IPCs,
weird JS, etc).  This seems like we can just move it to a CHECK/DCHECK.  We
shouldn't do that in this CL, but can you add a TODO? (Ditto for above)

[michaelpg, 2017/03/22 00:17:12]

Done.

     return;
   }
 
-  running_checks_ = enabled_checks;
-  fail_fast_ = fail_fast;
+  running_checks_ = enabled_checks_;
   callback_ = callback;
-  ResetResults();
 
   // Execute the management policy check first as it is synchronous.
-  if (enabled_checks & CHECK_MANAGEMENT_POLICY) {
+  if (enabled_checks_ & CHECK_MANAGEMENT_POLICY) {
     CheckManagementPolicy();
     if (!is_running())
       return;
   }
 
-  if (enabled_checks & CHECK_REQUIREMENTS) {
+  if (enabled_checks_ & CHECK_REQUIREMENTS) {
     CheckRequirements();
     if (!is_running())
       return;
   }
 
-  if (enabled_checks & CHECK_BLACKLIST)
+  if (enabled_checks_ & CHECK_BLACKLIST)
     CheckBlacklistState();
 }
 
@@ -92,17 +93,14 @@ void ExtensionInstallChecker::CheckRequirements() {
   if (!requirements_checker_.get())
     requirements_checker_.reset(new ChromeRequirementsChecker());
   requirements_checker_->Check(
-      extension_,
-      base::Bind(&ExtensionInstallChecker::OnRequirementsCheckDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 current_sequence_number_));
+      extension_, base::Bind(&ExtensionInstallChecker::OnRequirementsCheckDone,
+                             weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ExtensionInstallChecker::OnRequirementsCheckDone(
-    int sequence_number,
     const std::vector<std::string>& errors) {
   // Some pending results may arrive after fail fast.
-  if (sequence_number != current_sequence_number_)
+  if (!is_running())

[Devlin, 2017/03/20 15:36:46]

Actually, I think we can make this (D)CHECK is_running().  MaybeInvokeCallback()
invalidates weak ptrs if fail_fast_ is true and we fail, so the callback won't
be invoked.

[michaelpg, 2017/03/22 00:17:12]

Done.

     return;
 
   requirement_errors_ = errors;
@@ -118,14 +116,12 @@ void ExtensionInstallChecker::CheckBlacklistState() {
   blacklist->IsBlacklisted(
       extension_->id(),
       base::Bind(&ExtensionInstallChecker::OnBlacklistStateCheckDone,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 current_sequence_number_));
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void ExtensionInstallChecker::OnBlacklistStateCheckDone(int sequence_number,
-                                                        BlacklistState state) {
+void ExtensionInstallChecker::OnBlacklistStateCheckDone(BlacklistState state) {
   // Some pending results may arrive after fail fast.
-  if (sequence_number != current_sequence_number_)
+  if (!is_running())

[Devlin, 2017/03/20 15:36:46]

ditto

[michaelpg, 2017/03/22 00:17:12]

Done.

     return;
 
   blacklist_state_ = state;
@@ -134,13 +130,6 @@ void ExtensionInstallChecker::OnBlacklistStateCheckDone(int sequence_number,
   MaybeInvokeCallback();
 }
 
-void ExtensionInstallChecker::ResetResults() {
-  requirement_errors_.clear();
-  blacklist_state_ = NOT_BLACKLISTED;
-  policy_allows_load_ = true;
-  policy_error_.clear();
-}
-
 void ExtensionInstallChecker::MaybeInvokeCallback() {
   if (callback_.is_null())
     return;
@@ -160,14 +149,7 @@ void ExtensionInstallChecker::MaybeInvokeCallback() {
     // If we are failing fast, discard any pending results.
     weak_ptr_factory_.InvalidateWeakPtrs();
     running_checks_ = 0;
-    ++current_sequence_number_;
-
-    Callback callback_copy = callback_;
-    callback_.Reset();
-
-    // This instance may be owned by the callback recipient and deleted here,
-    // so reset |callback_| first and invoke a copy of the callback.
-    callback_copy.Run(failed_mask);
+    base::ResetAndReturn(&callback_).Run(failed_mask);
   }
 }