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..a2e70ca87fd83fc530368539413e464e61c3ee5a 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,69 +509,82 @@ 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;
 
+  if (errors.empty()) {
+    ConfirmInstall();
+    return;
+  }
+
   // Check for requirement errors.
-  if (!install_checker_->requirements_error_message().empty()) {
+  if (!requirements_check_->GetErrorMessage().empty()) {

[Devlin, 2017/04/06 02:03:53]

I know I mentioned before that it's weird to sometimes consult
GetErrorMessage(), other times consult |errors| in a different CL... do you have
any ideas in the pipeline to make it a bit less unusual?  (Not something that
needs to be done for this CL, but I do think it would be nice to have something
more clear).

[michaelpg, 2017/04/06 22:31:16]

:-\ Both pieces of info are useful. We could add a standard HadSomeError()
method (adding more redundancy), or we could explicitly check for every error
type (there are 3 requirements errors we'd check for). I see why it's weird, but
I haven't thought of anything clearly better.

The utility of PreloadCheck[Group] is outsourcing the boilerplate sync/async
check/check all code. At the end of the day we still have to allow for examining
each error and taking action individually.

[Devlin, 2017/04/07 14:33:57]

One thought I had is that we could make an Error struct
PreloadCheck::Error {
  ErrorCode code;
  base::string16 message;
}

That way, we still have all the utility have having code + error, and we don't
have the weirdness of sometimes checking errors.contains(foo) and sometimes
checking PreloadCheck::GetErrorMessage().

A few drawbacks:
- This doesn't work when we coalesce multiple errors from a preload check into a
single error message, like we do in RequirementsChecker.  Instead, that would be
the responsibility of the callback (or a utility method called by the callback).
- errors.count(SOME_ERROR) doesn't work, unless we have an operator==(ErrorCode)
on Error, or we make an ErrorSet class.

Overall, this might lead to overarchitecting.  For the time being, I'm fine with
this, but if/when we add more of these checks, it will get increasingly weird. 
Do you have an idea how many of these checks we'll have?

     if (error_on_unsupported_requirements_) {
       ReportFailureFromUIThread(
           CrxInstallError(CrxInstallError::ERROR_DECLINED,
-                          install_checker_->requirements_error_message()));
+                          requirements_check_->GetErrorMessage()));
       return;
     }
     install_flags_ |= kInstallFlagHasRequirementErrors;
   }
 
   // Check the blacklist state.
-  if (install_checker_->blacklist_error() == 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;
+  if (errors.count(PreloadCheck::BLACKLISTED_ID) ||
+      errors.count(PreloadCheck::BLACKLISTED_UNKNOWN)) {
+    if (allow_silent_install_) {
+      // 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))
+        install_flags_ |= kInstallFlagIsBlacklistedForMalware;
+    } else {
+      // 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.
-
   // Check for policy errors.
-  if (!install_checker_->policy_error().empty()) {
+  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, install_checker_->policy_error()));
+        CrxInstallError::ERROR_DECLINED, policy_check_->GetErrorMessage()));
     return;
   }