Update ExtensionInstallChecker to use PreloadCheck classes

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..599cdff3df2447122beaf056ec11bcb799bb7cec 100644
--- a/chrome/browser/extensions/extension_install_checker_unittest.cc
+++ b/chrome/browser/extensions/extension_install_checker_unittest.cc
@@ -2,19 +2,23 @@
 // 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";
 
@@ -29,23 +33,20 @@ class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
       : 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;
@@ -55,37 +56,35 @@ class ExtensionInstallCheckerForTest : public ExtensionInstallChecker {
     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
@@ -94,25 +93,10 @@ class ExtensionInstallCheckerAsync : public ExtensionInstallCheckerForTest {
  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 {
@@ -144,10 +128,39 @@ class ExtensionInstallCheckerTest : public testing::Test {
   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);
   }
 
@@ -180,21 +193,19 @@ class ExtensionInstallCheckerTest : public testing::Test {
   }
 
   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());
   }
@@ -218,6 +229,7 @@ class ExtensionInstallCheckerTest : public testing::Test {
   }
 
   void DoRunAllChecksPass(ExtensionInstallCheckerForTest* checker) {
+    SetAllPass(checker);
     RunChecker(checker,
                ExtensionInstallChecker::CHECK_ALL,
                0);
@@ -343,12 +355,11 @@ TEST_F(ExtensionInstallCheckerTest, FailFastAsync) {
   // 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,