Replace ExtensionInstallChecker with generic PreloadCheckGroup

chrome/browser/extensions/crx_installer.cc

 // Copyright (c) 2012 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/crx_installer.h"
 
 #include <map>
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #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"
 #include "chrome/browser/extensions/permissions_updater.h"
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/install/crx_install_error.h"
 #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"
 #include "extensions/common/manifest_handlers/kiosk_mode_info.h"
 #include "extensions/common/manifest_handlers/shared_module_info.h"
 #include "extensions/common/manifest_url_handlers.h"
 #include "extensions/common/permissions/permission_message_provider.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/user_script.h"
@@ skipping 438 matching lines @@
             CrxInstallError::ERROR_DECLINED,
             l10n_util::GetStringFUTF16(
                 IDS_EXTENSION_INSTALL_DEPENDENCY_NOT_WHITELISTED,
                 base::UTF8ToUTF16(extension()->name()),
                 base::UTF8ToUTF16(imported_module->name()))));
         return;
       }
     }
   }
 
-  // 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()));
+  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;
+    }
+
+    // Check the blacklist state.
+    if (errors.count(PreloadCheck::BLACKLISTED_ID))
+      install_flags_ |= kInstallFlagIsBlacklistedForMalware;
+
+    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;
     }
-    install_flags_ |= kInstallFlagHasRequirementErrors;
-  }
 
-  // Check the blacklist state.
-  if (install_checker_->blacklist_error() == PreloadCheck::BLACKLISTED_ID) {
-    install_flags_ |= kInstallFlagIsBlacklistedForMalware;
-  }
+    // 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 ((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.
-
-  // 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;
+    // 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();
 }
 
 void CrxInstaller::ConfirmInstall() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   ExtensionService* service = service_weak_.get();
   if (!service || service->browser_terminating())
     return;
@@ skipping 321 matching lines @@
         ExtensionInstallPrompt::GetReEnablePromptTypeForExtension(
             service->profile(), extension());
     client_->ShowDialog(base::Bind(&CrxInstaller::OnInstallPromptDone, this),
                         extension(), nullptr,
                         base::MakeUnique<ExtensionInstallPrompt::Prompt>(type),
                         ExtensionInstallPrompt::GetDefaultShowDialogCallback());
   }
 }
 
 }  // namespace extensions