Update ExtensionInstallChecker to use PreloadCheck classes

chrome/browser/extensions/extension_install_checker_unittest.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.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "chrome/browser/extensions/extension_install_checker.h"
+#include "extensions/browser/preload_check_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
 namespace {
 
-const BlacklistState kBlacklistStateError = BLACKLISTED_MALWARE;
+const PreloadCheck::Error kBlacklistError = PreloadCheck::BLACKLISTED_ID;
 const char kDummyRequirementsError[] = "Requirements error";
 const char kDummyPolicyError[] = "Cannot install extension";
 
 }  // 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 {
  public:
   ExtensionInstallCheckerForTest(int enabled_checks, bool fail_fast)
       : ExtensionInstallChecker(nullptr, nullptr, enabled_checks, fail_fast),
         requirements_check_called_(false),
         blacklist_check_called_(false),
-        policy_check_called_(false),
-        blacklist_state_(NOT_BLACKLISTED) {}
+        policy_check_called_(false) {}
 
   ~ExtensionInstallCheckerForTest() override {}
 
   void set_requirements_error(const std::string& error) {
     requirements_error_ = error;
   }
-  void set_policy_check_error(const std::string& error) {
-    policy_check_error_ = error;
-  }
-  void set_blacklist_state(BlacklistState state) { blacklist_state_ = state; }
 
   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_; }
 
+  virtual bool is_async() { return false; }
+
   void MockCheckRequirements() {
     if (!is_running())
       return;
     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_);
-  }
-
  protected:
   void CheckRequirements() override {
     requirements_check_called_ = true;
-    MockCheckRequirements();
+    if (is_async()) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE,
+          base::Bind(&ExtensionInstallCheckerForTest::MockCheckRequirements,
+                     base::Unretained(this)));
+    } else {
+      MockCheckRequirements();
+    }
   }
 
   void CheckManagementPolicy() override {
     policy_check_called_ = true;

[Devlin, 2017/03/22 15:08:46]

(Mostly just thinking out loud) I'm a little dubious of how useful these checks
are.  Given we can supply the preload check now, I'd probably prefer removing
the virtualness of these functions and relying on the supplied check to do the
right thing, and inferring whether or not the check was called through other
ways (e.g., does a checker correctly pass or fail with running checks and
certain assumptions).  WDYT?

Regardless, not something that needs to be done in this CL.

[michaelpg, 2017/03/24 02:34:16]

Makes sense; these primarily existed for mocking which we don't need anymore.
The test themselves should be enough to verify the checker behaves correctly.

-    OnManagementPolicyCheckDone(policy_check_error_.empty(),
-                                policy_check_error_);
+    ExtensionInstallChecker::CheckManagementPolicy();
   }
 
   void CheckBlacklistState() override {
     blacklist_check_called_ = true;
-    MockCheckBlacklistState();
+    ExtensionInstallChecker::CheckBlacklistState();
   }
 
   bool requirements_check_called_;
   bool blacklist_check_called_;
   bool policy_check_called_;
 
-  // Dummy errors for testing.
+  // Dummy error for testing.
   std::string requirements_error_;
-  std::string policy_check_error_;
-  BlacklistState blacklist_state_;
 };
 
 // This class implements asynchronous mocks of the requirements and blacklist
 // checks.
 class ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {

[Devlin, 2017/03/22 15:08:46]

Given how small this class is, I'd just expose a set_async method on
ExtensionInstallCheckerForTest.

[michaelpg, 2017/03/24 02:34:16]

Reasonable.

  public:
   ExtensionInstallCheckerAsync(int enabled_checks, bool fail_fast)
       : ExtensionInstallCheckerForTest(enabled_checks, fail_fast) {}
+  ~ExtensionInstallCheckerAsync() override {}
 
  protected:
-  void CheckRequirements() override {
-    requirements_check_called_ = true;
-
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&ExtensionInstallCheckerForTest::MockCheckRequirements,
-                   base::Unretained(this)));
-  }
-
-  void CheckBlacklistState() override {
-    blacklist_check_called_ = true;
-
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(&ExtensionInstallCheckerForTest::MockCheckBlacklistState,
-                   base::Unretained(this)));
-  }
+  bool is_async() override { return true; }
 };
 
 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) {
@@ skipping 11 matching lines @@
  private:
   int result_;
   int call_count_;
 };
 
 class ExtensionInstallCheckerTest : public testing::Test {
  public:
   ExtensionInstallCheckerTest() {}
   ~ExtensionInstallCheckerTest() override {}
 
+  void SetBlacklistError(ExtensionInstallCheckerForTest* checker,
+                         PreloadCheck::Error error) {
+    std::unique_ptr<PreloadCheckStub> blacklist_check =
+        base::MakeUnique<PreloadCheckStub>(checker->is_async());
+    if (error != PreloadCheck::NONE)
+      blacklist_check->AddError(error);
+    checker->SetBlacklistCheckForTesting(std::move(blacklist_check));
+  }
+
+  void SetPolicyError(ExtensionInstallCheckerForTest* checker,
+                      PreloadCheck::Error error,
+                      std::string 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));
+    }
+    checker->SetPolicyCheckForTesting(std::move(policy_check));
+  }
+
  protected:
