Simplify ExtensionInstallChecker into a single-use class

chrome/browser/extensions/extension_install_checker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_install_checker.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/blacklist.h"
 #include "chrome/browser/extensions/chrome_requirements_checker.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/management_policy.h"
 
 namespace extensions {
 
-ExtensionInstallChecker::ExtensionInstallChecker(Profile* profile)
+ExtensionInstallChecker::ExtensionInstallChecker(
+    Profile* profile,
+    scoped_refptr<const Extension> extension)
     : profile_(profile),
+      extension_(extension),
       blacklist_state_(NOT_BLACKLISTED),
       policy_allows_load_(true),
-      current_sequence_number_(0),
+      enabled_checks_(0),
       running_checks_(0),
       fail_fast_(false),
-      weak_ptr_factory_(this) {
-}
+      weak_ptr_factory_(this) {}
 
 ExtensionInstallChecker::~ExtensionInstallChecker() {
 }
 
 void ExtensionInstallChecker::Start(int enabled_checks,
                                     bool fail_fast,
                                     const Callback& callback) {
+  DCHECK_EQ(enabled_checks_, 0)
+      << "ExtensionInstallChecker::Start() can be called at most once";
+
   // Profile is null in tests.
   if (profile_) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     if (!extension_.get()) {
       NOTREACHED();
       return;
     }
   }
 
   if (is_running() || !enabled_checks || callback.is_null()) {
     NOTREACHED();
     return;
   }
 
+  enabled_checks_ = enabled_checks;

[Devlin, 2017/03/16 19:29:40]

With this class becoming single use, we can move a lot of these to the ctor.

[michaelpg, 2017/03/17 04:25:23]

Done.

   running_checks_ = enabled_checks;
   fail_fast_ = fail_fast;
   callback_ = callback;
-  ResetResults();
 
   // Execute the management policy check first as it is synchronous.
   if (enabled_checks & CHECK_MANAGEMENT_POLICY) {
     CheckManagementPolicy();
     if (!is_running())

[Devlin, 2017/03/16 19:29:40]

not your code, but we're here. :)

these is_running() calls are weird and kind of useless.  I think it's clearer to
just have
if (enabled & FOO) {
  CheckFoo();
}
if (enabled & BAR) {
  CheckBar();
}

I don't think the early-returns here are buying us much, if anything.

[michaelpg, 2017/03/17 04:25:23]

this particular early return triggers if the management policy check fails and
fail_fast = true. Changing that would be a change in behavior for this class,
which I'd rather not combine with a refactoring. (Also don't know if I agree
with that change.)

The early return after CheckRequirements() is useless, because that check
happens async (with the current permutation of RequirementsChecker and
WebGlChecker implementations used by Chrome), *but* the next CL or two will move
this logic into a PreloadCheckGroup class anyway, so I'd rather keep as-is for
now.

       return;
   }
 
   if (enabled_checks & CHECK_REQUIREMENTS) {
     CheckRequirements();
     if (!is_running())
       return;
   }
 
   if (enabled_checks & CHECK_BLACKLIST)
@@ skipping 19 matching lines @@
   running_checks_ &= ~CHECK_MANAGEMENT_POLICY;
   MaybeInvokeCallback();
 }
 
 void ExtensionInstallChecker::CheckRequirements() {
   DCHECK(extension_.get());
 
   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())
     return;
 
   requirement_errors_ = errors;
 
   running_checks_ &= ~CHECK_REQUIREMENTS;
   MaybeInvokeCallback();
 }
 
 void ExtensionInstallChecker::CheckBlacklistState() {
   DCHECK(extension_.get());
 
   extensions::Blacklist* blacklist = Blacklist::Get(profile_);
   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())
     return;
 
   blacklist_state_ = state;
 
   running_checks_ &= ~CHECK_BLACKLIST;
   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;
 
   // Set bits for failed checks.
   int failed_mask = 0;
   if (blacklist_state_ == BLACKLISTED_MALWARE)
     failed_mask |= CHECK_BLACKLIST;
   if (!requirement_errors_.empty())
     failed_mask |= CHECK_REQUIREMENTS;
   if (!policy_allows_load_)
     failed_mask |= CHECK_MANAGEMENT_POLICY;
 
   // Invoke callback if all checks are complete or there was at least one
   // failure and |fail_fast_| is true.
   if (!is_running() || (failed_mask && fail_fast_)) {
     // 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);

[Devlin, 2017/03/16 19:29:40]

sorry for the drive-by:
base::ResetAndReturn(&callback_).Run(failed_mask);

[michaelpg, 2017/03/17 04:25:23]

Awesome! Done and suggested documentation in
https://codereview.chromium.org/2755073002

   }
 }
 
 }  // namespace extensions