Switch CupsPrintersManager::GetPrinter to returning an optional

Previously, we were constructing and returning a unique_ptr solely for
the purpose of being able to return a nullptr if a printer was not found.
Instead, let's just return an optional which makes more sense for this.

Change-Id: I14db20b6f7ffc8855ee48de2eda622fb78ec6b57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1572263
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652894}

chrome/browser/chromeos/arc/print/arc_print_service.cc

@@ -238,21 +238,20 @@ class PrinterDiscoverySessionHostImpl
   }
 
   void StartPrinterStateTracking(const std::string& printer_id) override {
-    std::unique_ptr<chromeos::Printer> printer =
+    base::Optional<chromeos::Printer> printer =
         printers_manager_->GetPrinter(printer_id);
     if (!printer) {
       RemovePrinter(printer_id);
       return;
     }
     if (printers_manager_->IsPrinterInstalled(*printer)) {
-      PrinterInstalled(std::move(printer), chromeos::kSuccess);
+      PrinterInstalled(*printer, chromeos::kSuccess);
       return;
     }
-    const chromeos::Printer& printer_ref = *printer;
     configurer_->SetUpPrinter(
-        printer_ref,
+        *printer,
         base::BindOnce(&PrinterDiscoverySessionHostImpl::PrinterInstalled,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(printer)));
+                       weak_ptr_factory_.GetWeakPtr(), *printer));
   }
 
   void StopPrinterStateTracking(const std::string& printer_id) override {
@@ -281,19 +280,19 @@ class PrinterDiscoverySessionHostImpl
   }
 
   // Fetch capabilities for newly installed printer.
-  void PrinterInstalled(std::unique_ptr<chromeos::Printer> printer,
+  void PrinterInstalled(const chromeos::Printer& printer,
                         chromeos::PrinterSetupResult result) {
     if (result != chromeos::kSuccess) {
-      RemovePrinter(printer->id());
+      RemovePrinter(printer.id());
       return;
     }
-    printers_manager_->PrinterInstalled(*printer, true /*is_automatic*/);
-    const std::string& printer_id = printer->id();
+    printers_manager_->PrinterInstalled(printer, true /*is_automatic*/);
+    const std::string& printer_id = printer.id();
     base::PostTaskWithTraitsAndReplyWithResult(
         FROM_HERE, {base::MayBlock()},
         base::BindOnce(&FetchCapabilitiesOnBlockingTaskRunner, printer_id),
         base::BindOnce(&PrinterDiscoverySessionHostImpl::CapabilitiesReceived,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(printer)));
+                       weak_ptr_factory_.GetWeakPtr(), printer));
   }
 
   // Remove from the list of available printers.
@@ -303,14 +302,14 @@ class PrinterDiscoverySessionHostImpl
 
   // Transform printer capabilities to mojo type and send to container.
   void CapabilitiesReceived(
-      std::unique_ptr<chromeos::Printer> printer,
+      const chromeos::Printer& printer,
       std::unique_ptr<printing::PrinterSemanticCapsAndDefaults> caps) {
     if (!caps) {
-      RemovePrinter(printer->id());
+      RemovePrinter(printer.id());
       return;
     }
     std::vector<mojom::PrinterInfoPtr> arc_printers;
-    arc_printers.emplace_back(ToArcPrinter(*printer, std::move(caps)));
+    arc_printers.push_back(ToArcPrinter(printer, std::move(caps)));
     instance_->AddPrinters(std::move(arc_printers));
   }
 

chrome/browser/chromeos/printing/cups_printers_manager.cc

@@ -197,7 +197,7 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   // Note this is linear in the number of printers.  If the number of printers
   // gets so large that a linear search is prohibative, we'll have to rethink
   // more than just this function.