+  void SetAllPass(ExtensionInstallCheckerForTest* checker) {
+    SetBlacklistError(checker, PreloadCheck::NONE);
+    SetPolicyError(checker, PreloadCheck::NONE, "");
+    checker->set_requirements_error("");
+  }
+
   void SetAllErrors(ExtensionInstallCheckerForTest* checker) {
-    checker->set_blacklist_state(kBlacklistStateError);
-    checker->set_policy_check_error(kDummyPolicyError);
+    SetBlacklistError(checker, kBlacklistError);
+    SetPolicyError(checker, PreloadCheck::DISALLOWED_BY_POLICY,
+                   kDummyPolicyError);
     checker->set_requirements_error(kDummyRequirementsError);
   }
 
   void ValidateExpectedCalls(int call_mask,
                              const ExtensionInstallCheckerForTest& checker) {
     bool expect_blacklist_checked =
         (call_mask & ExtensionInstallChecker::CHECK_BLACKLIST) != 0;
     bool expect_requirements_checked =
         (call_mask & ExtensionInstallChecker::CHECK_REQUIREMENTS) != 0;
     bool expect_policy_checked =
@@ skipping 12 matching lines @@
     EXPECT_FALSE(checker.requirement_errors().empty());
     EXPECT_EQ(std::string(expected_error),
               checker.requirement_errors().front());
   }
 
   void ExpectRequirementsError(const ExtensionInstallCheckerForTest& checker) {
     ExpectRequirementsError(kDummyRequirementsError, checker);
   }
 
   void ExpectBlacklistPass(const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_EQ(NOT_BLACKLISTED, checker.blacklist_state());
+    EXPECT_EQ(PreloadCheck::NONE, checker.blacklist_error());
   }
 
   void ExpectBlacklistError(const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_EQ(kBlacklistStateError, checker.blacklist_state());
+    EXPECT_EQ(kBlacklistError, checker.blacklist_error());
   }
 
   void ExpectPolicyPass(const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_TRUE(checker.policy_allows_load());
     EXPECT_TRUE(checker.policy_error().empty());
   }
 
   void ExpectPolicyError(const char* expected_error,
                          const ExtensionInstallCheckerForTest& checker) {
-    EXPECT_FALSE(checker.policy_allows_load());
     EXPECT_FALSE(checker.policy_error().empty());
     EXPECT_EQ(std::string(expected_error), checker.policy_error());
   }
 
   void ExpectPolicyError(const ExtensionInstallCheckerForTest& checker) {
     ExpectPolicyError(kDummyPolicyError, checker);
   }
 
   void RunChecker(ExtensionInstallCheckerForTest* checker,
                   int expected_checks_run,
                   int expected_result) {
     CheckObserver observer;
     checker->Start(base::Bind(&CheckObserver::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);
   }
 
   void DoRunAllChecksPass(ExtensionInstallCheckerForTest* checker) {
+    SetAllPass(checker);
     RunChecker(checker,
                ExtensionInstallChecker::CHECK_ALL,
                0);
 
     ExpectRequirementsPass(*checker);
     ExpectPolicyPass(*checker);
     ExpectBlacklistPass(*checker);
   }
 
   void DoRunAllChecksFail(ExtensionInstallCheckerForTest* checker) {
@@ skipping 105 matching lines @@
 }
 
 // Test fail fast with asynchronous callbacks.
 TEST_F(ExtensionInstallCheckerTest, FailFastAsync) {
   // This test assumes some internal knowledge of the implementation - that
   // 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);
-
   SetAllErrors(&checker);
 
   // The policy check is synchronous and needs to pass for the other tests to
   // run.
-  checker.set_policy_check_error(std::string());
+  SetPolicyError(&checker, PreloadCheck::NONE, "");
 
   RunChecker(&checker,
              ExtensionInstallChecker::CHECK_ALL,
              ExtensionInstallChecker::CHECK_REQUIREMENTS);
 
   ExpectRequirementsError(checker);
   ExpectPolicyPass(checker);
   ExpectBlacklistPass(checker);
 }
 
 }  // namespace extensions