Split CalculatorsPoliciesBinder into a Prefs class and a Settings class.

Refactor is necessary so that we can have more Prefs binders than Settings
binders when we have multi-profile.  Making this change removes the
refcounting logic that we had and allows the two binders to be owned
by different classes.

Bug: chromium:1015949
Change-Id: Icd341c511d31c7a2742bd50343726718262f5dad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1935414Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Commit-Queue: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720758}

chrome/browser/chromeos/BUILD.gn

@@ -2890,6 +2890,7 @@ source_set("unit_tests") {
     "preferences_unittest.cc",
     "printing/automatic_usb_printer_configurer_unittest.cc",
     "printing/bulk_printers_calculator_unittest.cc",
+    "printing/calculators_policies_binder_unittest.cc",
     "printing/cups_printers_manager_unittest.cc",
     "printing/history/mock_print_job_history_service.cc",
     "printing/history/mock_print_job_history_service.h",

chrome/browser/chromeos/printing/calculators_policies_binder.h

@@ -6,14 +6,15 @@
 #define CHROME_BROWSER_CHROMEOS_PRINTING_CALCULATORS_POLICIES_BINDER_H_
 
 #include <memory>
+#include <string>
+#include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator.h"
 
-class Profile;
-
-namespace user_prefs {
-class PrefRegistrySyncable;
-}
+class PrefService;
+class PrefRegistrySimple;
 
 namespace chromeos {
 
@@ -24,18 +25,58 @@ class CrosSettings;
 // profile. All methods must be called from the same sequence (UI).
 class CalculatorsPoliciesBinder {
  public:
-  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  // |settings| is the source of device policies. |profile| is a user profile.
-  static std::unique_ptr<CalculatorsPoliciesBinder> Create(
+  // Binds events from |settings| to the appropriate fields in |calculator|.
+  static std::unique_ptr<CalculatorsPoliciesBinder> DeviceBinder(
       CrosSettings* settings,
-      Profile* profile);
-  virtual ~CalculatorsPoliciesBinder() = default;
+      base::WeakPtr<BulkPrintersCalculator> calculator);
+
+  // Binds events from |profile| to the appropriate fields in |calculator|.
+  static std::unique_ptr<CalculatorsPoliciesBinder> UserBinder(
+      PrefService* prefs,
+      base::WeakPtr<BulkPrintersCalculator> calculator);
+
+  virtual ~CalculatorsPoliciesBinder();
 
  protected:
-  CalculatorsPoliciesBinder() = default;
+  // |access_mode_name| is the name of the access mode policy.  |blacklist_name|
+  // is the name of the blacklist policy.  |whitelist_name| is the name of the
+  // whitelist policy.  |calculator| will receive updates from the bound
+  // policies.
+  CalculatorsPoliciesBinder(const char* access_mode_name,
+                            const char* blacklist_name,
+                            const char* whitelist_name,
+                            base::WeakPtr<BulkPrintersCalculator> calculator);
+
+  // Returns a WeakPtr to the Derived class.
+  base::WeakPtr<CalculatorsPoliciesBinder> GetWeakPtr();
+
+  // Binds |policy_name| to |callback| for each policy name, using the
+  // appropriate preference system.
+  virtual void Bind(const char* policy_name,
+                    base::RepeatingClosure callback) = 0;
+
+  // Returns the access mode integer preference for |name|.
+  virtual int GetAccessMode(const char* name) const = 0;
+
+  // Returns a string list for the prefrence |name|.
+  virtual std::vector<std::string> GetStringList(const char* name) const = 0;
 
  private:
+  // Attaches bindings since they cannot be safely bound in the constructor.
+  void Init();
+
+  void UpdateAccessMode();
+  void UpdateWhitelist();
+  void UpdateBlacklist();
+
+  const char* access_mode_name_;
+  const char* blacklist_name_;
+  const char* whitelist_name_;
+  base::WeakPtr<BulkPrintersCalculator> calculator_;
+  base::WeakPtrFactory<CalculatorsPoliciesBinder> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(CalculatorsPoliciesBinder);
 };
 

chrome/browser/chromeos/printing/calculators_policies_binder_unittest.cc

+// Copyright 2019 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/chromeos/printing/calculators_policies_binder.h"
+
+#include <array>
+#include <string>
+
+#include "base/test/task_environment.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/scoped_testing_cros_settings.h"
+#include "chrome/browser/chromeos/settings/stub_cros_settings_provider.h"
+#include "chrome/common/pref_names.h"
+#include "components/prefs/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace {
+
+constexpr size_t kNumPrinters = 5;
+
+constexpr size_t kWhitelistPrinters = 4;
+constexpr std::array<const char*, kWhitelistPrinters> kWhitelistIds = {
+    "First", "Second", "Third", "Fifth"};
+
+constexpr std::array<const char*, 3> kBlacklistIds = {"Second", "Third",
+                                                      "Fourth"};
+// kNumPrinters - kBlacklistIds.size() = kBlacklistPrinters (2)
+constexpr size_t kBlacklistPrinters = 2;
+
+constexpr char kBulkPolicyContentsJson[] = R"json(
+[
+  {
+    "id": "First",
+    "display_name": "LexaPrint",
+    "description": "Laser on the test shelf",
+    "manufacturer": "LexaPrint, Inc.",
+    "model": "MS610de",
+    "uri": "ipp://192.168.1.5",
+    "ppd_resource": {
+      "effective_model": "MS610de"
+    }
+  }, {
+    "id": "Second",
+    "display_name": "Color Laser",
+    "description": "The printer next to the water cooler.",
+    "manufacturer": "Printer Manufacturer",
+    "model":"Color Laser 2004",
+    "uri":"ipps://print-server.intranet.example.com:443/ipp/cl2k4",
+    "uuid":"1c395fdb-5d93-4904-b246-b2c046e79d12",
+    "ppd_resource":{
+      "effective_manufacturer": "MakesPrinters",
+      "effective_model": "ColorLaser2k4"
+    }
+  }, {
+    "id": "Third",
+    "display_name": "YaLP",
+    "description": "Fancy Fancy Fancy",
+    "manufacturer": "LexaPrint, Inc.",
+    "model": "MS610de",
+    "uri": "ipp://192.168.1.8",
+    "ppd_resource": {
+      "effective_manufacturer": "LexaPrint",
+      "effective_model": "MS610de"
+    }
+  }, {
+    "id": "Fourth",
+    "display_name": "Yon",
+    "description": "Another printer",
+    "manufacturer": "CrosPrints",
+    "model": "1000d7",
+    "uri": "ipp://192.168.1.9",
+    "ppd_resource": {
+      "effective_manufacturer": "Printer",
+      "effective_model": "Model"
+    }
+  }, {
+    "id": "Fifth",
+    "display_name": "ABCDE",
+    "description": "Yep yep yep",
+    "manufacturer": "Ink and toner",
+    "model": "34343434l",
+    "uri": "ipp://192.168.1.10",
+    "ppd_resource": {
+      "effective_manufacturer": "Blah",
+      "effective_model": "Blah blah Blah"
+    }
+  }
+])json";
+
+template <class Container>
+std::unique_ptr<base::Value> StringsToList(Container container) {
+  auto first = container.begin();
+  auto last = container.end();
+  auto list = std::make_unique<base::Value>(base::Value::Type::LIST);
+
+  while (first != last) {
+    list->Append(*first);
+    first++;
+  }
+  return list;
+}
+
+class CalculatorsPoliciesBinderTest : public testing::Test {
+ protected:
+  void SetUp() override {
+    CalculatorsPoliciesBinder::RegisterProfilePrefs(prefs_.registry());
+  }
+
+  std::unique_ptr<BulkPrintersCalculator> UserCalculator() {
+    auto calculator = BulkPrintersCalculator::Create();
+    binder_ =
+        CalculatorsPoliciesBinder::UserBinder(&prefs_, calculator->AsWeakPtr());
+
+    // Populate data
+    calculator->SetData(std::make_unique<std::string>(kBulkPolicyContentsJson));
+    return calculator;
+  }
+
+  std::unique_ptr<BulkPrintersCalculator> DeviceCalculator() {
+    auto calculator = BulkPrintersCalculator::Create();
+    binder_ = CalculatorsPoliciesBinder::DeviceBinder(CrosSettings::Get(),
+                                                      calculator->AsWeakPtr());
+
+    // Populate data
+    calculator->SetData(std::make_unique<std::string>(kBulkPolicyContentsJson));
+    return calculator;
+  }
+
+  void SetDeviceSetting(const std::string& path, const base::Value& value) {
+    testing_settings_.device_settings()->Set(path, value);
+  }
+
+  base::test::TaskEnvironment env_;
+  ScopedTestingCrosSettings testing_settings_;
+  TestingPrefServiceSimple prefs_;
+
+  // Class under test.  Expected to be destroyed first.
+  std::unique_ptr<CalculatorsPoliciesBinder> binder_;
+};
+
+TEST_F(CalculatorsPoliciesBinderTest, PrefsAllAccess) {
+  auto calculator = UserCalculator();
+
+  // Set prefs to complete computation
+  prefs_.SetManagedPref(prefs::kRecommendedNativePrintersAccessMode,
+                        std::make_unique<base::Value>(
+                            BulkPrintersCalculator::AccessMode::ALL_ACCESS));
+
+  env_.RunUntilIdle();
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kNumPrinters);
+}
+
+TEST_F(CalculatorsPoliciesBinderTest, PrefsWhitelist) {
+  auto calculator = UserCalculator();
+
+  // Set prefs to complete computation
+  prefs_.SetManagedPref(
+      prefs::kRecommendedNativePrintersAccessMode,
+      std::make_unique<base::Value>(
+          BulkPrintersCalculator::AccessMode::WHITELIST_ONLY));
+  prefs_.SetManagedPref(prefs::kRecommendedNativePrintersWhitelist,
+                        StringsToList(kWhitelistIds));
+
+  env_.RunUntilIdle();
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kWhitelistPrinters);
+}
+
+TEST_F(CalculatorsPoliciesBinderTest, PrefsBlacklist) {
+  auto calculator = UserCalculator();
+
+  // Set prefs to complete computation
+  prefs_.SetManagedPref(
+      prefs::kRecommendedNativePrintersAccessMode,
+      std::make_unique<base::Value>(
+          BulkPrintersCalculator::AccessMode::BLACKLIST_ONLY));
+  prefs_.SetManagedPref(prefs::kRecommendedNativePrintersBlacklist,
+                        StringsToList(kBlacklistIds));
+
+  env_.RunUntilIdle();
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kBlacklistPrinters);
+}
+
+TEST_F(CalculatorsPoliciesBinderTest, SettingsAllAccess) {
+  auto calculator = DeviceCalculator();
+
+  SetDeviceSetting(kDeviceNativePrintersAccessMode,
+                   base::Value(BulkPrintersCalculator::AccessMode::ALL_ACCESS));
+  env_.RunUntilIdle();
+
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kNumPrinters);
+}
+
+TEST_F(CalculatorsPoliciesBinderTest, SettingsWhitelist) {
+  auto calculator = DeviceCalculator();
+
+  SetDeviceSetting(
+      kDeviceNativePrintersAccessMode,
+      base::Value(BulkPrintersCalculator::AccessMode::WHITELIST_ONLY));
+  SetDeviceSetting(kDeviceNativePrintersWhitelist,
+                   *StringsToList(kWhitelistIds));
+
+  env_.RunUntilIdle();
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kWhitelistPrinters);
+}
+
+TEST_F(CalculatorsPoliciesBinderTest, SettingsBlacklist) {
+  auto calculator = DeviceCalculator();
+
+  SetDeviceSetting(
+      kDeviceNativePrintersAccessMode,
+      base::Value(BulkPrintersCalculator::AccessMode::BLACKLIST_ONLY));
+  SetDeviceSetting(kDeviceNativePrintersBlacklist,
+                   *StringsToList(kBlacklistIds));
+
+  env_.RunUntilIdle();
+  EXPECT_TRUE(calculator->IsComplete());
+  EXPECT_EQ(calculator->GetPrinters().size(), kBlacklistPrinters);
+}
+
+}  // namespace
+}  // namespace chromeos