-  std::unique_ptr<Printer> GetPrinter(const std::string& id) const override {
+  base::Optional<Printer> GetPrinter(const std::string& id) const override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_);
     if (!native_printers_allowed_.GetValue()) {
       LOG(WARNING) << "UserNativePrintersAllowed is disabled - only searching "
@@ -208,11 +208,11 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
     for (const auto& printer_list : printers_) {
       for (const auto& printer : printer_list) {
         if (printer.id() == id) {
-          return std::make_unique<Printer>(printer);
+          return printer;
         }
       }
     }
-    return std::unique_ptr<Printer>();
+    return base::nullopt;
   }
 
   // SyncedPrintersManager::Observer implementation
@@ -256,13 +256,13 @@ class CupsPrintersManagerImpl : public CupsPrintersManager,
   }
 
  private:
-  std::unique_ptr<Printer> GetEnterprisePrinter(const std::string& id) const {
+  base::Optional<Printer> GetEnterprisePrinter(const std::string& id) const {
     for (const auto& printer : printers_[kEnterprise]) {
       if (printer.id() == id) {
-        return std::make_unique<Printer>(printer);
+        return printer;
       }
     }
-    return nullptr;
+    return base::nullopt;
   }
 
   // Notify observers on the given classes the the relevant lists have changed.

chrome/browser/chromeos/printing/cups_printers_manager.h

@@ -113,8 +113,8 @@ class CupsPrintersManager : public KeyedService {
   virtual bool IsPrinterInstalled(const Printer& printer) const = 0;
 
   // Look for a printer with the given id in any class.  Returns a copy of the
-  // printer if found, null if not found.
-  virtual std::unique_ptr<Printer> GetPrinter(const std::string& id) const = 0;
+  // printer if found, base::nullopt if not found.
+  virtual base::Optional<Printer> GetPrinter(const std::string& id) const = 0;
 
   // Log an event that the user started trying to set up the given printer,
   // but setup was not completed for some reason.

chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc

@@ -475,13 +475,13 @@ TEST_F(CupsPrintersManagerTest, GetPrinter) {
 
   for (const std::string& id :
        {"Saved", "Enterprise", "Discovered", "Automatic"}) {
-    std::unique_ptr<Printer> printer = manager_->GetPrinter(id);
-    ASSERT_NE(printer, nullptr);
+    base::Optional<Printer> printer = manager_->GetPrinter(id);
+    ASSERT_TRUE(printer);
     EXPECT_EQ(printer->id(), id);
   }
 
-  std::unique_ptr<Printer> printer = manager_->GetPrinter("Nope");
-  EXPECT_EQ(printer, nullptr);
+  base::Optional<Printer> printer = manager_->GetPrinter("Nope");
+  EXPECT_FALSE(printer);
 }
 
 // Test that if |UserNativePrintersAllowed| pref is set to false, then
@@ -577,12 +577,13 @@ TEST_F(CupsPrintersManagerTest, GetPrinterUserNativePrintersDisabled) {
   // Diable the use of non-enterprise printers.
   UpdatePolicyValue(prefs::kUserNativePrintersAllowed, false);
 
-  std::unique_ptr<Printer> saved_ptr = manager_->GetPrinter("Saved");
-  EXPECT_EQ(saved_ptr, nullptr);
+  base::Optional<Printer> saved_printer = manager_->GetPrinter("Saved");
+  EXPECT_FALSE(saved_printer);
 
-  std::unique_ptr<Printer> enterprise_ptr = manager_->GetPrinter("Enterprise");
-  ASSERT_NE(enterprise_ptr, nullptr);
-  EXPECT_EQ(enterprise_ptr->id(), "Enterprise");
+  base::Optional<Printer> enterprise_printer =
+      manager_->GetPrinter("Enterprise");
+  ASSERT_TRUE(enterprise_printer);
+  EXPECT_EQ(enterprise_printer->id(), "Enterprise");
 }
 
 }  // namespace

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

@@ -77,19 +77,19 @@ void CapabilitiesFetched(base::Value policies,
   std::move(cb).Run(std::move(printer_info));
 }
 
-void FetchCapabilities(std::unique_ptr<chromeos::Printer> printer,
+void FetchCapabilities(const chromeos::Printer& printer,
                        base::Value policies,
                        LocalPrinterHandlerChromeos::GetCapabilityCallback cb) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  PrinterBasicInfo basic_info = ToBasicInfo(*printer);
-  bool has_secure_protocol = !printer->HasNetworkProtocol() ||
-                             printer->GetProtocol() == chromeos::Printer::kIpps;
+  PrinterBasicInfo basic_info = ToBasicInfo(printer);
+  bool has_secure_protocol = !printer.HasNetworkProtocol() ||
+                             printer.GetProtocol() == chromeos::Printer::kIpps;
 
   // USER_VISIBLE because the result is displayed in the print preview dialog.
   base::PostTaskWithTraitsAndReplyWithResult(
       FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
-      base::BindOnce(&GetSettingsOnBlockingPool, printer->id(), basic_info,
+      base::BindOnce(&GetSettingsOnBlockingPool, printer.id(), basic_info,
                      PrinterSemanticCapsAndDefaults::Papers(),
                      has_secure_protocol, nullptr),
       base::BindOnce(&CapabilitiesFetched, std::move(policies), std::move(cb)));
@@ -181,7 +181,7 @@ void LocalPrinterHandlerChromeos::StartGetCapability(
     GetCapabilityCallback cb) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  std::unique_ptr<chromeos::Printer> printer =
+  base::Optional<chromeos::Printer> printer =
       printers_manager_->GetPrinter(printer_name);
   if (!printer) {
     // If the printer was removed, the lookup will fail.
@@ -197,32 +197,29 @@ void LocalPrinterHandlerChromeos::StartGetCapability(
 
   if (printers_manager_->IsPrinterInstalled(*printer)) {
     // Skip setup if the printer is already installed.
-    HandlePrinterSetup(std::move(printer), std::move(cb), chromeos::kSuccess);
+    HandlePrinterSetup(*printer, std::move(cb), chromeos::kSuccess);
     return;
   }
 
-  const chromeos::Printer& printer_ref = *printer;
   printer_configurer_->SetUpPrinter(
-      printer_ref,
+      *printer,
       base::BindOnce(&LocalPrinterHandlerChromeos::HandlePrinterSetup,
-                     weak_factory_.GetWeakPtr(), std::move(printer),
-                     std::move(cb)));
+                     weak_factory_.GetWeakPtr(), *printer, std::move(cb)));
 }
 
 void LocalPrinterHandlerChromeos::HandlePrinterSetup(
-    std::unique_ptr<chromeos::Printer> printer,
+    const chromeos::Printer& printer,
     GetCapabilityCallback cb,
     chromeos::PrinterSetupResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   switch (result) {
     case chromeos::PrinterSetupResult::kSuccess: {
-      VLOG(1) << "Printer setup successful for " << printer->id()
+      VLOG(1) << "Printer setup successful for " << printer.id()
               << " fetching properties";
-      printers_manager_->PrinterInstalled(*printer, true /*is_automatic*/);
+      printers_manager_->PrinterInstalled(printer, true /*is_automatic*/);
       // fetch settings on the blocking pool and invoke callback.
-      FetchCapabilities(std::move(printer), GetNativePrinterPolicies(),
-                        std::move(cb));
+      FetchCapabilities(printer, GetNativePrinterPolicies(), std::move(cb));
       return;
     }
     case chromeos::PrinterSetupResult::kPpdNotFound:

chrome/browser/ui/webui/print_preview/local_printer_handler_chromeos.h

@@ -65,7 +65,7 @@ class LocalPrinterHandlerChromeos : public PrinterHandler {
   // |profile_|.
   base::Value GetNativePrinterPolicies() const;
 
-  void HandlePrinterSetup(std::unique_ptr<chromeos::Printer> printer,
+  void HandlePrinterSetup(const chromeos::Printer& printer,
                           GetCapabilityCallback cb,
                           chromeos::PrinterSetupResult result);
 

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

@@ -90,15 +90,17 @@ class FakeCupsPrintersManager : public CupsPrintersManager {
     return installed_.contains(printer.id());
   }
 
-  std::unique_ptr<Printer> GetPrinter(const std::string& id) const override {
+  base::Optional<Printer> GetPrinter(const std::string& id) const override {
+    // Search through each class of printers and find a printer with a
+    // matching id.
     for (const std::vector<Printer>& v : printers_) {
       auto iter = std::find_if(
           v.begin(), v.end(), [&id](const Printer& p) { return p.id() == id; });
       if (iter != v.end()) {
-        return std::make_unique<Printer>(*iter);
+        return *iter;
       }
     }
-    return nullptr;
+    return base::nullopt;
   }
 
   // Add |printer| to the corresponding list in |printers_| bases on the given

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

@@ -13,6 +13,7 @@
 #include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -478,7 +479,7 @@ void CupsPrintersHandler::HandleGetPrinterInfo(const base::ListValue* args) {
 }
 
 void CupsPrintersHandler::OnAutoconfQueriedDiscovered(
-    std::unique_ptr<Printer> printer,
+    Printer printer,
     bool success,
     const std::string& make,
     const std::string& model,
@@ -495,9 +496,9 @@ void CupsPrintersHandler::OnAutoconfQueriedDiscovered(
       // manufacturer and model are set with make_and_model because they are
       // derived from make_and_model for compatability and are slated for
       // removal.
-      printer->set_manufacturer(make);
-      printer->set_model(model);
-      printer->set_make_and_model(make_and_model);
+      printer.set_manufacturer(make);
+      printer.set_model(model);
+      printer.set_make_and_model(make_and_model);
       PRINTER_LOG(DEBUG) << "Printer queried for make and model "
                          << make_and_model;
     }
@@ -505,10 +506,11 @@ void CupsPrintersHandler::OnAutoconfQueriedDiscovered(
     // Autoconfig available, use it.
     if (ipp_everywhere) {
       PRINTER_LOG(DEBUG) << "Performing autoconf setup";
-      printer->mutable_ppd_reference()->autoconf = true;
+      printer.mutable_ppd_reference()->autoconf = true;
       printer_configurer_->SetUpPrinter(
-          *printer, base::Bind(&CupsPrintersHandler::OnAddedDiscoveredPrinter,
-                               weak_factory_.GetWeakPtr(), *printer));
+          printer,
+          base::BindOnce(&CupsPrintersHandler::OnAddedDiscoveredPrinter,
+                         weak_factory_.GetWeakPtr(), printer));
       return;
     }
   }
@@ -517,7 +519,7 @@ void CupsPrintersHandler::OnAutoconfQueriedDiscovered(
   // much information as we can about the printer, and ask the user to supply
   // the rest.
   PRINTER_LOG(EVENT) << "Could not query printer.  Fallback to asking the user";
-  FireManuallyAddDiscoveredPrinter(*printer);
+  FireManuallyAddDiscoveredPrinter(printer);
 }
 
 void CupsPrintersHandler::OnAutoconfQueried(
@@ -635,7 +637,7 @@ void CupsPrintersHandler::AddOrReconfigurePrinter(const base::ListValue* args,
     return;
   }
 
-  std::unique_ptr<Printer> existing_printer_object =
+  base::Optional<Printer> existing_printer_object =
       printers_manager_->GetPrinter(printer->id());
   if (existing_printer_object) {
     if (!IsValidUriChange(*existing_printer_object, *printer)) {
@@ -1009,8 +1011,8 @@ void CupsPrintersHandler::HandleAddDiscoveredPrinter(
   CHECK(args->GetString(0, &printer_id));
 
   PRINTER_LOG(USER) << "Adding discovered printer";
-  std::unique_ptr<Printer> printer = printers_manager_->GetPrinter(printer_id);
-  if (printer == nullptr) {
+  base::Optional<Printer> printer = printers_manager_->GetPrinter(printer_id);
+  if (!printer) {
     PRINTER_LOG(ERROR) << "Discovered printer disappeared";
     // Printer disappeared, so we don't have information about it anymore and
     // can't really do much. Fail the add.
@@ -1049,7 +1051,7 @@ void CupsPrintersHandler::HandleAddDiscoveredPrinter(
   }
   endpoint_resolver_->Start(
       address, base::BindOnce(&CupsPrintersHandler::OnIpResolved,
-                              weak_factory_.GetWeakPtr(), std::move(printer)));
+                              weak_factory_.GetWeakPtr(), std::move(*printer)));
 }
 
 void CupsPrintersHandler::HandleGetPrinterPpdManufacturerAndModel(
@@ -1094,32 +1096,32 @@ void CupsPrintersHandler::FireManuallyAddDiscoveredPrinter(
                     *GetCupsPrinterInfo(printer));
 }
 
-void CupsPrintersHandler::OnIpResolved(std::unique_ptr<Printer> printer,
+void CupsPrintersHandler::OnIpResolved(const Printer& printer,
                                        const net::IPEndPoint& endpoint) {
   bool address_resolved = endpoint.address().IsValid();
   UMA_HISTOGRAM_BOOLEAN("Printing.CUPS.AddressResolutionResult",
                         address_resolved);
   if (!address_resolved) {
-    PRINTER_LOG(ERROR) << printer->make_and_model() << " IP Resolution failed";
-    OnAddedDiscoveredPrinter(*printer, PrinterSetupResult::kPrinterUnreachable);
+    PRINTER_LOG(ERROR) << printer.make_and_model() << " IP Resolution failed";
+    OnAddedDiscoveredPrinter(printer, PrinterSetupResult::kPrinterUnreachable);
     return;
   }
 
-  PRINTER_LOG(EVENT) << printer->make_and_model() << " IP Resolution succeeded";
-  std::string resolved_uri = printer->ReplaceHostAndPort(endpoint);
+  PRINTER_LOG(EVENT) << printer.make_and_model() << " IP Resolution succeeded";
+  std::string resolved_uri = printer.ReplaceHostAndPort(endpoint);
 
   if (IsIppUri(resolved_uri)) {
     PRINTER_LOG(EVENT) << "Query printer for IPP attributes";
-    QueryAutoconf(resolved_uri,
-                  base::BindRepeating(
-                      &CupsPrintersHandler::OnAutoconfQueriedDiscovered,
-                      weak_factory_.GetWeakPtr(), base::Passed(&printer)));
+    QueryAutoconf(
+        resolved_uri,
+        base::BindRepeating(&CupsPrintersHandler::OnAutoconfQueriedDiscovered,
+                            weak_factory_.GetWeakPtr(), printer));
     return;
   }
 
   PRINTER_LOG(EVENT) << "Request make and model from user";
   // If it's not an IPP printer, the user must choose a PPD.
-  FireManuallyAddDiscoveredPrinter(*printer);
+  FireManuallyAddDiscoveredPrinter(printer);
 }
 
 }  // namespace settings

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

@@ -88,7 +88,7 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
 
   // Handles the callback for HandleGetPrinterInfo for a discovered printer.
   void OnAutoconfQueriedDiscovered(
-      std::unique_ptr<Printer> printer,
+      Printer printer,
       bool success,
       const std::string& make,
       const std::string& model,
@@ -191,8 +191,7 @@ class CupsPrintersHandler : public ::settings::SettingsPageUIHandler,
   // parameters.  See https://crbug.com/835476
   void FireManuallyAddDiscoveredPrinter(const Printer& printer);
 
-  void OnIpResolved(std::unique_ptr<Printer> printer,
-                    const net::IPEndPoint& endpoint);
+  void OnIpResolved(const Printer& printer, const net::IPEndPoint& endpoint);
 
   Profile* profile_;
 

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

@@ -72,8 +72,8 @@ class FakeCupsPrintersManager : public CupsPrintersManager {
     return false;
   }
 
-  std::unique_ptr<Printer> GetPrinter(const std::string& id) const override {
-    return std::make_unique<Printer>();
+  base::Optional<Printer> GetPrinter(const std::string& id) const override {
+    return Printer();
   }
 };