Move EnterprisePrintersProvider under CupsPrintersManager

Design prop: go/cups-printers-manager-refactor

This change removes EnterprisePrintersProvider from
SyncedPrintersManager and sources enterprise printers to
CupsPrintersManager directly. SyncedPrintersManager is now
solely responsible for saved printers.

Bug: chromium:1030127
Test: updated tests pass
Change-Id: I0fd78f2414f03b00858c107d0efacff406687658
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2072669
Commit-Queue: Scott Violet <sky@chromium.org>
Auto-Submit: Luum Habtemariam <luum@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Piotr Pawliczek <pawliczek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747317}

chrome/browser/chromeos/BUILD.gn

@@ -3046,6 +3046,7 @@ source_set("unit_tests") {
     "printing/bulk_printers_calculator_unittest.cc",
     "printing/calculators_policies_binder_unittest.cc",
     "printing/cups_printers_manager_unittest.cc",
+    "printing/enterprise_printers_provider_unittest.cc",
     "printing/history/mock_print_job_history_service.cc",
     "printing/history/mock_print_job_history_service.h",
     "printing/history/print_job_database_impl_unittest.cc",

chrome/browser/chromeos/printing/cups_printers_manager.cc

@@ -15,6 +15,7 @@
 #include "base/sequence_checker.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chromeos/printing/automatic_usb_printer_configurer.h"
+#include "chrome/browser/chromeos/printing/enterprise_printers_provider.h"
 #include "chrome/browser/chromeos/printing/ppd_provider_factory.h"
 #include "chrome/browser/chromeos/printing/ppd_resolution_tracker.h"
 #include "chrome/browser/chromeos/printing/print_servers_provider.h"
@@ -28,6 +29,7 @@
 #include "chrome/browser/chromeos/printing/usb_printer_detector.h"
 #include "chrome/browser/chromeos/printing/usb_printer_notification_controller.h"
 #include "chrome/browser/chromeos/printing/zeroconf_printer_detector.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/services/network_config/public/mojom/cros_network_config.mojom.h"
@@ -44,6 +46,7 @@ namespace {
 
 class CupsPrintersManagerImpl
     : public CupsPrintersManager,
+      public EnterprisePrintersProvider::Observer,
       public SyncedPrintersManager::Observer,
       public chromeos::network_config::mojom::CrosNetworkConfigObserver {
  public:
@@ -60,6 +63,7 @@ class CupsPrintersManagerImpl
       std::unique_ptr<UsbPrinterNotificationController>
           usb_notification_controller,
       std::unique_ptr<ServerPrintersProvider> server_printers_provider,
+      std::unique_ptr<EnterprisePrintersProvider> enterprise_printers_provider,
       PrinterEventTracker* event_tracker,
       PrefService* pref_service)
       : synced_printers_manager_(synced_printers_manager),
@@ -72,6 +76,8 @@ class CupsPrintersManagerImpl
                                      this,
                                      usb_notification_controller_.get()),
         server_printers_provider_(std::move(server_printers_provider)),
+        enterprise_printers_provider_(std::move(enterprise_printers_provider)),
+        enterprise_printers_provider_observer_(this),
         event_tracker_(event_tracker) {
     // Add the |auto_usb_printer_configurer_| as an observer.
     AddObserver(&auto_usb_printer_configurer_);
@@ -82,16 +88,15 @@ class CupsPrintersManagerImpl
     remote_cros_network_config_->AddObserver(
         cros_network_config_observer_receiver_.BindNewPipeAndPassRemote());
 
-    // Prime the printer cache with the saved and enterprise printers.
+    // Prime the printer cache with the saved printers.
     printers_.ReplacePrintersInClass(
         PrinterClass::kSaved, synced_printers_manager_->GetSavedPrinters());
     synced_printers_manager_observer_.Add(synced_printers_manager_);
 
-    std::vector<Printer> enterprise_printers;
-    enterprise_printers_are_ready_ =
-        synced_printers_manager_->GetEnterprisePrinters(&enterprise_printers);
-    printers_.ReplacePrintersInClass(PrinterClass::kEnterprise,
-                                     enterprise_printers);
+    // Prime the printer cache with the enterprise printers (observer called
+    // immediately).
+    enterprise_printers_provider_observer_.Add(
+        enterprise_printers_provider_.get());
 
     // Callbacks may ensue immediately when the observer proxies are set up, so
     // these instantiations must come after everything else is initialized.
@@ -226,15 +231,12 @@ class CupsPrintersManagerImpl
     NotifyObservers({PrinterClass::kSaved});
   }
 
-  // SyncedPrintersManager::Observer implementation
-  void OnEnterprisePrintersChanged() override {
+  // EnterprisePrintersProvider::Observer implementation
+  void OnPrintersChanged(bool complete,
+                         const std::vector<Printer>& printers) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
-    std::vector<Printer> enterprise_printers;
-    const bool enterprise_printers_are_ready =
-        synced_printers_manager_->GetEnterprisePrinters(&enterprise_printers);
-    if (enterprise_printers_are_ready) {
-      printers_.ReplacePrintersInClass(PrinterClass::kEnterprise,
-                                       std::move(enterprise_printers));
+    if (complete) {
+      printers_.ReplacePrintersInClass(PrinterClass::kEnterprise, printers);
       if (!enterprise_printers_are_ready_) {
         enterprise_printers_are_ready_ = true;
         for (auto& observer : observer_list_) {
@@ -529,6 +531,11 @@ class CupsPrintersManagerImpl
 
   std::unique_ptr<ServerPrintersProvider> server_printers_provider_;
 
+  std::unique_ptr<EnterprisePrintersProvider> enterprise_printers_provider_;
+  ScopedObserver<EnterprisePrintersProvider,
+                 EnterprisePrintersProvider::Observer>
+      enterprise_printers_provider_observer_;
+
   // Not owned
   PrinterEventTracker* const event_tracker_;
 
@@ -571,6 +578,7 @@ std::unique_ptr<CupsPrintersManager> CupsPrintersManager::Create(
       CreatePpdProvider(profile), PrinterConfigurer::Create(profile),
       UsbPrinterNotificationController::Create(profile),
       ServerPrintersProvider::Create(profile),
+      EnterprisePrintersProvider::Create(CrosSettings::Get(), profile),
       PrinterEventTrackerFactory::GetInstance()->GetForBrowserContext(profile),
       profile->GetPrefs());
 }
@@ -585,13 +593,15 @@ std::unique_ptr<CupsPrintersManager> CupsPrintersManager::CreateForTesting(
     std::unique_ptr<UsbPrinterNotificationController>
         usb_notification_controller,
     std::unique_ptr<ServerPrintersProvider> server_printers_provider,
+    std::unique_ptr<EnterprisePrintersProvider> enterprise_printers_provider,
     PrinterEventTracker* event_tracker,
     PrefService* pref_service) {
   return std::make_unique<CupsPrintersManagerImpl>(
       synced_printers_manager, std::move(usb_detector),
       std::move(zeroconf_detector), std::move(ppd_provider),
       std::move(printer_configurer), std::move(usb_notification_controller),
-      std::move(server_printers_provider), event_tracker, pref_service);
+      std::move(server_printers_provider),
+      std::move(enterprise_printers_provider), event_tracker, pref_service);
 }
 
 // static

chrome/browser/chromeos/printing/cups_printers_manager.h

@@ -22,6 +22,7 @@ class PrefRegistrySyncable;
 
 namespace chromeos {
 
+class EnterprisePrintersProvider;
 class PpdProvider;
 class PrinterConfigurer;
 class PrinterDetector;
@@ -65,6 +66,7 @@ class CupsPrintersManager : public PrinterInstallationManager,
       std::unique_ptr<UsbPrinterNotificationController>
           usb_notification_controller,
       std::unique_ptr<ServerPrintersProvider> server_printers_provider,
+      std::unique_ptr<EnterprisePrintersProvider> enterprise_printers_provider,
       PrinterEventTracker* event_tracker,
       PrefService* pref_service);
 
@@ -78,7 +80,7 @@ class CupsPrintersManager : public PrinterInstallationManager,
       PrinterClass printer_class) const = 0;
 
   // Saves |printer|. If |printer| already exists in the saved class, it will
-  // be overwritten.
+  // be overwritten. This is a NOP if |printer| is an enterprise or USB printer.
   virtual void SavePrinter(const Printer& printer) = 0;
 
   // Remove the saved printer with the given id.  This is a NOP if

chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc

@@ -18,6 +18,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "chrome/browser/chromeos/printing/enterprise_printers_provider.h"
 #include "chrome/browser/chromeos/printing/printer_configurer.h"
 #include "chrome/browser/chromeos/printing/printer_event_tracker.h"
 #include "chrome/browser/chromeos/printing/printers_map.h"
@@ -35,8 +36,47 @@
 namespace chromeos {
 namespace {
 
+// Fake backend for EnterprisePrintersProvider.  This allows us to poke
+// arbitrary changes in the enterprise printer lists.
+class FakeEnterprisePrintersProvider : public EnterprisePrintersProvider {
+ public:
+  FakeEnterprisePrintersProvider() = default;
+  ~FakeEnterprisePrintersProvider() override = default;
+
+  // Attach |observer| for notification of events.  |observer| is expected to
+  // live on the same thread (UI) as this object.  OnPrinter* methods are
+  // invoked inline so calling RegisterPrinter in response to OnPrinterAdded is
+  // forbidden.
+  void AddObserver(EnterprisePrintersProvider::Observer* observer) override {
+    observers_.AddObserver(observer);
+  }
+
+  // Remove |observer| so that it no longer receives notifications.  After the
+  // completion of this method, the |observer| can be safely destroyed.
+  void RemoveObserver(EnterprisePrintersProvider::Observer* observer) override {
+    observers_.RemoveObserver(observer);
+  }
+
+  // Fake manipulation functions.
+
+  // Add the given printers to the list of enterprise printers and
+  // notify observers.
+  void AddEnterprisePrinters(const std::vector<Printer>& printers) {
+    enterprise_printers_.insert(enterprise_printers_.end(), printers.begin(),
+                                printers.end());
+    for (Observer& observer : observers_) {
+      observer.OnPrintersChanged(true, enterprise_printers_);
+    }
+  }
+
+ private:
+  base::ObserverList<EnterprisePrintersProvider::Observer>::Unchecked
+      observers_;
+  std::vector<Printer> enterprise_printers_;
+};
+
 // Fake backend for SyncedPrintersManager.  This allows us to poke arbitrary
-// changes in the saved and enterprise printer lists.
+// changes in the saved printer lists.
 class FakeSyncedPrintersManager : public SyncedPrintersManager {
  public:
   FakeSyncedPrintersManager() = default;
@@ -47,13 +87,6 @@ class FakeSyncedPrintersManager : public SyncedPrintersManager {
     return saved_printers_;
   }
 
-  // Returns printers from enterprise policy.
-  bool GetEnterprisePrinters(std::vector<Printer>* printers) const override {
-    if (printers != nullptr)
-      *printers = enterprise_printers_;
-    return true;
-  }
-
   // Attach |observer| for notification of events.  |observer| is expected to
   // live on the same thread (UI) as this object.  OnPrinter* methods are
   // invoked inline so calling RegisterPrinter in response to OnPrinterAdded is
@@ -101,7 +134,7 @@ class FakeSyncedPrintersManager : public SyncedPrintersManager {
   // SyncedPrintersManager.
   PrintersSyncBridge* GetSyncBridge() override { return nullptr; }
   // Returns the printer with id |printer_id|, or nullptr if no such printer
-  // exists.  Searches both Saved and Enterprise printers.
+  // exists.
   std::unique_ptr<Printer> GetPrinter(
       const std::string& printer_id) const override {
     return nullptr;
@@ -124,25 +157,6 @@ class FakeSyncedPrintersManager : public SyncedPrintersManager {
     NotifyOnSavedPrintersObservers();
   }
 
-  // Add the given printers to the list of enterprise printers and
-  // notify observers.
-  void AddEnterprisePrinters(const std::vector<Printer>& printers) {
-    enterprise_printers_.insert(enterprise_printers_.end(), printers.begin(),
-                                printers.end());
-    for (Observer& observer : observers_) {
-      observer.OnEnterprisePrintersChanged();
-    }
-  }
-
-  // Remove the printers with the given ids from the set of enterprise printers,
-  // notify observers.
-  void RemoveEnterprisePrinters(const std::unordered_set<std::string>& ids) {
-    RemovePrinters(ids, &enterprise_printers_);
-    for (Observer& observer : observers_) {
-      observer.OnEnterprisePrintersChanged();
-    }
-  }
-
  private:
   void RemovePrinters(const std::unordered_set<std::string>& ids,
                       std::vector<Printer>* target) {
@@ -179,7 +193,6 @@ class FakeSyncedPrintersManager : public SyncedPrintersManager {
 
   base::ObserverList<SyncedPrintersManager::Observer>::Unchecked observers_;
   std::vector<Printer> saved_printers_;
-  std::vector<Printer> enterprise_printers_;
 };
 
 class FakePrinterDetector : public PrinterDetector {
@@ -347,6 +360,9 @@ class CupsPrintersManagerTest : public testing::Test,
     auto server_printers_provider =
         std::make_unique<FakeServerPrintersProvider>();
     server_printers_provider_ = server_printers_provider.get();
+    auto enterprise_printers_provider =
+        std::make_unique<FakeEnterprisePrintersProvider>();
+    enterprise_printers_provider_ = enterprise_printers_provider.get();
 
     // Register the pref |UserNativePrintersAllowed|
     CupsPrintersManager::RegisterProfilePrefs(pref_service_.registry());
@@ -355,7 +371,9 @@ class CupsPrintersManagerTest : public testing::Test,
         &synced_printers_manager_, std::move(usb_detector),
         std::move(zeroconf_detector), ppd_provider_,
         std::move(printer_configurer), std::move(usb_notif_controller),
-        std::move(server_printers_provider), &event_tracker_, &pref_service_);
+        std::move(server_printers_provider),
+        std::move(enterprise_printers_provider), &event_tracker_,
+        &pref_service_);
     manager_->AddObserver(this);
   }
 
@@ -391,6 +409,7 @@ class CupsPrintersManagerTest : public testing::Test,
 
   // Backend fakes driving the CupsPrintersManager.
   FakeSyncedPrintersManager synced_printers_manager_;
+  FakeEnterprisePrintersProvider* enterprise_printers_provider_;  // Not owned.
   FakePrinterDetector* usb_detector_;          // Not owned.
   FakePrinterDetector* zeroconf_detector_;     // Not owned.
   TestPrinterConfigurer* printer_configurer_;  // Not owned.
@@ -446,7 +465,7 @@ PrinterDetector::DetectedPrinter MakeAutomaticPrinter(const std::string& id) {
 // Test that Enterprise printers from SyncedPrinterManager are
 // surfaced appropriately.
 TEST_F(CupsPrintersManagerTest, GetEnterprisePrinters) {
-  synced_printers_manager_.AddEnterprisePrinters(
+  enterprise_printers_provider_->AddEnterprisePrinters(
       {Printer("Foo"), Printer("Bar")});
   task_environment_.RunUntilIdle();
   ExpectPrintersInClassAre(PrinterClass::kEnterprise, {"Foo", "Bar"});
@@ -576,7 +595,7 @@ TEST_F(CupsPrintersManagerTest, SavePrinter) {
 // a printer is not found.
 TEST_F(CupsPrintersManagerTest, GetPrinter) {
   synced_printers_manager_.AddSavedPrinters({Printer("Saved")});
-  synced_printers_manager_.AddEnterprisePrinters({Printer("Enterprise")});
+  enterprise_printers_provider_->AddEnterprisePrinters({Printer("Enterprise")});
   usb_detector_->AddDetections({MakeDiscoveredPrinter("Discovered")});
   zeroconf_detector_->AddDetections({MakeAutomaticPrinter("Automatic")});
   task_environment_.RunUntilIdle();
@@ -597,7 +616,7 @@ TEST_F(CupsPrintersManagerTest, GetPrinter) {
 // |PrinterClass::kEnterprise|.
 TEST_F(CupsPrintersManagerTest, GetPrintersUserNativePrintersDisabled) {
   synced_printers_manager_.AddSavedPrinters({Printer("Saved")});
-  synced_printers_manager_.AddEnterprisePrinters({Printer("Enterprise")});
+  enterprise_printers_provider_->AddEnterprisePrinters({Printer("Enterprise")});
   task_environment_.RunUntilIdle();
 
   // Disable the use of non-enterprise printers.
@@ -674,7 +693,7 @@ TEST_F(CupsPrintersManagerTest, SavePrinterUserNativePrintersDisabled) {
 // printer. Otherwise, it returns nothing.
 TEST_F(CupsPrintersManagerTest, GetPrinterUserNativePrintersDisabled) {
   synced_printers_manager_.AddSavedPrinters({Printer("Saved")});
-  synced_printers_manager_.AddEnterprisePrinters({Printer("Enterprise")});
+  enterprise_printers_provider_->AddEnterprisePrinters({Printer("Enterprise")});
   task_environment_.RunUntilIdle();
 
   // Sanity check that the printers were added.

chrome/browser/chromeos/printing/enterprise_printers_provider.cc

@@ -40,6 +40,14 @@ std::vector<std::string> ConvertToVector(const base::ListValue* list) {
   return string_list;
 }
 
+void AddPrintersFromMap(
+    const std::unordered_map<std::string, Printer>& printer_map,
+    std::vector<Printer>* printer_list) {
+  for (auto& printer_kv : printer_map) {
+    printer_list->push_back(printer_kv.second);
+  }
+}
+
 class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
                                        public BulkPrintersCalculator::Observer {
  public:
@@ -182,17 +190,23 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
 
   void RecalculateCurrentPrintersList() {
     complete_ = true;
-    printers_ = recommended_printers_;
+    std::vector<Printer> current_printers;
+    AddPrintersFromMap(recommended_printers_, &current_printers);
+
     if (device_printers_) {
       complete_ = complete_ && device_printers_is_complete_;
       const auto& printers = device_printers_->GetPrinters();
-      printers_.insert(printers.begin(), printers.end());
+      AddPrintersFromMap(printers, &current_printers);
     }
     if (user_printers_) {
       complete_ = complete_ && user_printers_is_complete_;
       const auto& printers = user_printers_->GetPrinters();
-      printers_.insert(printers.begin(), printers.end());
+      AddPrintersFromMap(printers, &current_printers);
     }
+
+    // Save current_printers.
+    printers_.swap(current_printers);
+
     for (auto& observer : observers_) {
       observer.OnPrintersChanged(complete_, printers_);
     }
@@ -240,7 +254,7 @@ class EnterprisePrintersProviderImpl : public EnterprisePrintersProvider,
 
   // current final results
   bool complete_ = false;
-  std::unordered_map<std::string, Printer> printers_;
+  std::vector<Printer> printers_;
 
   // Calculators for bulk printers from device and user policies. Unowned.
   base::WeakPtr<BulkPrintersCalculator> device_printers_;

chrome/browser/chromeos/printing/enterprise_printers_provider.h

@@ -33,12 +33,11 @@ class EnterprisePrintersProvider {
    public:
     // |complete| is true if all policies have been parsed and applied (even
     // when parsing errors occurred), false means that a new list of available
-    // printers is being calculated. |printers| contains the current list of
-    // available printers: the map is indexed by printers ids. This
+    // printers is being calculated. |printers| contains the current unordered
+    // list of available printers: the map is indexed by printers ids. This
     // notification is called when value of any of these two parameters changes.
-    virtual void OnPrintersChanged(
-        bool complete,
-        const std::unordered_map<std::string, Printer>& printers) = 0;
+    virtual void OnPrintersChanged(bool complete,
+                                   const std::vector<Printer>& printers) = 0;
   };
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
@@ -49,7 +48,7 @@ class EnterprisePrintersProvider {
       Profile* profile);
   virtual ~EnterprisePrintersProvider() = default;
 
-  // This method also calls directly OnPrintersChanged(...) from |observer|.
+  // This method also directly calls OnPrintersChanged(...) from |observer|.
   virtual void AddObserver(Observer* observer) = 0;
   virtual void RemoveObserver(Observer* observer) = 0;
 

chrome/browser/chromeos/printing/enterprise_printers_provider_unittest.cc

+// Copyright 2020 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/enterprise_printers_provider.h"
+
+#include <algorithm>
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/debug/dump_without_crashing.h"
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "base/strings/string_util.h"
+#include "base/time/time.h"
+#include "chrome/browser/chromeos/printing/bulk_printers_calculator_factory.h"
+#include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
+#include "chrome/browser/chromeos/printing/synced_printers_manager_factory.h"
+#include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/sync/model/fake_model_type_change_processor.h"
+#include "components/sync/model/model_type_store.h"
+#include "components/sync/model/model_type_store_test_util.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+
+namespace {
+
+constexpr char kLexJson[] = R"({
+        "display_name": "LexaPrint",
+        "description": "Laser on the test shelf",
+        "manufacturer": "LexaPrint, Inc.",
+        "model": "MS610de",
+        "uri": "ipp://192.168.1.5",
+        "ppd_resource": {
+          "effective_manufacturer": "LexaPrint",
+          "effective_model": "MS610de",
+        },
+      } )";
+
+constexpr char kColorLaserJson[] = R"json({
+      "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"
+       }
+      })json";
+
+// Helper class to record observed events.
+class LoggingObserver : public EnterprisePrintersProvider::Observer {
+ public:
+  explicit LoggingObserver(EnterprisePrintersProvider* source)
+      : observer_(this) {
+    observer_.Add(source);
+  }
+
+  void OnPrintersChanged(bool complete,
+                         const std::vector<Printer>& printers) override {
+    complete_ = complete;
+    printers_ = printers;
+  }
+
+  const std::vector<Printer>& printers() const { return printers_; }
+
+ private:
+  bool complete_ = false;
+  std::vector<Printer> printers_;
+  ScopedObserver<EnterprisePrintersProvider,
+                 EnterprisePrintersProvider::Observer>
+      observer_;
+};
+
+class EnterprisePrintersProviderTest : public testing::Test {
+ protected:
+  EnterprisePrintersProviderTest()
+      : provider_(EnterprisePrintersProvider::Create(CrosSettings::Get(),
+                                                     &profile_)) {
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void SetPolicyPrinters(const std::vector<std::string>& printer_json_blobs) {
+    auto value = std::make_unique<base::ListValue>();
+    for (const std::string& blob : printer_json_blobs) {
+      value->AppendString(blob);
+    }
+
+    sync_preferences::TestingPrefServiceSyncable* prefs =
+        profile_.GetTestingPrefService();
+    // TestingPrefSyncableService assumes ownership of |value|.
+    prefs->SetManagedPref(prefs::kRecommendedNativePrinters, std::move(value));
+  }
+
+  // Must outlive |profile_|.
+  content::BrowserTaskEnvironment task_environment_;
+
+  // Must outlive |manager_|.
+  TestingProfile profile_;
+
+  BulkPrintersCalculatorFactory bulk_factory;
+  std::unique_ptr<EnterprisePrintersProvider> provider_;
+};
+
+TEST_F(EnterprisePrintersProviderTest, SinglePrinter) {
+  LoggingObserver observer(provider_.get());
+  SetPolicyPrinters({kLexJson});
+
+  const Printer& from_list = observer.printers().front();
+  EXPECT_EQ("LexaPrint", from_list.display_name());
+  EXPECT_EQ(Printer::Source::SRC_POLICY, from_list.source());
+}
+
+TEST_F(EnterprisePrintersProviderTest, MultiplePrinters) {
+  LoggingObserver observer(provider_.get());
+  SetPolicyPrinters({kLexJson, kColorLaserJson});
+  ASSERT_EQ(2U, observer.printers().size());
+
+  const Printer& from_list = observer.printers().front();
+  EXPECT_EQ(Printer::Source::SRC_POLICY, from_list.source());
+}
+
+TEST_F(EnterprisePrintersProviderTest, ChangingEnterprisePrinter) {
+  LoggingObserver observer(provider_.get());
+  SetPolicyPrinters({kLexJson});
+
+  const Printer& from_list = observer.printers().front();
+  EXPECT_EQ("LexaPrint", from_list.display_name());
+  EXPECT_EQ(Printer::Source::SRC_POLICY, from_list.source());
+
+  SetPolicyPrinters({kColorLaserJson});
+
+  const Printer& from_list2 = observer.printers().front();
+  EXPECT_EQ("Color Laser", from_list2.display_name());
+  EXPECT_EQ(Printer::Source::SRC_POLICY, from_list2.source());
+}
+
+}  // namespace
+}  // namespace chromeos

chrome/browser/chromeos/printing/synced_printers_manager.cc

@@ -18,7 +18,6 @@
 #include "chrome/browser/chromeos/printing/printers_sync_bridge.h"
 #include "chrome/browser/chromeos/printing/specifics_translation.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/printing/printer_configuration.h"
@@ -33,23 +32,17 @@ namespace chromeos {
 namespace {
 
 class SyncedPrintersManagerImpl : public SyncedPrintersManager,
-                                  public PrintersSyncBridge::Observer,
-                                  public EnterprisePrintersProvider::Observer {
+                                  public PrintersSyncBridge::Observer {
  public:
-  SyncedPrintersManagerImpl(Profile* profile,
+  explicit SyncedPrintersManagerImpl(
       std::unique_ptr<PrintersSyncBridge> sync_bridge)
-      : profile_(profile),
-        sync_bridge_(std::move(sync_bridge)),
+      : sync_bridge_(std::move(sync_bridge)),
         observers_(new base::ObserverListThreadSafe<
                    SyncedPrintersManager::Observer>()) {
-    printers_provider_ =
-        EnterprisePrintersProvider::Create(CrosSettings::Get(), profile_);
-    printers_provider_->AddObserver(this);
     sync_bridge_->AddObserver(this);
   }
 
   ~SyncedPrintersManagerImpl() override {
-    printers_provider_->RemoveObserver(this);
     sync_bridge_->RemoveObserver(this);
   }
 
@@ -65,13 +58,6 @@ class SyncedPrintersManagerImpl : public SyncedPrintersManager,
     return printers;
   }
 
-  bool GetEnterprisePrinters(std::vector<Printer>* printers) const override {
-    base::AutoLock l(lock_);
-    if (printers != nullptr)
-      *printers = GetEnterprisePrintersLocked();
-    return enterprise_printers_are_ready_;
-  }
-
   std::unique_ptr<Printer> GetPrinter(
       const std::string& printer_id) const override {
     base::AutoLock l(lock_);
@@ -80,9 +66,6 @@ class SyncedPrintersManagerImpl : public SyncedPrintersManager,
 
   void UpdateSavedPrinter(const Printer& printer) override {
     base::AutoLock l(lock_);
-    if (IsEnterprisePrinter(printer.id())) {
-      return;
-    }
     UpdateSavedPrinterLocked(printer);
   }
 
@@ -106,39 +89,15 @@ class SyncedPrintersManagerImpl : public SyncedPrintersManager,
         FROM_HERE, &SyncedPrintersManager::Observer::OnSavedPrintersChanged);
   }
 
-  // EnterprisePrintersProvider::Observer override
-  void OnPrintersChanged(
-      bool complete,
-      const std::unordered_map<std::string, Printer>& printers) override {
-    // Enterprise printers policy changed.  Update the lists.
-    base::AutoLock l(lock_);
-    enterprise_printers_ = printers;
-    enterprise_printers_are_ready_ = complete;
-    observers_->Notify(
-        FROM_HERE,
-        &SyncedPrintersManager::Observer::OnEnterprisePrintersChanged);
-  }
-
  private:
   std::unique_ptr<Printer> GetPrinterLocked(
       const std::string& printer_id) const {
     lock_.AssertAcquired();
-    // check for a policy printer first
-    auto found = enterprise_printers_.find(printer_id);
-    if (found != enterprise_printers_.end()) {
-      // Copy a printer.
-      return std::make_unique<Printer>(found->second);
-    }
-
     base::Optional<sync_pb::PrinterSpecifics> printer =
         sync_bridge_->GetPrinter(printer_id);
     return printer.has_value() ? SpecificsToPrinter(*printer) : nullptr;
   }
 
-  bool IsEnterprisePrinter(const std::string& printer_id) const {
-    return enterprise_printers_.find(printer_id) != enterprise_printers_.end();
-  }
-
   void UpdateSavedPrinterLocked(const Printer& printer_arg) {
     lock_.AssertAcquired();
     DCHECK_EQ(Printer::SRC_USER_PREFS, printer_arg.source());
@@ -152,31 +111,11 @@ class SyncedPrintersManagerImpl : public SyncedPrintersManager,
     sync_bridge_->UpdatePrinter(PrinterToSpecifics(printer));
   }
 
-  std::vector<Printer> GetEnterprisePrintersLocked() const {
-    lock_.AssertAcquired();
-    std::vector<Printer> ret;
-    ret.reserve(enterprise_printers_.size());
-    for (auto kv : enterprise_printers_) {
-      ret.push_back(kv.second);
-    }
-    return ret;
-  }
-
   mutable base::Lock lock_;
 
-  Profile* profile_;
-
   // The backend for profile printers.
   std::unique_ptr<PrintersSyncBridge> sync_bridge_;
 
-  // The Object that provides updates about enterprise printers.
-  std::unique_ptr<EnterprisePrintersProvider> printers_provider_;
-
-  // Enterprise printers as of the last time we got a policy update.
-  std::unordered_map<std::string, Printer> enterprise_printers_;
-  // This flag is set to true if all enterprise policies were loaded.
-  bool enterprise_printers_are_ready_ = false;
-
   scoped_refptr<base::ObserverListThreadSafe<SyncedPrintersManager::Observer>>
       observers_;
   base::WeakPtrFactory<SyncedPrintersManagerImpl> weak_factory_{this};
@@ -184,18 +123,10 @@ class SyncedPrintersManagerImpl : public SyncedPrintersManager,
 
 }  // namespace
 
-// static
-void SyncedPrintersManager::RegisterProfilePrefs(
-    user_prefs::PrefRegistrySyncable* registry) {
-  EnterprisePrintersProvider::RegisterProfilePrefs(registry);
-}
-
 // static
 std::unique_ptr<SyncedPrintersManager> SyncedPrintersManager::Create(
-    Profile* profile,
     std::unique_ptr<PrintersSyncBridge> sync_bridge) {
-  return std::make_unique<SyncedPrintersManagerImpl>(profile,
-                                                     std::move(sync_bridge));
+  return std::make_unique<SyncedPrintersManagerImpl>(std::move(sync_bridge));
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/synced_printers_manager.h

@@ -15,12 +15,6 @@
 #include "chromeos/printing/printer_translator.h"
 #include "components/keyed_service/core/keyed_service.h"
 
-class Profile;
-
-namespace user_prefs {
-class PrefRegistrySyncable;
-}
-
 namespace chromeos {
 
 class PrintersSyncBridge;
@@ -31,29 +25,22 @@ class PrintersSyncBridge;
 // PrintersSyncBridge.
 //
 // This class is thread-safe.
+// TODO(crbug/1030127): Rename to SavedPrintersManager & remove KeyedService.
+// TODO(crbug/1030127): Remove lock and add a sequence_checker.
 class SyncedPrintersManager : public KeyedService {
  public:
   class Observer {
    public:
     virtual void OnSavedPrintersChanged() = 0;
-    virtual void OnEnterprisePrintersChanged() {}
   };
 
   static std::unique_ptr<SyncedPrintersManager> Create(
-      Profile* profile,
       std::unique_ptr<PrintersSyncBridge> sync_bridge);
   ~SyncedPrintersManager() override = default;
 
-  // Register the printing preferences with the |registry|.
-  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
-
   // Returns the printers that are saved in preferences.
   virtual std::vector<Printer> GetSavedPrinters() const = 0;
 
-  // Replaces given vector with vector of printers from enterprise policy.
-  // Returns true if the enterprise policy was loaded and is valid.
-  virtual bool GetEnterprisePrinters(std::vector<Printer>* printers) const = 0;
-
   // Returns the printer with id |printer_id|, or nullptr if no such printer
   // exists.  Searches both Saved and Enterprise printers.
   virtual std::unique_ptr<Printer> GetPrinter(

chrome/browser/chromeos/printing/synced_printers_manager_factory.cc

@@ -68,8 +68,7 @@ SyncedPrintersManager* SyncedPrintersManagerFactory::BuildServiceInstanceFor(
           base::BindRepeating(&syncer::ReportUnrecoverableError,
                               chrome::GetChannel()));
 
-  return SyncedPrintersManager::Create(profile, std::move(sync_bridge))
-      .release();
+  return SyncedPrintersManager::Create(std::move(sync_bridge)).release();
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/synced_printers_manager_unittest.cc

@@ -35,31 +35,6 @@ constexpr char kTestPrinterId[] = "UUID-UUID-UUID-PRINTER";
 constexpr char kTestPrinterId2[] = "UUID-UUID-UUID-PRINTR2";
 constexpr char kTestUri[] = "ipps://printer.chromium.org/ipp/print";
 
-constexpr char kLexJson[] = R"({
-        "display_name": "LexaPrint",
-        "description": "Laser on the test shelf",
-        "manufacturer": "LexaPrint, Inc.",
-        "model": "MS610de",
-        "uri": "ipp://192.168.1.5",
-        "ppd_resource": {
-          "effective_manufacturer": "LexaPrint",
-          "effective_model": "MS610de",
-        },
-      } )";
-
-constexpr char kColorLaserJson[] = R"json({
-      "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"
-       }
-      })json";
-
 // Helper class to record observed events.
 class LoggingObserver : public SyncedPrintersManager::Observer {
  public:
@@ -72,18 +47,10 @@ class LoggingObserver : public SyncedPrintersManager::Observer {
     saved_printers_ = manager_->GetSavedPrinters();
   }
 
-  void OnEnterprisePrintersChanged() override {
-    manager_->GetEnterprisePrinters(&enterprise_printers_);
-  }
-
   const std::vector<Printer>& saved_printers() const { return saved_printers_; }
-  const std::vector<Printer>& enterprise_printers() const {
-    return enterprise_printers_;
-  }
 
  private:
   std::vector<Printer> saved_printers_;
-  std::vector<Printer> enterprise_printers_;
   ScopedObserver<SyncedPrintersManager, SyncedPrintersManager::Observer>
       observer_;
   SyncedPrintersManager* manager_;
@@ -93,7 +60,6 @@ class SyncedPrintersManagerTest : public testing::Test {
  protected:
   SyncedPrintersManagerTest()
       : manager_(SyncedPrintersManager::Create(
-            &profile_,
             std::make_unique<PrintersSyncBridge>(
                 syncer::ModelTypeStoreTestUtil::
                     FactoryForInMemoryStoreForTest(),
@@ -105,12 +71,6 @@ class SyncedPrintersManagerTest : public testing::Test {
   // Must outlive |profile_|.
   content::BrowserTaskEnvironment task_environment_;
 
-  // Must outlive |manager_|.
-  TestingProfile profile_;
-
-  // TODO(https://crbug.com/1030127): Remove this dependency after enterprise
-  // printers are removed from this class.
-  BulkPrintersCalculatorFactory bulk_factory_;
   std::unique_ptr<SyncedPrintersManager> manager_;
 };
 
@@ -186,67 +146,9 @@ TEST_F(SyncedPrintersManagerTest, RemovePrinter) {
   EXPECT_NE(kTestPrinterId, printers.at(1).id());
 }
 
-// Tests for policy printers
-
-TEST_F(SyncedPrintersManagerTest, EnterprisePrinters) {
-  std::string first_printer = kColorLaserJson;
-  std::string second_printer = kLexJson;
-
-  auto value = std::make_unique<base::ListValue>();
-  value->AppendString(first_printer);
-  value->AppendString(second_printer);
-
-  sync_preferences::TestingPrefServiceSyncable* prefs =
-      profile_.GetTestingPrefService();
-  // TestingPrefSyncableService assumes ownership of |value|.
-  prefs->SetManagedPref(prefs::kRecommendedNativePrinters, std::move(value));
-
-  std::vector<Printer> printers;
-  manager_->GetEnterprisePrinters(&printers);
-  ASSERT_EQ(2U, printers.size());
-  // order not specified
-  // EXPECT_EQ("Color Laser", printers[0].display_name());
-  // EXPECT_EQ("ipp://192.168.1.5", printers[1].uri());
-  EXPECT_EQ(Printer::Source::SRC_POLICY, printers[1].source());
-}
-
-TEST_F(SyncedPrintersManagerTest, GetEnterprisePrinter) {
-  std::string printer = kLexJson;
-  auto value = std::make_unique<base::ListValue>();
-  value->AppendString(printer);
-
-  sync_preferences::TestingPrefServiceSyncable* prefs =
-      profile_.GetTestingPrefService();
-  // TestingPrefSyncableService assumes ownership of |value|.
-  prefs->SetManagedPref(prefs::kRecommendedNativePrinters, std::move(value));
-
-  std::vector<Printer> printers;
-  manager_->GetEnterprisePrinters(&printers);
-
-  const Printer& from_list = printers.front();
-  std::unique_ptr<Printer> retrieved = manager_->GetPrinter(from_list.id());
-
-  EXPECT_EQ(from_list.id(), retrieved->id());
-  EXPECT_EQ("LexaPrint", from_list.display_name());
-  EXPECT_EQ(Printer::Source::SRC_POLICY, from_list.source());
-}
-
 // Test that UpdateSavedPrinter saves a printer if it doesn't appear in the
-// enterprise or saved printer lists.
+// saved printer lists.
 TEST_F(SyncedPrintersManagerTest, UpdateSavedPrinterSavesPrinter) {
-  // Set up an enterprise printer.
-  auto value = std::make_unique<base::ListValue>();
-  value->AppendString(kColorLaserJson);
-
-  sync_preferences::TestingPrefServiceSyncable* prefs =
-      profile_.GetTestingPrefService();
-  prefs->SetManagedPref(prefs::kRecommendedNativePrinters, std::move(value));
-
-  // Figure out the id of the enterprise printer that was just installed.
-  std::vector<Printer> printers;
-  manager_->GetEnterprisePrinters(&printers);
-  std::string enterprise_id = printers.at(0).id();
-
   Printer saved(kTestPrinterId);
 
   // Install |saved| printer.
@@ -255,9 +157,9 @@ TEST_F(SyncedPrintersManagerTest, UpdateSavedPrinterSavesPrinter) {
   ASSERT_TRUE(found_printer);
   EXPECT_TRUE(found_printer->display_name().empty());
 
-  // Saving the enterprise printer should *not* generate a configuration
+  // Saving a printer we know about *should not* generate a configuration
   // update.
-  manager_->UpdateSavedPrinter(Printer(enterprise_id));
+  manager_->UpdateSavedPrinter(*found_printer);
   EXPECT_EQ(1U, manager_->GetSavedPrinters().size());
 
   // Saving a printer we don't know about *should* generate a configuration

chrome/browser/prefs/browser_prefs.cc

@@ -305,8 +305,8 @@
 #include "chrome/browser/chromeos/power/power_metrics_reporter.h"
 #include "chrome/browser/chromeos/preferences.h"
 #include "chrome/browser/chromeos/printing/cups_printers_manager.h"
+#include "chrome/browser/chromeos/printing/enterprise_printers_provider.h"
 #include "chrome/browser/chromeos/printing/history/print_job_history_service.h"
-#include "chrome/browser/chromeos/printing/synced_printers_manager.h"
 #include "chrome/browser/chromeos/release_notes/release_notes_storage.h"
 #include "chrome/browser/chromeos/resource_reporter/resource_reporter.h"
 #include "chrome/browser/chromeos/settings/device_settings_cache.h"
@@ -1022,7 +1022,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry,
   chromeos::quick_unlock::PinStoragePrefs::RegisterProfilePrefs(registry);
   chromeos::Preferences::RegisterProfilePrefs(registry);
   chromeos::PrintJobHistoryService::RegisterProfilePrefs(registry);
-  chromeos::SyncedPrintersManager::RegisterProfilePrefs(registry);
+  chromeos::EnterprisePrintersProvider::RegisterProfilePrefs(registry);
   chromeos::parent_access::ParentAccessService::RegisterProfilePrefs(registry);
   chromeos::quick_answers::prefs::RegisterProfilePrefs(registry);
   chromeos::quick_unlock::RegisterProfilePrefs(registry);