Revert of Simplify ExtensionInstallChecker into a single-use class

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 fb6b9e6253dc6a06005031cc30dd56ebb44467ec..f70d9ddce34242be4c5b38bee2b6ca13b7b94b3f 100644
--- a/chrome/browser/extensions/extension_install_checker_unittest.cc
+++ b/chrome/browser/extensions/extension_install_checker_unittest.cc
@@ -18,6 +18,10 @@
 const char kDummyRequirementsError[] = "Requirements error";
 const char kDummyPolicyError[] = "Cannot install extension";
 
+const char kDummyPolicyError2[] = "Another policy error";
+const char kDummyRequirementsError2[] = "Another requirements error";
+const BlacklistState kBlacklistState2 = BLACKLISTED_SECURITY_VULNERABILITY;
+
 }  // namespace
 
 // Stubs most of the checks since we are interested in validating the logic in
@@ -25,8 +29,8 @@
 // checks.
 class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
  public:
-  ExtensionInstallCheckerForTest(int enabled_checks, bool fail_fast)
-      : ExtensionInstallChecker(nullptr, nullptr, enabled_checks, fail_fast),
+  ExtensionInstallCheckerForTest()
+      : ExtensionInstallChecker(NULL),
         requirements_check_called_(false),
         blacklist_check_called_(false),
         policy_check_called_(false),
@@ -46,25 +50,21 @@
   bool blacklist_check_called() const { return blacklist_check_called_; }
   bool policy_check_called() const { return policy_check_called_; }
 
-  void MockCheckRequirements() {
-    if (!is_running())
-      return;
+  void MockCheckRequirements(int sequence_number) {
     std::vector<std::string> errors;
     if (!requirements_error_.empty())
       errors.push_back(requirements_error_);
-    OnRequirementsCheckDone(errors);
-  }
-
-  void MockCheckBlacklistState() {
-    if (!is_running())
-      return;
-    OnBlacklistStateCheckDone(blacklist_state_);
+    OnRequirementsCheckDone(sequence_number, errors);
+  }
+
+  void MockCheckBlacklistState(int sequence_number) {
+    OnBlacklistStateCheckDone(sequence_number, blacklist_state_);
   }
 
  protected:
   void CheckRequirements() override {
     requirements_check_called_ = true;
-    MockCheckRequirements();
+    MockCheckRequirements(current_sequence_number());
   }
 
   void CheckManagementPolicy() override {
@@ -75,7 +75,15 @@
 
   void CheckBlacklistState() override {
     blacklist_check_called_ = true;
-    MockCheckBlacklistState();
+    MockCheckBlacklistState(current_sequence_number());
+  }
+
+  void ResetResults() override {
+    ExtensionInstallChecker::ResetResults();
+
+    requirements_check_called_ = false;
+    blacklist_check_called_ = false;
+    policy_check_called_ = false;
   }
 
   bool requirements_check_called_;
@@ -91,10 +99,6 @@
 // This class implements asynchronous mocks of the requirements and blacklist
 // checks.
 class ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
- public:
-  ExtensionInstallCheckerAsync(int enabled_checks, bool fail_fast)
-      : ExtensionInstallCheckerForTest(enabled_checks, fail_fast) {}
-
  protected:
   void CheckRequirements() override {
     requirements_check_called_ = true;
@@ -102,7 +106,7 @@
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&ExtensionInstallCheckerForTest::MockCheckRequirements,
-                   base::Unretained(this)));
+                   base::Unretained(this), current_sequence_number()));
   }
 
   void CheckBlacklistState() override {
@@ -111,7 +115,7 @@
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(&ExtensionInstallCheckerForTest::MockCheckBlacklistState,
-                   base::Unretained(this)));
+                   base::Unretained(this), current_sequence_number()));
   }
 };
 
@@ -143,6 +147,40 @@
  public:
   ExtensionInstallCheckerTest() {}
   ~ExtensionInstallCheckerTest() override {}
