PrinterInstaller refactor

- Changed interface from ipp_t* -> std::string
- Removed virtual abstraction since there are no other implementations
- Updated tests

Bug: chromium:945409
Test: unittests pass
Change-Id: Icd7094c1f3b8f07b0dac295186e0e839ce093478
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1671928Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Luum Habtemariam <luum@chromium.org>
Cr-Commit-Position: refs/heads/master@{#671519}

chrome/services/cups_proxy/printer_installer.cc

@@ -19,49 +19,20 @@
 #include "chromeos/printing/printer_configuration.h"
 
 namespace cups_proxy {
-namespace {
 
-using CupsProxyServiceDelegate = chromeos::printing::CupsProxyServiceDelegate;
-
-class PrinterInstallerImpl : public PrinterInstaller {
- public:
-  explicit PrinterInstallerImpl(
-      base::WeakPtr<CupsProxyServiceDelegate> delegate);
-  ~PrinterInstallerImpl() override = default;
-
-  void InstallPrinterIfNeeded(ipp_t* ipp, InstallPrinterCallback cb) override;
-
- private:
-  void OnInstallPrinter(InstallPrinterCallback cb, bool success);
-
-  void Finish(InstallPrinterCallback cb, InstallPrinterResult res);
-
-  // Service delegate granting access to printing stack dependencies.
-  base::WeakPtr<CupsProxyServiceDelegate> delegate_;
-
-  SEQUENCE_CHECKER(sequence_checker_);
-  base::WeakPtrFactory<PrinterInstallerImpl> weak_factory_;
-};
-
-}  // namespace
-
-PrinterInstallerImpl::PrinterInstallerImpl(
-    base::WeakPtr<CupsProxyServiceDelegate> delegate)
-    : delegate_(std::move(delegate)), weak_factory_(this) {
+PrinterInstaller::PrinterInstaller(
+    base::WeakPtr<chromeos::printing::CupsProxyServiceDelegate> delegate)
+    : delegate_(std::move(delegate)) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
-void PrinterInstallerImpl::InstallPrinterIfNeeded(ipp_t* ipp,
-                                                  InstallPrinterCallback cb) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+PrinterInstaller::~PrinterInstaller() = default;
 
-  auto printer_uuid = GetPrinterId(ipp);
-  if (!printer_uuid) {
-    Finish(std::move(cb), InstallPrinterResult::kNoPrintersFound);
-    return;
-  }
+void PrinterInstaller::InstallPrinter(std::string printer_id,
+                                      InstallPrinterCallback cb) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  auto printer = delegate_->GetPrinter(*printer_uuid);
+  auto printer = delegate_->GetPrinter(printer_id);
   if (!printer) {
     // If the requested printer DNE, we proxy to CUPSd and allow it to
     // handle the error.
@@ -76,14 +47,14 @@ void PrinterInstallerImpl::InstallPrinterIfNeeded(ipp_t* ipp,
 
   // Install printer.
   delegate_->SetupPrinter(
-      *printer, base::BindOnce(&PrinterInstallerImpl::OnInstallPrinter,
+      *printer, base::BindOnce(&PrinterInstaller::OnInstallPrinter,
                                weak_factory_.GetWeakPtr(), std::move(cb)));
 }
 
 // TODO(crbug.com/945409): Test whether we need to call
 // CupsPrintersManager::PrinterInstalled here.
-void PrinterInstallerImpl::OnInstallPrinter(InstallPrinterCallback cb,
-                                            bool success) {
+void PrinterInstaller::OnInstallPrinter(InstallPrinterCallback cb,
+                                        bool success) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   Finish(std::move(cb),
@@ -91,16 +62,10 @@ void PrinterInstallerImpl::OnInstallPrinter(InstallPrinterCallback cb,
                  : InstallPrinterResult::kPrinterInstallationFailure);
 }
 
-void PrinterInstallerImpl::Finish(InstallPrinterCallback cb,
-                                  InstallPrinterResult res) {
+void PrinterInstaller::Finish(InstallPrinterCallback cb,
+                              InstallPrinterResult res) {
   base::SequencedTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::BindOnce(std::move(cb), res));
 }
 
-// static
-std::unique_ptr<PrinterInstaller> PrinterInstaller::Create(
-    base::WeakPtr<CupsProxyServiceDelegate> delegate) {
-  return std::make_unique<PrinterInstallerImpl>(std::move(delegate));
-}
-
 }  // namespace cups_proxy

chrome/services/cups_proxy/printer_installer.h

@@ -8,6 +8,7 @@
 #include <cups/cups.h>
 
 #include <memory>
+#include <string>
 
 #include "base/callback_forward.h"
 #include "base/memory/weak_ptr.h"
@@ -18,9 +19,6 @@ namespace cups_proxy {
 enum class InstallPrinterResult {
   kSuccess = 0,
 
-  // Couldn't find any printers referenced by the incoming request.
-  kNoPrintersFound,
-
   // Referenced printer is unknown to Chrome.
   kUnknownPrinterFound,
   kPrinterInstallationFailure,
@@ -34,16 +32,23 @@ using InstallPrinterCallback = base::OnceCallback<void(InstallPrinterResult)>;
 // sequenced context.
 class PrinterInstaller {
  public:
-  // Factory function.
-  static std::unique_ptr<PrinterInstaller> Create(
+  explicit PrinterInstaller(
       base::WeakPtr<chromeos::printing::CupsProxyServiceDelegate> delegate);
-
-  virtual ~PrinterInstaller() = default;
+  ~PrinterInstaller();
 
   // Pre-installs any printers required by |ipp| into the CUPS daemon, as
   // needed. |cb| will be run on this instance's sequenced context.
-  virtual void InstallPrinterIfNeeded(ipp_t* ipp,
-                                      InstallPrinterCallback cb) = 0;
+  void InstallPrinter(std::string printer_id, InstallPrinterCallback cb);
+
+ private:
+  void OnInstallPrinter(InstallPrinterCallback cb, bool success);
+  void Finish(InstallPrinterCallback cb, InstallPrinterResult res);
+
+  // Service delegate granting access to printing stack dependencies.
+  base::WeakPtr<chromeos::printing::CupsProxyServiceDelegate> delegate_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+  base::WeakPtrFactory<PrinterInstaller> weak_factory_{this};
 };
 
 }  // namespace cups_proxy

chrome/services/cups_proxy/printer_installer_unittest.cc

@@ -37,6 +37,8 @@ class FakeServiceDelegate
     installed_printers_.insert({printer.id(), false});
   }
 
+  void FailSetupPrinter() { fail_printer_setup_ = true; }
+
   // Service delegate overrides.
   bool IsPrinterInstalled(const Printer& printer) override {
     if (!base::Contains(installed_printers_, printer.id())) {
@@ -57,6 +59,10 @@ class FakeServiceDelegate
   void SetupPrinter(
       const Printer& printer,
       chromeos::printing::PrinterSetupCallback callback) override {
+    if (fail_printer_setup_) {
+      return std::move(callback).Run(false);
+    }
+
     // PrinterInstaller is expected to have checked if |printer| is already
     // installed before trying setup.
     if (IsPrinterInstalled(printer)) {
@@ -70,43 +76,42 @@ class FakeServiceDelegate
 
  private:
   std::map<std::string, bool> installed_printers_;
+
+  // Conditions whether calls to SetupPrinter succeed.
+  bool fail_printer_setup_ = false;
 };
 
 class PrinterInstallerTest : public testing::Test {
  public:
   PrinterInstallerTest() : weak_factory_(this) {
     delegate_ = std::make_unique<FakeServiceDelegate>();
-    printer_installer_ = PrinterInstaller::Create(delegate_->GetWeakPtr());
+    printer_installer_ =
+        std::make_unique<PrinterInstaller>(delegate_->GetWeakPtr());
   }
 
   ~PrinterInstallerTest() override = default;
 
-  bool RunInstallPrinterIfNeeded(ipp_t* ipp, Printer to_install) {
-    bool success = false;
-    printer_installer_->InstallPrinterIfNeeded(
-        ipp, base::BindOnce(&PrinterInstallerTest::OnRunInstallPrinterIfNeeded,
-                            weak_factory_.GetWeakPtr(), &success,
-                            std::move(to_install)));
-    scoped_task_environment_.RunUntilIdle();
-    return success;
+  InstallPrinterResult RunInstallPrinter(std::string printer_id) {
+    InstallPrinterResult ret;
+
+    base::RunLoop run_loop;
+    printer_installer_->InstallPrinter(
+        printer_id, base::BindOnce(&PrinterInstallerTest::OnRunInstallPrinter,
+                                   weak_factory_.GetWeakPtr(),
+                                   run_loop.QuitClosure(), &ret));
+
+    run_loop.Run();
+    return ret;
   }
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
-  void OnRunInstallPrinterIfNeeded(bool* ret,
-                                   Printer to_install,
-                                   InstallPrinterResult result) {
-    if (result != InstallPrinterResult::kSuccess) {
-      return;
-    }
-
-    // If printer wasn't installed, fail.
-    if (!delegate_->IsPrinterInstalled(to_install)) {
-      return;
-    }
-
-    *ret = true;
+  void OnRunInstallPrinter(base::OnceClosure finish_cb,
+                           InstallPrinterResult* ret,
+                           InstallPrinterResult result) {
+    *ret = result;
+    std::move(finish_cb).Run();
   }
 
   // Backend fake driving the PrinterInstaller.
@@ -119,33 +124,23 @@ class PrinterInstallerTest : public testing::Test {
   base::WeakPtrFactory<PrinterInstallerTest> weak_factory_;
 };
 
-// Return a valid ScopedIppPtr that correctly references |id| in
-// the printer-uri field.
-::printing::ScopedIppPtr MakeIppReferencingPrinters(const std::string& id) {
-  ::printing::ScopedIppPtr ret = ::printing::WrapIpp(ippNew());
-
-  std::string uri = "ipp://localhost/printers/" + id;
-  ippAddString(ret.get(), IPP_TAG_PRINTER, IPP_TAG_URI, "printer-uri", NULL,
-               uri.c_str());
-
-  return ret;
-}
-
 // Standard install known printer workflow.
 TEST_F(PrinterInstallerTest, SimpleSanityTest) {
   Printer to_install(kGenericGUID);
   delegate_->AddPrinter(to_install);
 
-  auto ipp = MakeIppReferencingPrinters(to_install.id());
-  EXPECT_TRUE(RunInstallPrinterIfNeeded(ipp.get(), std::move(to_install)));
+  auto ret = RunInstallPrinter(kGenericGUID);
+  EXPECT_EQ(ret, InstallPrinterResult::kSuccess);
+  EXPECT_TRUE(delegate_->IsPrinterInstalled(to_install));
 }
 
 // Should fail to install an unknown(previously unseen) printer.
 TEST_F(PrinterInstallerTest, UnknownPrinter) {
   Printer to_install(kGenericGUID);
 
-  auto ipp = MakeIppReferencingPrinters(to_install.id());
-  EXPECT_FALSE(RunInstallPrinterIfNeeded(ipp.get(), std::move(to_install)));
+  auto ret = RunInstallPrinter(kGenericGUID);
+  EXPECT_EQ(ret, InstallPrinterResult::kUnknownPrinterFound);
+  EXPECT_FALSE(delegate_->IsPrinterInstalled(to_install));
 }
 
 // Ensure we never setup a printer that's already installed.
@@ -153,12 +148,23 @@ TEST_F(PrinterInstallerTest, InstallPrinterTwice) {
   Printer to_install(kGenericGUID);
   delegate_->AddPrinter(to_install);
 
-  auto ipp = MakeIppReferencingPrinters(to_install.id());
-  EXPECT_TRUE(RunInstallPrinterIfNeeded(ipp.get(), to_install));
+  auto ret = RunInstallPrinter(kGenericGUID);
+  EXPECT_EQ(ret, InstallPrinterResult::kSuccess);
 
   // |printer_installer_| should notice printer is already installed and bail
   // out. If it attempts setup, FakeServiceDelegate will fail the request.
-  EXPECT_TRUE(RunInstallPrinterIfNeeded(ipp.get(), to_install));
+  ret = RunInstallPrinter(kGenericGUID);
+  EXPECT_EQ(ret, InstallPrinterResult::kSuccess);
+}
+
+// Checks for correct response to failed SetupPrinter call.
+TEST_F(PrinterInstallerTest, SetupPrinterFailure) {
+  Printer to_install(kGenericGUID);
+  delegate_->AddPrinter(to_install);
+  delegate_->FailSetupPrinter();
+
+  auto ret = RunInstallPrinter(kGenericGUID);
+  EXPECT_EQ(ret, InstallPrinterResult::kPrinterInstallationFailure);
 }
 
 }  // namespace

chrome/services/cups_proxy/public/cpp/cups_util.h

@@ -23,6 +23,7 @@ static const size_t kHttpMaxBufferSize = 2048;
 // If |ipp| refers to a printer, we return the associated printer_id.
 // Note: Expects the printer id to be embedded in the resource field of the
 // 'printer-uri' IPP attribute.
+// TODO(crbug.com/945409): Add testing suite.
 base::Optional<std::string> GetPrinterId(ipp_t* ipp);
 
 }  // namespace cups_proxy