Move ChromeRequirementsChecker to //extensions as a PreloadCheck

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/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/blacklist.h"
 #include "chrome/browser/extensions/blacklist_check.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/policy_check.h"
+#include "extensions/browser/requirements_checker.h"
 
 namespace extensions {
 
 ExtensionInstallChecker::ExtensionInstallChecker(
     Profile* profile,
     scoped_refptr<const Extension> extension,
     int enabled_checks,
     bool fail_fast)
     : profile_(profile),
       extension_(extension),
@@ skipping 55 matching lines @@
   if (!errors.empty()) {
     DCHECK_EQ(1u, errors.count(PreloadCheck::DISALLOWED_BY_POLICY));
     policy_error_ = base::UTF16ToUTF8(policy_check_->GetErrorMessage());
   }
 
   running_checks_ &= ~CHECK_MANAGEMENT_POLICY;
   MaybeInvokeCallback();
 }
 
 void ExtensionInstallChecker::CheckRequirements() {
-  requirements_checker_ = base::MakeUnique<ChromeRequirementsChecker>();
-  requirements_checker_->Check(
-      extension_, base::Bind(&ExtensionInstallChecker::OnRequirementsCheckDone,
-                             weak_ptr_factory_.GetWeakPtr()));
+  // In tests, this check may already be stubbed.
+  if (!requirements_check_) {
+    requirements_check_ =
+        base::MakeUnique<RequirementsChecker>(extension_.get());
+  }
+  requirements_check_->Start(
+      base::BindOnce(&ExtensionInstallChecker::OnRequirementsCheckDone,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ExtensionInstallChecker::OnRequirementsCheckDone(
-    const std::vector<std::string>& errors) {
+    PreloadCheck::Errors errors) {
   DCHECK(is_running());
 
-  requirement_errors_ = errors;
+  requirements_error_message_ =
+      base::UTF16ToUTF8(requirements_check_->GetErrorMessage());

[Devlin, 2017/03/30 17:10:55]

This back and forth between conversions is a shame.  Can we just store it as a
base::string16 on this object?

[michaelpg, 2017/03/30 22:49:01]

Done.

 
   running_checks_ &= ~CHECK_REQUIREMENTS;
   MaybeInvokeCallback();
 }
 
 void ExtensionInstallChecker::CheckBlacklistState() {
   // In tests, this check may already be stubbed.
   if (!blacklist_check_) {
     blacklist_check_ = base::MakeUnique<BlacklistCheck>(
         Blacklist::Get(profile_), extension_.get());
@@ skipping 19 matching lines @@
 }
 
 void ExtensionInstallChecker::MaybeInvokeCallback() {
   if (callback_.is_null())
     return;
 
   // Set bits for failed checks.
   int failed_mask = 0;
   if (blacklist_error_ == PreloadCheck::BLACKLISTED_ID)
     failed_mask |= CHECK_BLACKLIST;
-  if (!requirement_errors_.empty())
+  if (requirements_error_message_.size())

[Devlin, 2017/03/30 17:10:55]

nit: I think we have a slight preference to prefer !empty to size.

It's also a little weird to me that we rely on different indicators here for
whether there's an error - checking the code for blacklist error and the message
for policy and requirements errors.  It'd be cleaner to see if we can coalesce
those, but not a big deal right now (and can be saved for later cls).

[michaelpg, 2017/03/30 22:49:00]

Done. It's not slight -- apparently it's about 35:1, in /chrome and /extensions
:-O
Agreed. I'm not sure how to fix that, since requirements errors could be any of
a number of codes and there isn't not a pretty way of writing "if foo in {...}".
Maybe a helper function? Stay tuned for the next CL.

     failed_mask |= CHECK_REQUIREMENTS;
   if (!policy_error_.empty())
     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_)) {
     running_checks_ = 0;
 
     // If we are failing fast, discard any pending results.
     blacklist_check_.reset();
     policy_check_.reset();
-    requirements_checker_.reset();
+    requirements_check_.reset();
     weak_ptr_factory_.InvalidateWeakPtrs();
     base::ResetAndReturn(&callback_).Run(failed_mask);
   }
 }
 
 }  // namespace extensions