+
+  void RunSecondInvocation(ExtensionInstallCheckerForTest* checker,
+                           int checks_failed) {
+    EXPECT_GT(checks_failed, 0);
+    EXPECT_FALSE(checker->is_running());
+    ValidateExpectedCalls(ExtensionInstallChecker::CHECK_ALL, *checker);
+
+    // Set up different return values.
+    checker->set_blacklist_state(kBlacklistState2);
+    checker->set_policy_check_error(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,
+        false /* fail fast */,
+        base::Bind(&ExtensionInstallCheckerTest::ValidateSecondInvocation,
+                   base::Unretained(this),
+                   checker));
+  }
+
+  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* 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);
+
+    EXPECT_EQ(kBlacklistState2, checker->blacklist_state());
+    ExpectPolicyError(kDummyPolicyError2, *checker);
+    ExpectRequirementsError(kDummyRequirementsError2, *checker);
+  }
 
  protected:
   void SetAllErrors(ExtensionInstallCheckerForTest* checker) {
@@ -204,10 +242,14 @@
   }
 
   void RunChecker(ExtensionInstallCheckerForTest* checker,
+                  bool fail_fast,
+                  int checks_to_run,
                   int expected_checks_run,
                   int expected_result) {
     CheckObserver observer;
-    checker->Start(base::Bind(&CheckObserver::OnChecksComplete,
+    checker->Start(checks_to_run,
+                   fail_fast,
+                   base::Bind(&CheckObserver::OnChecksComplete,
                               base::Unretained(&observer)));
     observer.Wait();
 
@@ -219,6 +261,8 @@
 
   void DoRunAllChecksPass(ExtensionInstallCheckerForTest* checker) {
     RunChecker(checker,
+               false /* fail fast */,
+               ExtensionInstallChecker::CHECK_ALL,
                ExtensionInstallChecker::CHECK_ALL,
                0);
 
@@ -230,6 +274,8 @@
   void DoRunAllChecksFail(ExtensionInstallCheckerForTest* checker) {
     SetAllErrors(checker);
     RunChecker(checker,
+               false /* fail fast */,
+               ExtensionInstallChecker::CHECK_ALL,
                ExtensionInstallChecker::CHECK_ALL,
                ExtensionInstallChecker::CHECK_ALL);
 
@@ -238,34 +284,35 @@
     ExpectBlacklistError(*checker);
   }
 
-  void DoRunSubsetOfChecks(int checks_to_run) {
-    ExtensionInstallCheckerForTest sync_checker(checks_to_run,
-                                                /*fail_fast=*/false);
-    ExtensionInstallCheckerAsync async_checker(checks_to_run,
-                                               /*fail_fast=*/false);
-    ExtensionInstallCheckerForTest* checkers[] = {
-        &sync_checker, &async_checker,
-    };
-
-    for (auto* checker : checkers) {
-      SetAllErrors(checker);
-      RunChecker(checker, checks_to_run, checks_to_run);
-
-      if (checks_to_run & ExtensionInstallChecker::CHECK_REQUIREMENTS)
-        ExpectRequirementsError(*checker);
-      else
-        ExpectRequirementsPass(*checker);
-
-      if (checks_to_run & ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY)
-        ExpectPolicyError(*checker);
-      else
-        ExpectPolicyPass(*checker);
-
-      if (checks_to_run & ExtensionInstallChecker::CHECK_BLACKLIST)
-        ExpectBlacklistError(*checker);
-      else
-        ExpectBlacklistPass(*checker);
-    }
+  void DoRunSubsetOfChecks(ExtensionInstallCheckerForTest* checker) {
+    // Test check set 1.
+    int tests_to_run = ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY |
+                       ExtensionInstallChecker::CHECK_REQUIREMENTS;
+    SetAllErrors(checker);
+    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
+
+    ExpectRequirementsError(*checker);
+    ExpectPolicyError(*checker);
+    ExpectBlacklistPass(*checker);
+
+    // Test check set 2.
+    tests_to_run = ExtensionInstallChecker::CHECK_BLACKLIST |
+                   ExtensionInstallChecker::CHECK_REQUIREMENTS;
+    SetAllErrors(checker);
+    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
+
+    ExpectRequirementsError(*checker);
+    ExpectPolicyPass(*checker);
+    ExpectBlacklistError(*checker);
+
+    // Test a single check.
+    tests_to_run = ExtensionInstallChecker::CHECK_BLACKLIST;
+    SetAllErrors(checker);
+    RunChecker(checker, false, tests_to_run, tests_to_run, tests_to_run);
+
+    ExpectRequirementsPass(*checker);
+    ExpectPolicyPass(*checker);
+    ExpectBlacklistError(*checker);
   }
 
  private:
@@ -273,66 +320,108 @@
   base::MessageLoop message_loop;
 };
 
+class ExtensionInstallCheckerMultipleInvocationTest
+    : public ExtensionInstallCheckerTest {
+ public:
+  ExtensionInstallCheckerMultipleInvocationTest() : callback_count_(0) {}
+  ~ExtensionInstallCheckerMultipleInvocationTest() override {}
+
+  void RunSecondInvocation(ExtensionInstallCheckerForTest* 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);
+
+    // Set up different return values.
+    checker->set_blacklist_state(kBlacklistState2);
+    checker->set_policy_check_error(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,
+                   false /* fail fast */,
+                   base::Bind(&ExtensionInstallCheckerMultipleInvocationTest::
+                                  ValidateSecondInvocation,
+                              base::Unretained(this),
+                              checker));
+  }
+
+  void ValidateSecondInvocation(ExtensionInstallCheckerForTest* 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);
+
+    EXPECT_EQ(kBlacklistState2, checker->blacklist_state());
+    ExpectPolicyError(kDummyPolicyError2, *checker);
+    ExpectRequirementsError(kDummyRequirementsError2, *checker);
+  }
+
+ private:
+  int callback_count_;
+};
+
 // Test the case where all tests pass.
 TEST_F(ExtensionInstallCheckerTest, AllSucceeded) {
-  ExtensionInstallCheckerForTest sync_checker(
-      ExtensionInstallChecker::CHECK_ALL, /*fail_fast=*/false);
+  ExtensionInstallCheckerForTest sync_checker;
   DoRunAllChecksPass(&sync_checker);
 
-  ExtensionInstallCheckerAsync async_checker(ExtensionInstallChecker::CHECK_ALL,
-                                             /*fail_fast=*/false);
+  ExtensionInstallCheckerAsync async_checker;
   DoRunAllChecksPass(&async_checker);
 }
 
 // Test the case where all tests fail.
 TEST_F(ExtensionInstallCheckerTest, AllFailed) {
-  ExtensionInstallCheckerForTest sync_checker(
-      ExtensionInstallChecker::CHECK_ALL, /*fail_fast=*/false);
+  ExtensionInstallCheckerForTest sync_checker;
   DoRunAllChecksFail(&sync_checker);
 
-  ExtensionInstallCheckerAsync async_checker(ExtensionInstallChecker::CHECK_ALL,
-                                             /*fail_fast=*/false);
+  ExtensionInstallCheckerAsync async_checker;
   DoRunAllChecksFail(&async_checker);
 }
 
 // Test running only a subset of tests.
 TEST_F(ExtensionInstallCheckerTest, RunSubsetOfChecks) {
-  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY |
-                      ExtensionInstallChecker::CHECK_REQUIREMENTS);
-  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_BLACKLIST |
-                      ExtensionInstallChecker::CHECK_REQUIREMENTS);
-  DoRunSubsetOfChecks(ExtensionInstallChecker::CHECK_BLACKLIST);
+  ExtensionInstallCheckerForTest sync_checker;
+  ExtensionInstallCheckerAsync async_checker;
+  DoRunSubsetOfChecks(&sync_checker);
+  DoRunSubsetOfChecks(&async_checker);
 }
 
 // Test fail fast with synchronous callbacks.
 TEST_F(ExtensionInstallCheckerTest, FailFastSync) {
   // This test assumes some internal knowledge of the implementation - that
   // the policy check runs first.
-  {
-    ExtensionInstallCheckerForTest checker(ExtensionInstallChecker::CHECK_ALL,
-                                           /*fail_fast=*/true);
-    SetAllErrors(&checker);
-    RunChecker(&checker, ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
-               ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY);
-
-    ExpectRequirementsPass(checker);
-    ExpectPolicyError(checker);
-    ExpectBlacklistPass(checker);
-  }
-
-  {
-    ExtensionInstallCheckerForTest checker(
-        ExtensionInstallChecker::CHECK_REQUIREMENTS |
-            ExtensionInstallChecker::CHECK_BLACKLIST,
-        /*fail_fast=*/true);
-    SetAllErrors(&checker);
-    RunChecker(&checker, ExtensionInstallChecker::CHECK_REQUIREMENTS,
-               ExtensionInstallChecker::CHECK_REQUIREMENTS);
-
-    ExpectRequirementsError(checker);
-    ExpectPolicyPass(checker);
-    ExpectBlacklistPass(checker);
-  }
+  ExtensionInstallCheckerForTest checker;
+  SetAllErrors(&checker);
+  RunChecker(&checker,
+             true /* fail fast */,
+             ExtensionInstallChecker::CHECK_ALL,
+             ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY,
+             ExtensionInstallChecker::CHECK_MANAGEMENT_POLICY);
+
+  ExpectRequirementsPass(checker);
+  ExpectPolicyError(checker);
+  ExpectBlacklistPass(checker);
+
+  // This test assumes some internal knowledge of the implementation - that
+  // the requirements check runs before the blacklist check.
+  SetAllErrors(&checker);
+  RunChecker(&checker,
+             true /* fail fast */,
+             ExtensionInstallChecker::CHECK_REQUIREMENTS |
+                 ExtensionInstallChecker::CHECK_BLACKLIST,
+             ExtensionInstallChecker::CHECK_REQUIREMENTS,
+             ExtensionInstallChecker::CHECK_REQUIREMENTS);
+
+  ExpectRequirementsError(checker);
+  ExpectPolicyPass(checker);
+  ExpectBlacklistPass(checker);
 }
 
 // Test fail fast with asynchronous callbacks.