chrome/browser/chromeos/printing/enterprise_printers_provider.cc

@@ -48,11 +48,11 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
     // initialization of pref_change_registrar
     pref_change_registrar_.Init(profile->GetPrefs());
 
-    // Binds instances of BulkPrintersCalculator to policies.
-    policies_binder_ = CalculatorsPoliciesBinder::Create(settings, profile);
     // Get instance of BulkPrintersCalculator for device policies.
     device_printers_ = BulkPrintersCalculatorFactory::Get()->GetForDevice();
     if (device_printers_) {
+      devices_binder_ =
+          CalculatorsPoliciesBinder::DeviceBinder(settings, device_printers_);
       device_printers_->AddObserver(this);
       RecalculateCompleteFlagForDevicePrinters();
     }
@@ -64,6 +64,8 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
       account_id_ = user->GetAccountId();
       user_printers_ =
           BulkPrintersCalculatorFactory::Get()->GetForAccountId(account_id_);
+      profile_binder_ = CalculatorsPoliciesBinder::UserBinder(
+          profile->GetPrefs(), user_printers_);
       user_printers_->AddObserver(this);
       RecalculateCompleteFlagForUserPrinters();
     }
@@ -227,7 +229,8 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
   base::WeakPtr<BulkPrintersCalculator> user_printers_;
 
   // Policies binder (bridge between policies and calculators). Owned.
-  std::unique_ptr<CalculatorsPoliciesBinder> policies_binder_;
+  std::unique_ptr<CalculatorsPoliciesBinder> devices_binder_;
+  std::unique_ptr<CalculatorsPoliciesBinder> profile_binder_;
 
   // Profile (user) settings.
   Profile* profile_;