Use PreloadCheckGroup in ExtensionInstallChecker. Make RequirementsChecker a PreloadCheck.

chrome/browser/extensions/extension_install_checker_unittest.cc

Index: chrome/browser/extensions/extension_install_checker_unittest.cc
diff --git a/chrome/browser/extensions/extension_install_checker_unittest.cc b/chrome/browser/extensions/extension_install_checker_unittest.cc
index 7b655ad3596621b02f25658330fa1b2d62587a3b..e68dd697da5eb1b2e82d2022f3a3c9a39d366f2a 100644
--- a/chrome/browser/extensions/extension_install_checker_unittest.cc
+++ b/chrome/browser/extensions/extension_install_checker_unittest.cc
@@ -17,149 +17,96 @@ namespace extensions {
 
 namespace {
 
+const PreloadCheck::Error kRequirementsError =
+    PreloadCheck::WEBGL_NOT_SUPPORTED;
+const base::string16 kDummyRequirementsError =
+    base::ASCIIToUTF16("Requirements error");
+const PreloadCheck::Error kRequirementsError2 =
+    PreloadCheck::WINDOW_SHAPE_NOT_SUPPORTED;
+const base::string16 kDummyRequirementsError2 =
+    base::ASCIIToUTF16("Another requirements error");
+
 const PreloadCheck::Error kBlacklistError = PreloadCheck::BLACKLISTED_ID;
-const char kDummyRequirementsError[] = "Requirements error";
-const char kDummyPolicyError[] = "Cannot install extension";
+const PreloadCheck::Error kBlacklistError2 = PreloadCheck::BLACKLISTED_UNKNOWN;
 
-const char kDummyPolicyError2[] = "Another policy error";
-const char kDummyRequirementsError2[] = "Another requirements error";
-const PreloadCheck::Error kBlacklistError2 = PreloadCheck::NONE;
+const base::string16 kDummyPolicyError =
+    base::ASCIIToUTF16("Cannot install extension");
+const base::string16 kDummyPolicyError2 =
+    base::ASCIIToUTF16("Another policy error");
 
 }  // namespace
 
-// Stubs most of the checks since we are interested in validating the logic in
-// the install checker. This class implements a synchronous version of all
-// checks.
-class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
+class ExtensionInstallCheckerObserver : public PreloadCheckObserver {
  public:
-  explicit ExtensionInstallCheckerForTest(bool is_async)
-      : ExtensionInstallChecker(NULL),
-        requirements_check_called_(false),
-        blacklist_check_called_(false),
-        policy_check_called_(false),
-        is_async_(is_async) {}
-
-  ~ExtensionInstallCheckerForTest() override {}
-
-  void set_requirements_error(const std::string& error) {
-    requirements_error_ = error;
-  }
-
-  bool requirements_check_called() const { return requirements_check_called_; }
-  bool blacklist_check_called() const { return blacklist_check_called_; }
-  bool policy_check_called() const { return policy_check_called_; }
+  ~ExtensionInstallCheckerObserver() override {}
 
-  bool is_async() { return is_async_; }
+  int result() { return result_; }
 
-  void MockCheckRequirements(int sequence_number) {
-    std::vector<std::string> errors;
-    if (!requirements_error_.empty())
-      errors.push_back(requirements_error_);
-    OnRequirementsCheckDone(sequence_number, errors);
-  }
-
- protected:
-  void CheckRequirements() override {
-    requirements_check_called_ = true;
-    if (is_async_) {
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::Bind(&ExtensionInstallCheckerForTest::MockCheckRequirements,
-                     base::Unretained(this), current_sequence_number()));
-    } else {
-      MockCheckRequirements(current_sequence_number());
-    }
-  }
-
-  void CheckManagementPolicy() override {
-    policy_check_called_ = true;
-    ExtensionInstallChecker::CheckManagementPolicy();
-  }
-
-  void CheckBlacklistState() override {
-    blacklist_check_called_ = true;
-    ExtensionInstallChecker::CheckBlacklistState();
-  }
-
-  void ResetResults() override {
-    ExtensionInstallChecker::ResetResults();
-
-    requirements_check_called_ = false;
-    blacklist_check_called_ = false;
-    policy_check_called_ = false;
-  }
-
-  bool requirements_check_called_;
-  bool blacklist_check_called_;
-  bool policy_check_called_;
-
-  // Dummy errors for testing.
-  std::string requirements_error_;
-
-  // Whether the checks that can run asynchronously should do so.
-  bool is_async_;
-};
-
-class CheckObserver {
- public:
-  CheckObserver() : result_(0), call_count_(0) {}
-
-  int result() const { return result_; }
-  int call_count() const { return call_count_; }
-
-  void OnChecksComplete(int checks_failed) {
-    result_ = checks_failed;
-    ++call_count_;
-  }
-
-  void Wait() {
-    if (call_count_)
-      return;
-
-    base::RunLoop().RunUntilIdle();
+  void OnChecksComplete(int result) {
+    result_ = result;
+    // We're tracking errors separately, but still want to update called_.
+    OnCheckComplete(PreloadCheck::Errors());
   }
 
  private:
   int result_;
-  int call_count_;
 };
 
 class ExtensionInstallCheckerTest : public testing::Test {
  public:
-  ExtensionInstallCheckerTest() {}
+  ExtensionInstallCheckerTest() : 
+    requirements_checker_stub_(nullptr),
+    blacklist_check_stub_(nullptr),
+    policy_check_stub_(nullptr),
+    is_async_(false) {}
   ~ExtensionInstallCheckerTest() override {}
 
-  void SetBlacklistError(ExtensionInstallCheckerForTest* checker,
+  void SetRequirementsError(ExtensionInstallChecker* checker,
+                            PreloadCheck::Error error,
+                            base::string16 message) {
+    std::unique_ptr<PreloadCheckStub> requirements_checker =
+        base::MakeUnique<PreloadCheckStub>(is_async_);
+    if (error != PreloadCheck::NONE) {
+      requirements_checker->AddError(error);
+      requirements_checker->set_error_message(message);
+    }
+    requirements_checker_stub_ = requirements_checker.get();
+    checker->SetRequirementsCheckerForTesting(std::move(requirements_checker));
+  }
+
+  void SetBlacklistError(ExtensionInstallChecker* checker,
                          PreloadCheck::Error error) {
     std::unique_ptr<PreloadCheckStub> blacklist_check =
-        base::MakeUnique<PreloadCheckStub>(checker->is_async());
+        base::MakeUnique<PreloadCheckStub>(is_async_);
     if (error != PreloadCheck::NONE)
       blacklist_check->AddError(error);
+    blacklist_check_stub_ = blacklist_check.get();
     checker->SetBlacklistCheckForTesting(std::move(blacklist_check));
   }
 
-  void SetPolicyError(ExtensionInstallCheckerForTest* checker,
+  void SetPolicyError(ExtensionInstallChecker* checker,
                       PreloadCheck::Error error,
-                      std::string message) {
+                      base::string16 message) {
     // The policy check always runs synchronously.
     std::unique_ptr<PreloadCheckStub> policy_check =
         base::MakeUnique<PreloadCheckStub>();
     if (error != PreloadCheck::NONE) {
       policy_check->AddError(error);
-      policy_check->set_error_message(base::UTF8ToUTF16(message));
+      policy_check->set_error_message(message);
     }
+    policy_check_stub_ = policy_check.get();
     checker->SetPolicyCheckForTesting(std::move(policy_check));
   }
 
-  void RunSecondInvocation(ExtensionInstallCheckerForTest* checker,
+  void RunSecondInvocation(ExtensionInstallChecker* checker,
                            int checks_failed) {
     EXPECT_GT(checks_failed, 0);
     EXPECT_FALSE(checker->is_running());
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
+    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL);
 
     // Set up different return values.
-    checker->set_requirements_error(kDummyRequirementsError2);
-
+    SetRequirementsError(
+        checker, kRequirementsError2, kDummyRequirementsError2);
     SetBlacklistError(checker, PreloadCheck::NONE);
     SetPolicyError(checker, PreloadCheck::DISALLOWED_BY_POLICY,
                    kDummyPolicyError2);
@@ -167,6 +114,7 @@ class ExtensionInstallCheckerTest : public testing::Test {
     // Run the install checker again and ensure the second set of return values
     // is received.
     checker->Start(
+        nullptr,
         ExtensionInstallChecker::CHECK_ALL,
         false /* fail fast */,
         base::Bind(&ExtensionInstallCheckerTest::ValidateSecondInvocation,
@@ -174,13 +122,13 @@ class ExtensionInstallCheckerTest : public testing::Test {
                    checker));
   }
 
-  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* checker,
+  void ValidateSecondInvocation(ExtensionInstallChecker* checker,
                                 int checks_failed) {
     EXPECT_FALSE(checker->is_running());
     EXPECT_EQ(ExtensionInstallChecker::CHECK_REQUIREMENTS |
                   ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
               checks_failed);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
+    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL);
 
     EXPECT_EQ(kBlacklistError2, checker->blacklist_error());
     ExpectPolicyError(kDummyPolicyError2, *checker);
@@ -188,88 +136,92 @@ class ExtensionInstallCheckerTest : public testing::Test {
   }
 
  protected:
-  void SetAllPass(ExtensionInstallCheckerForTest* checker) {
+  void SetAllPass(ExtensionInstallChecker* checker) {
+    SetRequirementsError(checker, PreloadCheck::NONE, base::string16());
     SetBlacklistError(checker, PreloadCheck::NONE);
-    SetPolicyError(checker, PreloadCheck::NONE, "");
-    checker->set_requirements_error("");
+    SetPolicyError(checker, PreloadCheck::NONE, base::string16());
   }
 
-  void SetAllErrors(ExtensionInstallCheckerForTest* checker) {
+  void SetAllErrors(ExtensionInstallChecker* checker) {
+    SetRequirementsError(checker, kRequirementsError, kDummyRequirementsError);
     SetBlacklistError(checker, kBlacklistError);
     SetPolicyError(checker, PreloadCheck::DISALLOWED_BY_POLICY,
                    kDummyPolicyError);
-    checker->set_requirements_error(kDummyRequirementsError);
   }
 
-  void ValidateExpectedCalls(int call_mask,
-                             const ExtensionInstallCheckerForTest& checker) {
+  void ValidateExpectedCalls(int call_mask) {
     bool expect_blacklist_checked =
         (call_mask & ExtensionInstallChecker::CHECK_BLACKLIST) != 0;
     bool expect_requirements_checked =
         (call_mask & ExtensionInstallChecker::CHECK_REQUIREMENTS) != 0;
     bool expect_policy_checked =
         (call_mask & ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY) != 0;
-    EXPECT_EQ(expect_blacklist_checked, checker.blacklist_check_called());
-    EXPECT_EQ(expect_policy_checked, checker.policy_check_called());
-    EXPECT_EQ(expect_requirements_checked, checker.requirements_check_called());
+    EXPECT_EQ(expect_blacklist_checked, 
+              blacklist_check_stub_ && blacklist_check_stub_->started());
+    EXPECT_EQ(expect_policy_checked,
+        policy_check_stub_ && policy_check_stub_->started());
+    EXPECT_EQ(expect_requirements_checked,
+        requirements_checker_stub_ && requirements_checker_stub_->started());
   }
 
-  void ExpectRequirementsPass(const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_TRUE(checker.requirement_errors().empty());
+  void ExpectRequirementsPass(const ExtensionInstallChecker& checker) {
+    EXPECT_TRUE(checker.requirement_error_message().empty());
   }
 
-  void ExpectRequirementsError(const char* expected_error,
-                               const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_FALSE(checker.requirement_errors().empty());
-    EXPECT_EQ(std::string(expected_error),
-              checker.requirement_errors().front());
+  void ExpectRequirementsError(base::string16 expected_error,
+                               const ExtensionInstallChecker& checker) {
+    EXPECT_FALSE(checker.requirement_error_message().empty());
+    EXPECT_EQ(expected_error,
+              checker.requirement_error_message());
   }
 
-  void ExpectRequirementsError(const ExtensionInstallCheckerForTest& checker) {
+  void ExpectRequirementsError(const ExtensionInstallChecker& checker) {
     ExpectRequirementsError(kDummyRequirementsError, checker);
   }
 
-  void ExpectBlacklistPass(const ExtensionInstallCheckerForTest& checker) {
+  void ExpectBlacklistPass(const ExtensionInstallChecker& checker) {
     EXPECT_EQ(PreloadCheck::NONE, checker.blacklist_error());
   }
 
-  void ExpectBlacklistError(const ExtensionInstallCheckerForTest& checker) {
+  void ExpectBlacklistError(const ExtensionInstallChecker& checker) {
     EXPECT_EQ(kBlacklistError, checker.blacklist_error());
   }
 
-  void ExpectPolicyPass(const ExtensionInstallCheckerForTest& checker) {
+  void ExpectPolicyPass(const ExtensionInstallChecker& checker) {
     EXPECT_TRUE(checker.policy_error().empty());
   }
 
-  void ExpectPolicyError(const char* expected_error,
-                         const ExtensionInstallCheckerForTest& checker) {
+  void ExpectPolicyError(base::string16 expected_error,
+                         const ExtensionInstallChecker& checker) {
     EXPECT_FALSE(checker.policy_error().empty());
-    EXPECT_EQ(std::string(expected_error), checker.policy_error());
+    EXPECT_EQ(expected_error, checker.policy_error());
   }
 
-  void ExpectPolicyError(const ExtensionInstallCheckerForTest& checker) {
+  void ExpectPolicyError(const ExtensionInstallChecker& checker) {
     ExpectPolicyError(kDummyPolicyError, checker);
   }
 
-  void RunChecker(ExtensionInstallCheckerForTest* checker,
+  void RunChecker(ExtensionInstallChecker* checker,
                   bool fail_fast,
                   int checks_to_run,
                   int expected_checks_run,
                   int expected_result) {
-    CheckObserver observer;
-    checker->Start(checks_to_run,
+    ExtensionInstallCheckerObserver observer;
+    checker->Start(nullptr,
+                   checks_to_run,
                    fail_fast,
-                   base::Bind(&CheckObserver::OnChecksComplete,
-                              base::Unretained(&observer)));
+                   base::Bind(
+                       &ExtensionInstallCheckerObserver::OnChecksComplete,
+                       base::Unretained(&observer)));
     observer.Wait();
 
     EXPECT_FALSE(checker->is_running());
     EXPECT_EQ(expected_result, observer.result());
-    EXPECT_EQ(1, observer.call_count());
-    ValidateExpectedCalls(expected_checks_run, *checker);
+    EXPECT_TRUE(observer.called());
+    ValidateExpectedCalls(expected_checks_run);
   }
 
-  void DoRunAllChecksPass(ExtensionInstallCheckerForTest* checker) {
+  void DoRunAllChecksPass(ExtensionInstallChecker* checker) {
     SetAllPass(checker);
     RunChecker(checker,
                false /* fail fast */,
@@ -282,7 +234,7 @@ class ExtensionInstallCheckerTest : public testing::Test {
     ExpectBlacklistPass(*checker);
   }
 
-  void DoRunAllChecksFail(ExtensionInstallCheckerForTest* checker) {
+  void DoRunAllChecksFail(ExtensionInstallChecker* checker) {
     SetAllErrors(checker);
     RunChecker(checker,
                false /* fail fast */,
@@ -295,7 +247,7 @@ class ExtensionInstallCheckerTest : public testing::Test {
     ExpectBlacklistError(*checker);
   }
 
-  void DoRunSubsetOfChecks(ExtensionInstallCheckerForTest* checker) {
+  void DoRunSubsetOfChecks(ExtensionInstallChecker* checker) {
     // Test check set 1.
     int tests_to_run = ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY |
                        ExtensionInstallChecker::CHECK_REQUIREMENTS;
@@ -326,9 +278,17 @@ class ExtensionInstallCheckerTest : public testing::Test {
     ExpectBlacklistError(*checker);
   }
 
+  // Call to specify whether the checks should simulate running asynchronously.
+  void set_is_async(bool is_async) { is_async_ = is_async; }
+
  private:
   // A message loop is required for the asynchronous tests.
   base::MessageLoop message_loop;
+
+  PreloadCheckStub* requirements_checker_stub_;
+  PreloadCheckStub* blacklist_check_stub_;
+  PreloadCheckStub* policy_check_stub_;
+  bool is_async_;
 };
 
 class ExtensionInstallCheckerMultipleInvocationTest
@@ -337,23 +297,25 @@ class ExtensionInstallCheckerMultipleInvocationTest
   ExtensionInstallCheckerMultipleInvocationTest() : callback_count_(0) {}
   ~ExtensionInstallCheckerMultipleInvocationTest() override {}
 
-  void RunSecondInvocation(ExtensionInstallCheckerForTest* checker,
+  void RunSecondInvocation(ExtensionInstallChecker* checker,
                            int checks_failed) {
     ASSERT_EQ(0, callback_count_);
     ++callback_count_;
     EXPECT_FALSE(checker->is_running());
     EXPECT_GT(checks_failed, 0);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
+    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL);
 
     // Set up different return values.
+    SetRequirementsError(
+        checker, kRequirementsError2, kDummyRequirementsError2);
     SetBlacklistError(checker, kBlacklistError2);
     SetPolicyError(checker, PreloadCheck::DISALLOWED_BY_POLICY,
                    kDummyPolicyError2);
-    checker->set_requirements_error(kDummyRequirementsError2);
 
     // Run the install checker again and ensure the second set of return values
     // is received.
-    checker->Start(ExtensionInstallChecker::CHECK_ALL,
+    checker->Start(nullptr,
+                   ExtensionInstallChecker::CHECK_ALL,
                    false /* fail fast */,
                    base::Bind(&ExtensionInstallCheckerMultipleInvocationTest::
                                   ValidateSecondInvocation,
@@ -361,14 +323,14 @@ class ExtensionInstallCheckerMultipleInvocationTest
                               checker));
   }
 
-  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* checker,
+  void ValidateSecondInvocation(ExtensionInstallChecker* checker,
                                 int checks_failed) {
     ASSERT_EQ(1, callback_count_);
     EXPECT_FALSE(checker->is_running());
     EXPECT_EQ(ExtensionInstallChecker::CHECK_REQUIREMENTS |
                   ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
               checks_failed);
-    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
+    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL);
 
     EXPECT_EQ(kBlacklistError2, checker->blacklist_error());
     ExpectPolicyError(kDummyPolicyError2, *checker);
@@ -381,28 +343,31 @@ class ExtensionInstallCheckerMultipleInvocationTest
 
 // Test the case where all tests pass.
 TEST_F(ExtensionInstallCheckerTest, AllSucceeded) {
-  ExtensionInstallCheckerForTest sync_checker(false);
+  ExtensionInstallChecker sync_checker(nullptr);
   DoRunAllChecksPass(&sync_checker);
 
-  ExtensionInstallCheckerForTest async_checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker async_checker(nullptr);
   DoRunAllChecksPass(&async_checker);
 }
 
 // Test the case where all tests fail.
 TEST_F(ExtensionInstallCheckerTest, AllFailed) {
-  ExtensionInstallCheckerForTest sync_checker(false);
+  ExtensionInstallChecker sync_checker(nullptr);
   DoRunAllChecksFail(&sync_checker);
 
-  ExtensionInstallCheckerForTest async_checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker async_checker(nullptr);
   DoRunAllChecksFail(&async_checker);
 }
 
 // Test running only a subset of tests.
 TEST_F(ExtensionInstallCheckerTest, RunSubsetOfChecks) {
-  ExtensionInstallCheckerForTest sync_checker(false);
+  ExtensionInstallChecker sync_checker(nullptr);
   DoRunSubsetOfChecks(&sync_checker);
 
-  ExtensionInstallCheckerForTest async_checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker async_checker(nullptr);
   DoRunSubsetOfChecks(&async_checker);
 }
 
@@ -410,7 +375,7 @@ TEST_F(ExtensionInstallCheckerTest, RunSubsetOfChecks) {
 TEST_F(ExtensionInstallCheckerTest, FailFastSync) {
   // This test assumes some internal knowledge of the implementation - that
   // the policy check runs first.
-  ExtensionInstallCheckerForTest checker(false);
+  ExtensionInstallChecker checker(nullptr);
   SetAllErrors(&checker);
   RunChecker(&checker,
              true /* fail fast */,
@@ -443,12 +408,13 @@ TEST_F(ExtensionInstallCheckerTest, FailFastAsync) {
   // the requirements check runs before the blacklist check. Both checks should
   // be called, but the requirements check callback arrives first and the
   // blacklist result will be discarded.
-  ExtensionInstallCheckerForTest checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker checker(nullptr);
   SetAllErrors(&checker);
 
   // The policy check is synchronous and needs to pass for the other tests to
   // run.
-  SetPolicyError(&checker, PreloadCheck::NONE, "");
+  SetPolicyError(&checker, PreloadCheck::NONE, base::string16());
 
   RunChecker(&checker,
              true /* fail fast */,
@@ -464,11 +430,13 @@ TEST_F(ExtensionInstallCheckerTest, FailFastAsync) {
 // Test multiple invocations of the install checker. Wait for all checks to
 // complete.
 TEST_F(ExtensionInstallCheckerMultipleInvocationTest, CompleteAll) {
-  ExtensionInstallCheckerForTest checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker checker(nullptr);
   SetAllErrors(&checker);
 
   // Start the second check as soon as the callback of the first run is invoked.
   checker.Start(
+      nullptr,
       ExtensionInstallChecker::CHECK_ALL,
       false /* fail fast */,
       base::Bind(
@@ -480,15 +448,17 @@ TEST_F(ExtensionInstallCheckerMultipleInvocationTest, CompleteAll) {
 
 // Test multiple invocations of the install checker and fail fast.
 TEST_F(ExtensionInstallCheckerMultipleInvocationTest, FailFast) {
-  ExtensionInstallCheckerForTest checker(true);
+  set_is_async(true);
+  ExtensionInstallChecker checker(nullptr);
   SetAllErrors(&checker);
 
   // The policy check is synchronous and needs to pass for the other tests to
   // run.
-  SetPolicyError(&checker, PreloadCheck::NONE, "");
+  SetPolicyError(&checker, PreloadCheck::NONE, base::string16());
 
   // Start the second check as soon as the callback of the first run is invoked.
   checker.Start(
+      nullptr,
       ExtensionInstallChecker::CHECK_ALL,
       true /* fail fast */,
       base::Bind(