@@ -341,9 +430,7 @@
   // 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.
-  ExtensionInstallCheckerAsync checker(ExtensionInstallChecker::CHECK_ALL,
-                                       /*fail_fast=*/true);
-
+  ExtensionInstallCheckerAsync checker;
   SetAllErrors(&checker);
 
   // The policy check is synchronous and needs to pass for the other tests to
@@ -351,6 +438,8 @@
   checker.set_policy_check_error(std::string());
 
   RunChecker(&checker,
+             true /* fail fast */,
+             ExtensionInstallChecker::CHECK_ALL,
              ExtensionInstallChecker::CHECK_ALL,
              ExtensionInstallChecker::CHECK_REQUIREMENTS);
 
@@ -359,4 +448,41 @@
   ExpectBlacklistPass(checker);
 }
 
+// Test multiple invocations of the install checker. Wait for all checks to
+// complete.
+TEST_F(ExtensionInstallCheckerMultipleInvocationTest, CompleteAll) {
+  ExtensionInstallCheckerAsync checker;
+  SetAllErrors(&checker);
+
+  // Start the second check as soon as the callback of the first run is invoked.
+  checker.Start(
+      ExtensionInstallChecker::CHECK_ALL,
+      false /* fail fast */,
+      base::Bind(
+          &ExtensionInstallCheckerMultipleInvocationTest::RunSecondInvocation,
+          base::Unretained(this),
+          &checker));
+  base::RunLoop().RunUntilIdle();
+}
+
+// Test multiple invocations of the install checker and fail fast.
+TEST_F(ExtensionInstallCheckerMultipleInvocationTest, FailFast) {
+  ExtensionInstallCheckerAsync checker;
+  SetAllErrors(&checker);
+
+  // The policy check is synchronous and needs to pass for the other tests to
+  // run.
+  checker.set_policy_check_error(std::string());
+
+  // Start the second check as soon as the callback of the first run is invoked.
+  checker.Start(
+      ExtensionInstallChecker::CHECK_ALL,
+      true /* fail fast */,
+      base::Bind(
+          &ExtensionInstallCheckerMultipleInvocationTest::RunSecondInvocation,
+          base::Unretained(this),
+          &checker));
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace extensions