Refactor SyncedPrintersManager.

This change brings SyncedPrintersManager closer to PrinterDetector
style interfaces, and aligns function names/styles with the design doc
for CUPS printers management.  It also removes a bunch of unique_ptr
wrapping of printers that are passed around, since these are not at
all performance critical and the unique_ptr stuff hampers readability
(and leads to subtle bugs when unique_ptrs are both used in a bind and
passed as a parameter at the same time).

Also convert it to the factory-function pattern while I'm mucking with
stuff and clean up the header.


Bug: 742487
Change-Id: Ibc9dd0a2f94dad8cb7a1f4adb1897668800311bf
Reviewed-on: https://chromium-review.googlesource.com/575289Reviewed-by: Sky Malice <skym@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Justin Carlson <justincarlson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488276}

chrome/browser/chromeos/printing/synced_printers_manager.h

@@ -34,78 +34,59 @@ class SyncedPrintersManager : public KeyedService {
  public:
   class Observer {
    public:
-    virtual void OnPrinterAdded(const Printer& printer) = 0;
-    virtual void OnPrinterUpdated(const Printer& printer) = 0;
-    virtual void OnPrinterRemoved(const Printer& printer) = 0;
+    virtual void OnConfiguredPrintersChanged(
+        const std::vector<Printer>& printers) = 0;
+    virtual void OnEnterprisePrintersChanged(
+        const std::vector<Printer>& printers) = 0;
   };
 
-  SyncedPrintersManager(Profile* profile,
-                        std::unique_ptr<PrintersSyncBridge> sync_bridge);
-  ~SyncedPrintersManager() override;
+  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.
-  std::vector<std::unique_ptr<Printer>> GetPrinters() const;
+  virtual std::vector<Printer> GetConfiguredPrinters() const = 0;
 
   // Returns printers from enterprise policy.
-  std::vector<std::unique_ptr<Printer>> GetRecommendedPrinters() const;
+  virtual std::vector<Printer> GetEnterprisePrinters() const = 0;
 
-  // Returns the printer with id |printer_id|, or nullptr if no such
-  // printer exists.
-  std::unique_ptr<Printer> GetPrinter(const std::string& printer_id) const;
+  // Returns the printer with id |printer_id|, or nullptr if no such printer
+  // exists.  Searches both Configured and Enterprise printers.
+  virtual std::unique_ptr<Printer> GetPrinter(
+      const std::string& printer_id) const = 0;
 
   // Adds or updates a printer in profile preferences.  The |printer| is
   // identified by its id field.  Those with an empty id are treated as new
   // printers.
-  void RegisterPrinter(std::unique_ptr<Printer> printer);
+  virtual void UpdateConfiguredPrinter(const Printer& printer) = 0;
 
   // Remove printer from preferences with the id |printer_id|.  Returns true if
   // the printer was successfully removed.
-  bool RemovePrinter(const std::string& printer_id);
+  virtual bool RemoveConfiguredPrinter(const std::string& printer_id) = 0;
 
   // 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(SyncedPrintersManager::Observer* observer);
+  virtual void AddObserver(SyncedPrintersManager::Observer* observer) = 0;
 
   // Remove |observer| so that it no longer receives notifications.  After the
   // completion of this method, the |observer| can be safely destroyed.
-  void RemoveObserver(SyncedPrintersManager::Observer* observer);
+  virtual void RemoveObserver(SyncedPrintersManager::Observer* observer) = 0;
 
   // Returns a ModelTypeSyncBridge for the sync client.
-  PrintersSyncBridge* GetSyncBridge();
+  virtual PrintersSyncBridge* GetSyncBridge() = 0;
 
   // Registers that the printer was installed in CUPS. This is independent of
   // whether a printer is saved in profile preferences.
-  void PrinterInstalled(const Printer& printer);
+  virtual void PrinterInstalled(const Printer& printer) = 0;
 
   // Returns true if |printer| is currently installed in CUPS.
-  bool IsConfigurationCurrent(const Printer& printer) const;
-
- private:
-  // Updates the in-memory recommended printer list.
-  void UpdateRecommendedPrinters();
-
-  Profile* profile_;
-  PrefChangeRegistrar pref_change_registrar_;
-
-  // The backend for profile printers.
-  std::unique_ptr<PrintersSyncBridge> sync_bridge_;
-
-  // Contains the keys for all recommended printers in order so we can return
-  // the list of recommended printers in the order they were received.
-  std::vector<std::string> recommended_printer_ids_;
-  std::map<std::string, std::unique_ptr<Printer>> recommended_printers_;
-
-  // Map of printer ids to installation timestamps.
-  std::map<std::string, base::Time> installed_printer_timestamps_;
-
-  base::ObserverList<Observer> observers_;
-
-  DISALLOW_COPY_AND_ASSIGN(SyncedPrintersManager);
+  virtual bool IsConfigurationCurrent(const Printer& printer) const = 0;
 };
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/synced_printers_manager_factory.cc

