Replace ExtensionInstallChecker with generic PreloadCheckGroup

chrome/browser/extensions/crx_installer.cc

Index: chrome/browser/extensions/crx_installer.cc
diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index 05462ce54952023d8afb93524cc9f1de1bc53c69..1ff793bab104cad4f20c09c3f4d7f3394bd8a733 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -23,11 +23,11 @@
 #include "base/time/time.h"
 #include "base/version.h"
 #include "build/build_config.h"
+#include "chrome/browser/extensions/blacklist_check.h"
 #include "chrome/browser/extensions/convert_user_script.h"
 #include "chrome/browser/extensions/convert_web_app.h"
 #include "chrome/browser/extensions/extension_assets_manager.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
-#include "chrome/browser/extensions/extension_install_checker.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/install_tracker.h"
 #include "chrome/browser/extensions/install_tracker_factory.h"
@@ -48,7 +48,9 @@
 #include "extensions/browser/install/extension_install_ui.h"
 #include "extensions/browser/install_flag.h"
 #include "extensions/browser/notification_types.h"
-#include "extensions/browser/preload_check.h"
+#include "extensions/browser/policy_check.h"
+#include "extensions/browser/preload_check_group.h"
+#include "extensions/browser/requirements_checker.h"
 #include "extensions/common/extension_icon_set.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/manifest.h"
@@ -507,70 +509,79 @@ void CrxInstaller::CheckInstall() {
     }
   }
 
-  // Run the policy, requirements and blacklist checks in parallel. Skip the
-  // checks if the extension is a bookmark app.
+  // Skip the checks if the extension is a bookmark app.
   if (extension()->from_bookmark()) {
     ConfirmInstall();
-  } else {
-    install_checker_ = base::MakeUnique<ExtensionInstallChecker>(
-        profile_, extension_, ExtensionInstallChecker::CHECK_ALL,
-        false /* fail fast */);
-    install_checker_->Start(
-        base::Bind(&CrxInstaller::OnInstallChecksComplete, this));
+    return;
   }
+
+  // Run the policy, requirements and blacklist checks in parallel.
+  check_group_ = base::MakeUnique<PreloadCheckGroup>();
+
+  policy_check_ = base::MakeUnique<PolicyCheck>(profile_, extension());
+  requirements_check_ = base::MakeUnique<RequirementsChecker>(extension());
+  blacklist_check_ =
+      base::MakeUnique<BlacklistCheck>(Blacklist::Get(profile_), extension_);
+
+  check_group_->AddCheck(policy_check_.get());
+  check_group_->AddCheck(requirements_check_.get());
+  check_group_->AddCheck(blacklist_check_.get());
+
+  check_group_->Start(
+      base::BindOnce(&CrxInstaller::OnInstallChecksComplete, this));
 }
 
-void CrxInstaller::OnInstallChecksComplete(int failed_checks) {
+void CrxInstaller::OnInstallChecksComplete(PreloadCheck::Errors errors) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!service_weak_)
     return;
 
-  // Check for requirement errors.
-  if (!install_checker_->requirements_error_message().empty()) {
-    if (error_on_unsupported_requirements_) {
-      ReportFailureFromUIThread(
-          CrxInstallError(CrxInstallError::ERROR_DECLINED,
-                          install_checker_->requirements_error_message()));
-      return;
+  if (!errors.empty()) {

[Devlin, 2017/04/04 01:37:26]

this block hurts my eyes a bit.  Maybe
if (errors.empty()) {
  ConfirmInstall();
  return;
}

WDYT?  Worth the duplicated ConfirmInstall() at the end?

[michaelpg, 2017/04/04 21:41:29]

Done. I also fiddled with the order of nested ifs below. (It does change when
|install_flags_| is updated for the failure case, but it's ignored in that case
anyway, which matches the other checks too.)

+    // Check for requirement errors.
+    if (!requirements_check_->GetErrorMessage().empty()) {
+      if (error_on_unsupported_requirements_) {
+        ReportFailureFromUIThread(
+            CrxInstallError(CrxInstallError::ERROR_DECLINED,
+                            requirements_check_->GetErrorMessage()));
+        return;
+      }
+      install_flags_ |= kInstallFlagHasRequirementErrors;
     }
-    install_flags_ |= kInstallFlagHasRequirementErrors;
-  }
 
-  // Check the blacklist state.
-  if (install_checker_->blacklist_error() == PreloadCheck::BLACKLISTED_ID) {
-    install_flags_ |= kInstallFlagIsBlacklistedForMalware;
-  }
+    // Check the blacklist state.
+    if (errors.count(PreloadCheck::BLACKLISTED_ID))
+      install_flags_ |= kInstallFlagIsBlacklistedForMalware;
 
-  if ((install_checker_->blacklist_error() == PreloadCheck::BLACKLISTED_ID ||
-       install_checker_->blacklist_error() ==
-           PreloadCheck::BLACKLISTED_UNKNOWN) &&
-      !allow_silent_install_) {
-    // User tried to install a blacklisted extension. Show an error and
-    // refuse to install it.
-    ReportFailureFromUIThread(CrxInstallError(
-        CrxInstallError::ERROR_DECLINED,
-        l10n_util::GetStringFUTF16(IDS_EXTENSION_IS_BLACKLISTED,
-                                   base::UTF8ToUTF16(extension()->name()))));
-    UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.BlockCRX",
-                              extension()->location(),
-                              Manifest::NUM_LOCATIONS);
-    return;
-  }
-
-  // NOTE: extension may still be blacklisted, but we're forced to silently
-  // install it. In this case, ExtensionService::OnExtensionInstalled needs to
-  // deal with it.
+    if ((errors.count(PreloadCheck::BLACKLISTED_ID) ||
+         errors.count(PreloadCheck::BLACKLISTED_UNKNOWN)) &&
+        !allow_silent_install_) {
+      // User tried to install a blacklisted extension. Show an error and
+      // refuse to install it.
+      ReportFailureFromUIThread(CrxInstallError(
+          CrxInstallError::ERROR_DECLINED,
+          l10n_util::GetStringFUTF16(IDS_EXTENSION_IS_BLACKLISTED,
+                                     base::UTF8ToUTF16(extension()->name()))));
+      UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.BlockCRX",
+                                extension()->location(),
+                                Manifest::NUM_LOCATIONS);
+      return;
+    }
 
-  // Check for policy errors.
-  if (!install_checker_->policy_error().empty()) {
-    // We don't want to show the error infobar for installs from the WebStore,
-    // because the WebStore already shows an error dialog itself.
-    // Note: |client_| can be NULL in unit_tests!
-    if (extension()->from_webstore() && client_)
-      client_->install_ui()->SetSkipPostInstallUI(true);
-    ReportFailureFromUIThread(CrxInstallError(
-        CrxInstallError::ERROR_DECLINED, install_checker_->policy_error()));
-    return;
+    // NOTE: extension may still be blacklisted, but we're forced to silently
+    // install it. In this case, ExtensionService::OnExtensionInstalled needs to
+    // deal with it.
+
+    // Check for policy errors.
+    if (errors.count(PreloadCheck::DISALLOWED_BY_POLICY)) {
+      // We don't want to show the error infobar for installs from the WebStore,
+      // because the WebStore already shows an error dialog itself.
+      // Note: |client_| can be NULL in unit_tests!
+      if (extension()->from_webstore() && client_)
+        client_->install_ui()->SetSkipPostInstallUI(true);
+      ReportFailureFromUIThread(CrxInstallError(
+          CrxInstallError::ERROR_DECLINED, policy_check_->GetErrorMessage()));
+      return;
+    }
   }
 
   ConfirmInstall();