@@ -64,7 +64,8 @@ SyncedPrintersManager* SyncedPrintersManagerFactory::BuildServiceInstanceFor(
           store_factory, base::BindRepeating(base::IgnoreResult(
                              &base::debug::DumpWithoutCrashing)));
 
-  return new SyncedPrintersManager(profile, std::move(sync_bridge));
+  return SyncedPrintersManager::Create(profile, std::move(sync_bridge))
+      .release();
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/printing/usb_printer_detector.cc

@@ -261,11 +261,8 @@ class UsbPrinterDetectorImpl : public UsbPrinterDetector,
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     if (result == PrinterSetupResult::kSuccess) {
       if (data->is_new) {
-        // We aren't done with data->printer yet, so we have to copy it instead
-        // of moving it.
-        auto printer_copy = base::MakeUnique<Printer>(*data->printer);
         SyncedPrintersManagerFactory::GetForBrowserContext(profile_)
-            ->RegisterPrinter(std::move(printer_copy));
+            ->UpdateConfiguredPrinter(*data->printer);
       }
       // TODO(justincarlson): If the device was hotplugged, pop a timed
       // notification that says the printer is now available for printing.

chrome/browser/sync/test/integration/printers_helper.cc

@@ -26,7 +26,7 @@ namespace printers_helper {
 
 namespace {
 
-using PrinterList = std::vector<std::unique_ptr<chromeos::Printer>>;
+using PrinterList = std::vector<chromeos::Printer>;
 
 // Returns true if Printer#id, Printer#description, and Printer#uri all match.
 bool PrintersAreMostlyEqual(const chromeos::Printer& left,
@@ -40,16 +40,16 @@ bool ListsContainTheSamePrinters(const PrinterList& list_a,
                                  const PrinterList& list_b) {
   std::unordered_multimap<std::string, const chromeos::Printer*> map_b;
   for (const auto& b : list_b) {
-    map_b.insert({b->id(), b.get()});
+    map_b.insert({b.id(), &b});
   }
 
   for (const auto& a : list_a) {
-    auto range = map_b.equal_range(a->id());
+    auto range = map_b.equal_range(a.id());
 
     auto it = std::find_if(
         range.first, range.second,
         [&a](const std::pair<std::string, const chromeos::Printer*>& entry)
-            -> bool { return PrintersAreMostlyEqual(*a, *(entry.second)); });
+            -> bool { return PrintersAreMostlyEqual(a, *(entry.second)); });
 
     if (it == range.second) {
       // Element in a does not match an element in b. Lists do not contain the
@@ -88,30 +88,30 @@ chromeos::SyncedPrintersManager* GetPrinterStore(
 
 void AddPrinter(chromeos::SyncedPrintersManager* manager,
                 const chromeos::Printer& printer) {
-  manager->RegisterPrinter(base::MakeUnique<chromeos::Printer>(printer));
+  manager->UpdateConfiguredPrinter(printer);
 }
 
 void RemovePrinter(chromeos::SyncedPrintersManager* manager, int index) {
   chromeos::Printer testPrinter(CreateTestPrinter(index));
-  manager->RemovePrinter(testPrinter.id());
+  manager->RemoveConfiguredPrinter(testPrinter.id());
 }
 
 bool EditPrinterDescription(chromeos::SyncedPrintersManager* manager,
                             int index,
                             const std::string& description) {
-  PrinterList printers = manager->GetPrinters();
+  PrinterList printers = manager->GetConfiguredPrinters();
   std::string printer_id = PrinterId(index);
-  auto found = std::find_if(
-      printers.begin(), printers.end(),
-      [&printer_id](const std::unique_ptr<chromeos::Printer>& printer) -> bool {
-        return printer->id() == printer_id;
-      });
+  auto found =
+      std::find_if(printers.begin(), printers.end(),
+                   [&printer_id](const chromeos::Printer& printer) -> bool {
+                     return printer.id() == printer_id;
+                   });
 
   if (found == printers.end())
     return false;
 
-  (*found)->set_description(description);
-  manager->RegisterPrinter(std::move(*found));
+  found->set_description(description);
+  manager->UpdateConfiguredPrinter(*found);
 
   return true;
 }
@@ -139,17 +139,17 @@ chromeos::SyncedPrintersManager* GetPrinterStore(int index) {
 }
 
 int GetVerifierPrinterCount() {
-  return GetVerifierPrinterStore()->GetPrinters().size();
+  return GetVerifierPrinterStore()->GetConfiguredPrinters().size();
 }
 
 int GetPrinterCount(int index) {
-  return GetPrinterStore(index)->GetPrinters().size();
+  return GetPrinterStore(index)->GetConfiguredPrinters().size();
 }
 
 bool AllProfilesContainSamePrinters() {
-  auto reference_printers = GetPrinterStore(0)->GetPrinters();
+  auto reference_printers = GetPrinterStore(0)->GetConfiguredPrinters();
   for (int i = 1; i < test()->num_clients(); ++i) {
-    auto printers = GetPrinterStore(i)->GetPrinters();
+    auto printers = GetPrinterStore(i)->GetConfiguredPrinters();
     if (!ListsContainTheSamePrinters(reference_printers, printers)) {
       VLOG(1) << "Printers in client [" << i << "] don't match client 0";
       return false;
@@ -160,8 +160,9 @@ bool AllProfilesContainSamePrinters() {
 }
 
 bool ProfileContainsSamePrintersAsVerifier(int index) {
-  return ListsContainTheSamePrinters(GetVerifierPrinterStore()->GetPrinters(),
-                                     GetPrinterStore(index)->GetPrinters());
+  return ListsContainTheSamePrinters(
+      GetVerifierPrinterStore()->GetConfiguredPrinters(),
+      GetPrinterStore(index)->GetConfiguredPrinters());
 }
 
 PrintersMatchChecker::PrintersMatchChecker()

chrome/browser/sync/test/integration/two_client_printers_sync_test.cc

@@ -99,7 +99,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, RemoveAndEditPrinters) {
   ASSERT_TRUE(PrintersMatchChecker().Wait());
   EXPECT_EQ(2, GetPrinterCount(0));
   EXPECT_EQ(updated_description,
-            GetPrinterStore(1)->GetPrinters()[0]->description());
+            GetPrinterStore(1)->GetConfiguredPrinters()[0].description());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, ConflictResolution) {
@@ -125,7 +125,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, ConflictResolution) {
   // Conflict resolution shoud run here.
   ASSERT_TRUE(PrintersMatchChecker().Wait());
   // The more recent update should win.
-  EXPECT_EQ(valid_message, GetPrinterStore(1)->GetPrinters()[0]->description());
+  EXPECT_EQ(valid_message,
+            GetPrinterStore(1)->GetConfiguredPrinters()[0].description());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientPrintersSyncTest, SimpleMerge) {

chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc

@@ -52,11 +52,10 @@ printing::PrinterBasicInfo ToBasicInfo(const chromeos::Printer& printer) {
   return basic_info;
 }
 
-void AddPrintersToList(
-    const std::vector<std::unique_ptr<chromeos::Printer>>& printers,
-    PrinterList* list) {
+void AddPrintersToList(const std::vector<chromeos::Printer>& printers,
+                       PrinterList* list) {
   for (const auto& printer : printers) {
-    list->push_back(ToBasicInfo(*printer));
+    list->push_back(ToBasicInfo(printer));
   }
 }
 
@@ -103,8 +102,8 @@ class PrinterBackendProxyChromeos : public PrinterBackendProxy {
 
     if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
             switches::kDisableNativeCups)) {
-      AddPrintersToList(prefs_->GetPrinters(), &printer_list);
-      AddPrintersToList(prefs_->GetRecommendedPrinters(), &printer_list);
+      AddPrintersToList(prefs_->GetConfiguredPrinters(), &printer_list);
+      AddPrintersToList(prefs_->GetEnterprisePrinters(), &printer_list);
     }
 
     content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

@@ -185,15 +185,13 @@ void CupsPrintersHandler::HandleGetCupsPrintersList(
   std::string callback_id;
   CHECK(args->GetString(0, &callback_id));
 
-  std::vector<std::unique_ptr<Printer>> printers =
+  std::vector<Printer> printers =
       SyncedPrintersManagerFactory::GetForBrowserContext(profile_)
-          ->GetPrinters();
+          ->GetConfiguredPrinters();
 
   auto printers_list = base::MakeUnique<base::ListValue>();
-  for (const std::unique_ptr<Printer>& printer : printers) {
-    std::unique_ptr<base::DictionaryValue> printer_info =
-        GetPrinterInfo(*printer.get());
-    printers_list->Append(std::move(printer_info));
+  for (const Printer& printer : printers) {
+    printers_list->Append(GetPrinterInfo(printer));
   }
 
   auto response = base::MakeUnique<base::DictionaryValue>();
@@ -207,10 +205,10 @@ void CupsPrintersHandler::HandleUpdateCupsPrinter(const base::ListValue* args) {
   CHECK(args->GetString(0, &printer_id));
   CHECK(args->GetString(1, &printer_name));
 
-  std::unique_ptr<Printer> printer = base::MakeUnique<Printer>(printer_id);
-  printer->set_display_name(printer_name);
-  SyncedPrintersManagerFactory::GetForBrowserContext(profile_)->RegisterPrinter(
-      std::move(printer));
+  Printer printer(printer_id);
+  printer.set_display_name(printer_name);
+  SyncedPrintersManagerFactory::GetForBrowserContext(profile_)
+      ->UpdateConfiguredPrinter(printer);
 }
 
 void CupsPrintersHandler::HandleRemoveCupsPrinter(const base::ListValue* args) {
@@ -225,7 +223,7 @@ void CupsPrintersHandler::HandleRemoveCupsPrinter(const base::ListValue* args) {
     return;
 
   Printer::PrinterProtocol protocol = printer->GetProtocol();
-  prefs->RemovePrinter(printer_id);
+  prefs->RemoveConfiguredPrinter(printer_id);
 
   DebugDaemonClient* client = DBusThreadManager::Get()->GetDebugDaemonClient();
   client->CupsRemovePrinter(printer_name,
@@ -355,19 +353,19 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
   std::string printer_ppd_path;
   printer_dict->GetString("printerPPDPath", &printer_ppd_path);
 
-  std::unique_ptr<Printer> printer = base::MakeUnique<Printer>(printer_id);
-  printer->set_display_name(printer_name);
-  printer->set_description(printer_description);
-  printer->set_manufacturer(printer_manufacturer);
-  printer->set_model(printer_model);
-  printer->set_uri(printer_uri);
+  Printer printer(printer_id);
+  printer.set_display_name(printer_name);
+  printer.set_description(printer_description);
+  printer.set_manufacturer(printer_manufacturer);
+  printer.set_model(printer_model);
+  printer.set_uri(printer_uri);
 
   bool autoconf = false;
   printer_dict->GetBoolean("printerAutoconf", &autoconf);
 
   // Verify that the printer is autoconf or a valid ppd path is present.
   if (autoconf) {
-    printer->mutable_ppd_reference()->autoconf = true;
+    printer.mutable_ppd_reference()->autoconf = true;
   } else if (!printer_ppd_path.empty()) {
     RecordPpdSource(kUser);
     GURL tmp = net::FilePathToFileURL(base::FilePath(printer_ppd_path));
@@ -376,24 +374,24 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
       OnAddPrinterError();
       return;
     }
-    printer->mutable_ppd_reference()->user_supplied_ppd_url = tmp.spec();
+    printer.mutable_ppd_reference()->user_supplied_ppd_url = tmp.spec();
   } else if (!ppd_manufacturer.empty() && !ppd_model.empty()) {
     RecordPpdSource(kScs);
     // Using the manufacturer and model, get a ppd reference.
     if (!ppd_provider_->GetPpdReference(ppd_manufacturer, ppd_model,
-                                        printer->mutable_ppd_reference())) {
+                                        printer.mutable_ppd_reference())) {
       LOG(ERROR) << "Failed to get ppd reference";
       OnAddPrinterError();
       return;
     }
 
-    if (printer->make_and_model().empty()) {
+    if (printer.make_and_model().empty()) {
       // In lieu of more accurate information, populate the make and model
       // fields with the PPD information.
-      printer->set_manufacturer(ppd_manufacturer);
-      printer->set_model(ppd_model);
+      printer.set_manufacturer(ppd_manufacturer);
+      printer.set_model(ppd_model);
       // PPD Model names are actually make and model.
-      printer->set_make_and_model(ppd_model);
+      printer.set_make_and_model(ppd_model);
     }
   } else {
     // TODO(crbug.com/738514): Support PPD guessing for non-autoconf printers.
@@ -402,28 +400,23 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
         << "A configuration option must have been selected to add a printer";
   }
 
-  // Copy the printer for the configurer.  Ownership needs to be transfered to
-  // the receiver of the callback.
-  const Printer printer_copy = *printer;
   printer_configurer_->SetUpPrinter(
-      printer_copy,
-      base::Bind(&CupsPrintersHandler::OnAddedPrinter,
-                 weak_factory_.GetWeakPtr(), base::Passed(&printer)));
+      printer, base::Bind(&CupsPrintersHandler::OnAddedPrinter,
+                          weak_factory_.GetWeakPtr(), printer));
 }
 
-void CupsPrintersHandler::OnAddedPrinter(std::unique_ptr<Printer> printer,
+void CupsPrintersHandler::OnAddedPrinter(const Printer& printer,
                                          PrinterSetupResult result_code) {
-  std::string printer_name = printer->display_name();
   UMA_HISTOGRAM_ENUMERATION("Printing.CUPS.PrinterSetupResult", result_code,
                             PrinterSetupResult::kMaxValue);
   switch (result_code) {
     case PrinterSetupResult::kSuccess: {
       UMA_HISTOGRAM_ENUMERATION("Printing.CUPS.PrinterAdded",
-                                printer->GetProtocol(), Printer::kProtocolMax);
+                                printer.GetProtocol(), Printer::kProtocolMax);
       auto* manager =
           SyncedPrintersManagerFactory::GetForBrowserContext(profile_);
-      manager->PrinterInstalled(*printer);
-      manager->RegisterPrinter(std::move(printer));
+      manager->PrinterInstalled(printer);
+      manager->UpdateConfiguredPrinter(printer);
       break;
     }
     case PrinterSetupResult::kPpdNotFound:
@@ -452,7 +445,7 @@ void CupsPrintersHandler::OnAddedPrinter(std::unique_ptr<Printer> printer,
   CallJavascriptFunction(
       "cr.webUIListenerCallback", base::Value("on-add-cups-printer"),
       base::Value(result_code == PrinterSetupResult::kSuccess),
-      base::Value(printer_name));
+      base::Value(printer.display_name()));
 }
 
 void CupsPrintersHandler::OnAddPrinterError() {

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.h

@@ -68,8 +68,7 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
                      bool ipp_everywhere);
 
   void HandleAddCupsPrinter(const base::ListValue* args);
-  void OnAddedPrinter(std::unique_ptr<Printer> printer,
-                      PrinterSetupResult result);
+  void OnAddedPrinter(const Printer& printer, PrinterSetupResult result);
   void OnAddPrinterError();
 
   // Get a list of all manufacturers for which we have at least one model of