TestPrintBackend cleanup & enhancements

The TestPrintBackend class has been a minimal implementation of a
PrintBackend that could be used in a limited fashion for focused unit
tests.  It did not behave in a consistent fashion between the various
interfaces; for example, setting a new default printer would not be
reflected when getting PrinterBasicInfo.

The out-of-process print driver feature is intending to use this class
in a more general manner, for use of fake printer drivers in browser
tests.  This makes it desirable to make this class be better behaved.

Introduce unit tests specifically for TestPrintBackend, to provide
confidence for use by larger browser tests.

Ensure that the same mapping of available printers is used across
the PrintBackend APIs, so that calls to get printer info or
capabilities would be guaranteed available for any printer identified
by EnumeratePrinters().  Also ensure that the default printer is kept
consistent when the environment is generated or later modified.

Updated PrintBackend::PrinterBasicInfo class to have a
fully-parameterized constructor and equality operator to assist with
unit tests here as well as in the forthcoming tests for later OOPPD
changes.

Bug: 809738
Change-Id: Ia35e0c99bdb0eba8faa48dfc955d73a816e7d84d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463559
Commit-Queue: Alan Screen <awscreen@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827566}

chrome/browser/printing/print_backend_browsertest.cc

@@ -131,7 +131,10 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, FailWithoutInit) {
   EXPECT_FALSE(default_printer_name.has_value());
 }
 
-IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, GetDefaultPrinterName) {
+// TODO(crbug.com/809738):  Re-enable after the updates for setting up the
+// printer test environment are made to print_backend_service.mojom.
+IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest,
+                       DISABLED_GetDefaultPrinterName) {
   base::Optional<std::string> default_printer_name;
 
   DoInitAndSetupTestData();

printing/BUILD.gn

@@ -322,6 +322,7 @@ static_library("test_support") {
 test("printing_unittests") {
   sources = [
     "backend/mojom/print_backend_mojom_traits_unittest.cc",
+    "backend/test_print_backend_unittest.cc",
     "metafile_skia_unittest.cc",
     "nup_parameters_unittest.cc",
     "page_number_unittest.cc",
@@ -337,6 +338,7 @@ test("printing_unittests") {
   configs += [ "//build/config/compiler:noshadowing" ]
   deps = [
     ":printing",
+    ":test_support",
     "//base/test:test_support",
     "//build:chromeos_buildflags",
     "//mojo/core/test:run_all_unittests",

printing/backend/print_backend.cc

@@ -4,6 +4,9 @@
 
 #include "printing/backend/print_backend.h"
 
+#include <string>
+
+#include "base/memory/scoped_refptr.h"
 #include "build/chromeos_buildflags.h"
 
 namespace {
@@ -17,10 +20,31 @@ namespace printing {
 
 PrinterBasicInfo::PrinterBasicInfo() = default;
 
+PrinterBasicInfo::PrinterBasicInfo(const std::string& printer_name,
+                                   const std::string& display_name,
+                                   const std::string& printer_description,
+                                   int printer_status,
+                                   bool is_default,
+                                   const PrinterBasicInfoOptions& options)
+    : printer_name(printer_name),
+      display_name(display_name),
+      printer_description(printer_description),
+      printer_status(printer_status),
+      is_default(is_default),
+      options(options) {}
+
 PrinterBasicInfo::PrinterBasicInfo(const PrinterBasicInfo& other) = default;
 
 PrinterBasicInfo::~PrinterBasicInfo() = default;
 
+bool PrinterBasicInfo::operator==(const PrinterBasicInfo& other) const {
+  return printer_name == other.printer_name &&
+         display_name == other.display_name &&
+         printer_description == other.printer_description &&
+         printer_status == other.printer_status &&
+         is_default == other.is_default && options == other.options;
+}
+
 #if BUILDFLAG(IS_ASH)
 
 AdvancedCapabilityValue::AdvancedCapabilityValue() = default;

printing/backend/print_backend.h

@@ -35,9 +35,17 @@ using PrinterBasicInfoOptions = std::map<std::string, std::string>;
 
 struct PRINTING_EXPORT PrinterBasicInfo {
   PrinterBasicInfo();
+  PrinterBasicInfo(const std::string& printer_name,
+                   const std::string& display_name,
+                   const std::string& printer_description,
+                   int printer_status,
+                   bool is_default,
+                   const PrinterBasicInfoOptions& options);
   PrinterBasicInfo(const PrinterBasicInfo& other);
   ~PrinterBasicInfo();
 
+  bool operator==(const PrinterBasicInfo& other) const;
+
   // The name of the printer as understood by OS.
   std::string printer_name;
 

printing/backend/test_print_backend.cc

@@ -5,8 +5,11 @@
 #include "printing/backend/test_print_backend.h"
 
 #include <memory>
+#include <string>
 #include <utility>
 
+#include "base/check.h"
+#include "base/logging.h"
 #include "base/stl_util.h"
 #include "printing/backend/print_backend.h"
 
@@ -17,11 +20,17 @@ TestPrintBackend::TestPrintBackend() : PrintBackend(/*locale=*/std::string()) {}
 TestPrintBackend::~TestPrintBackend() = default;
 
 bool TestPrintBackend::EnumeratePrinters(PrinterList* printer_list) {
-  // TODO(crbug.com/809738) There was never a call to provide a list of
-  // printers for the environment, so this would always be empty.
-  // This needs to be updated as part of a larger cleanup to make
-  // TestPrintBackend have consistent behavior across its APIs.
-  return false;
+  if (printer_map_.empty())
+    return false;
+
+  for (const auto& entry : printer_map_) {
+    const std::unique_ptr<PrinterData>& data = entry.second;
+
+    // Can only return basic info for printers which have registered info.
+    if (data->info)
+      printer_list->emplace_back(*data->info);
+  }
+  return true;
 }
 
 std::string TestPrintBackend::GetDefaultPrinterName() {
@@ -30,53 +39,97 @@ std::string TestPrintBackend::GetDefaultPrinterName() {
 
 bool TestPrintBackend::GetPrinterBasicInfo(const std::string& printer_name,
                                            PrinterBasicInfo* printer_info) {
-  NOTREACHED() << "Not implemented";
-  return false;
+  auto found = printer_map_.find(printer_name);
+  if (found == printer_map_.end())
+    return false;  // Matching entry not found.
+
+  // Basic info might not have been provided.
+  const std::unique_ptr<PrinterData>& data = found->second;
+  if (!data->info)
+    return false;
+
+  *printer_info = *data->info;
+  return true;
 }
 
 bool TestPrintBackend::GetPrinterSemanticCapsAndDefaults(
     const std::string& printer_name,
-    PrinterSemanticCapsAndDefaults* printer_info) {
-  auto found = valid_printers_.find(printer_name);
-  if (found == valid_printers_.end())
+    PrinterSemanticCapsAndDefaults* printer_caps) {
+  auto found = printer_map_.find(printer_name);
+  if (found == printer_map_.end())
     return false;
 
-  const std::unique_ptr<PrinterSemanticCapsAndDefaults>& caps = found->second;
-  if (!caps)
+  // Capabilities might not have been provided.
+  const std::unique_ptr<PrinterData>& data = found->second;
+  if (!data->caps)
     return false;
 
-  *printer_info = *(found->second);
+  *printer_caps = *data->caps;
   return true;
 }
 
 bool TestPrintBackend::GetPrinterCapsAndDefaults(
     const std::string& printer_name,
-    PrinterCapsAndDefaults* printer_info) {
+    PrinterCapsAndDefaults* printer_caps) {
   // not implemented
   return false;
 }
 
 std::string TestPrintBackend::GetPrinterDriverInfo(
-    const std::string& printr_name) {
+    const std::string& printer_name) {
   // not implemented
   return "";
 }
 
 bool TestPrintBackend::IsValidPrinter(const std::string& printer_name) {
-  return base::Contains(valid_printers_, printer_name);
+  return base::Contains(printer_map_, printer_name);
 }
 
 void TestPrintBackend::SetDefaultPrinterName(const std::string& printer_name) {
+  if (default_printer_name_ == printer_name)
+    return;
+
+  auto found = printer_map_.find(printer_name);
+  if (found == printer_map_.end()) {
+    DLOG(ERROR) << "Unable to set an unknown printer as the default.  Unknown "
+                << "printer name: " << printer_name;
+    return;
+  }
+
+  // Previous default printer is no longer the default.
+  const std::unique_ptr<PrinterData>& new_default_data = found->second;
+  if (!default_printer_name_.empty())
+    printer_map_[default_printer_name_]->info->is_default = false;
+
+  // Now update new printer as default.
   default_printer_name_ = printer_name;
+  if (!default_printer_name_.empty())
+    new_default_data->info->is_default = true;
 }
 
 void TestPrintBackend::AddValidPrinter(
     const std::string& printer_name,
     std::unique_ptr<PrinterSemanticCapsAndDefaults> caps,
     std::unique_ptr<PrinterBasicInfo> info) {
-  // TODO(crbug.com/809738) Utilize the extra `info` parameter to improve
-  // TestPrintBackend internal consistency.
-  valid_printers_[printer_name] = std::move(caps);
+  DCHECK(!printer_name.empty());
+
+  const bool is_default = info && info->is_default;
+  printer_map_[printer_name] =
+      std::make_unique<PrinterData>(std::move(caps), std::move(info));
+
+  // Ensure that default settings are honored if more than one is attempted to
+  // be marked as default or if this prior default should no longer be so.
+  if (is_default)
+    SetDefaultPrinterName(printer_name);
+  else if (default_printer_name_ == printer_name)
+    default_printer_name_.clear();
 }
 
+TestPrintBackend::PrinterData::PrinterData(
+    std::unique_ptr<PrinterSemanticCapsAndDefaults> caps,
+    std::unique_ptr<PrinterBasicInfo> info)
+    : caps(std::move(caps)), info(std::move(info)) {}
+
+TestPrintBackend::PrinterData::~PrinterData() = default;
+
 }  // namespace printing

printing/backend/test_print_backend.h

@@ -5,11 +5,10 @@
 #ifndef PRINTING_BACKEND_TEST_PRINT_BACKEND_H_
 #define PRINTING_BACKEND_TEST_PRINT_BACKEND_H_
 
-#include <map>
 #include <memory>
 #include <string>
-#include <vector>
 
+#include "base/containers/flat_map.h"
 #include "printing/backend/print_backend.h"
 
 namespace printing {
@@ -33,17 +32,18 @@ class TestPrintBackend : public PrintBackend {
   std::string GetPrinterDriverInfo(const std::string& printer_name) override;
   bool IsValidPrinter(const std::string& printer_name) override;
 
-  // Set a default printer.  The default is the empty string.
+  // Methods for test setup:
+
+  // Sets a default printer.  The default is the empty string.
   void SetDefaultPrinterName(const std::string& printer_name);
 
-  // Add a printer to satisfy IsValidPrinter(), EnumeratePrinters(),
+  // Adds a printer to satisfy IsValidPrinter(), EnumeratePrinters(),
   // GetPrinterBasicInfo(), and GetPrinterSemanticCapsAndDefaults().
   // While `caps` can be null, it will cause queries for the capabilities to
   // fail, and thus is likely not of interest for most tests.  IsValidPrinter()
   // will still show true even if `caps` is null, which provides the benefit of
   // simulating a printer that exists in the system but cannot be queried.
-  // `info` can be null, which will result in empty information being provided
-  // for any queries.
+  // `info` can be null, which will result in queries for basic info to fail.
   // Calling EnumeratePrinters() will include the identified `printer_name`
   // even if either parameter is null.
   void AddValidPrinter(const std::string& printer_name,
@@ -54,9 +54,21 @@ class TestPrintBackend : public PrintBackend {
   ~TestPrintBackend() override;
 
  private:
+  struct PrinterData {
+    PrinterData(std::unique_ptr<PrinterSemanticCapsAndDefaults> caps,
+                std::unique_ptr<PrinterBasicInfo> info);
+    ~PrinterData();
+
+    std::unique_ptr<PrinterSemanticCapsAndDefaults> caps;
+    std::unique_ptr<PrinterBasicInfo> info;
+  };
+
   std::string default_printer_name_;
-  std::map<std::string, std::unique_ptr<PrinterSemanticCapsAndDefaults>>
-      valid_printers_;
+  // The values in `printer_map_` will not be null.  The use of a pointer to
+  // PrinterData is just a workaround for the deleted copy assignment operator
+  // for the `caps` field, as that prevents the PrinterData container from
+  // being copied into the map.
+  base::flat_map<std::string, std::unique_ptr<PrinterData>> printer_map_;
 };
 
 }  // namespace printing

printing/backend/test_print_backend_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 "printing/backend/test_print_backend.h"
+
+#include <stdint.h>
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/memory/scoped_refptr.h"
+#include "base/stl_util.h"
+#include "base/test/gtest_util.h"
+#include "printing/backend/print_backend.h"
+#include "printing/mojom/print.mojom.h"
+#include "testing/gmock/include/gmock/gmock-matchers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/geometry/size.h"
+
+namespace printing {
+
+namespace {
+
+constexpr char kDefaultPrinterName[] = "default-test-printer";
+constexpr char kAlternatePrinterName[] = "alternate-test-printer";
+constexpr char kNullDataPrinterName[] = "null-data-test-printer";
+constexpr char kInvalidPrinterName[] = "invalid-test-printer";
+
+constexpr int kDefaultPrinterStatus = 0;
+constexpr int kAlternatePrinterStatus = 1;
+
+const PrinterBasicInfo kDefaultPrinterInfo(
+    /*printer_name=*/kDefaultPrinterName,
+    /*display_name=*/"default test printer",
+    /*printer_description=*/"Default printer for testing.",
+    /*printer_status=*/kDefaultPrinterStatus,
+    /*is_default=*/true,
+    /*options=*/PrinterBasicInfoOptions{});
+const PrinterBasicInfo kAlternatePrinterInfo(
+    /*printer_name=*/kAlternatePrinterName,
+    /*display_name=*/"alternate test printer",
+    /*printer_description=*/"Alternate printer for testing.",
+    /*printer_status=*/kAlternatePrinterStatus,
+    /*is_default=*/false,
+    /*options=*/PrinterBasicInfoOptions{});
+
+constexpr int32_t kDefaultCopiesMax = 123;
+constexpr int32_t kAlternateCopiesMax = 456;
+
+}  // namespace
+
+class TestPrintBackendTest : public testing::Test {
+ public:
+  void SetUp() override {
+    test_print_backend_ = base::MakeRefCounted<TestPrintBackend>();
+  }
+
+  void AddPrinters() {
+    // Add some printers; only bother to set one capabilities field that will
+    // be paid attention to in the tests as way of knowing it has provided the
+    // real capabilities.
+    auto caps = std::make_unique<PrinterSemanticCapsAndDefaults>();
+    caps->copies_max = kDefaultCopiesMax;
+    test_print_backend_->AddValidPrinter(
+        kDefaultPrinterName, std::move(caps),
+        std::make_unique<PrinterBasicInfo>(kDefaultPrinterInfo));
+
+    caps = std::make_unique<PrinterSemanticCapsAndDefaults>();
+    caps->copies_max = kAlternateCopiesMax;
+    test_print_backend_->AddValidPrinter(
+        kAlternatePrinterName, std::move(caps),
+        std::make_unique<PrinterBasicInfo>(kAlternatePrinterInfo));
+
+    test_print_backend_->AddValidPrinter(kNullDataPrinterName, /*caps=*/nullptr,
+                                         /*info=*/nullptr);
+  }
+
+  // Get the test print backend.
+  TestPrintBackend* GetPrintBackend() const {
+    return test_print_backend_.get();
+  }
+
+ private:
+  scoped_refptr<TestPrintBackend> test_print_backend_;
+};
+
+TEST_F(TestPrintBackendTest, EnumeratePrinters) {
+  const PrinterList kPrinterList{kAlternatePrinterInfo, kDefaultPrinterInfo};
+  PrinterList printer_list;
+
+  // Should return false when there are no printers in the environment.
+  EXPECT_FALSE(GetPrintBackend()->EnumeratePrinters(&printer_list));
+
+  AddPrinters();
+
+  EXPECT_TRUE(GetPrintBackend()->EnumeratePrinters(&printer_list));
+  EXPECT_THAT(printer_list, testing::ContainerEq(kPrinterList));
+}
+
+TEST_F(TestPrintBackendTest, DefaultPrinterName) {
+  // If no printers added then no default.
+  EXPECT_TRUE(GetPrintBackend()->GetDefaultPrinterName().empty());
+
+  // Once printers are available, should be a default.
+  AddPrinters();
+  EXPECT_EQ(GetPrintBackend()->GetDefaultPrinterName(), kDefaultPrinterName);
+
+  // Changing default should be reflected on next query.
+  GetPrintBackend()->SetDefaultPrinterName(kAlternatePrinterName);
+  EXPECT_EQ(GetPrintBackend()->GetDefaultPrinterName(), kAlternatePrinterName);
+
+  // Adding a new printer to environment which is marked as default should
+  // automatically make it the new default.
+  static constexpr char kNewDefaultPrinterName[] = "new-default-test-printer";
+  auto caps = std::make_unique<PrinterSemanticCapsAndDefaults>();
+  auto printer_info = std::make_unique<PrinterBasicInfo>();
+  printer_info->printer_name = kNewDefaultPrinterName;
+  printer_info->is_default = true;
+  GetPrintBackend()->AddValidPrinter(kNewDefaultPrinterName, std::move(caps),
+                                     std::move(printer_info));
+  EXPECT_EQ(GetPrintBackend()->GetDefaultPrinterName(), kNewDefaultPrinterName);
+
+  // Requesting an invalid printer name to be a default should have no effect.
+  GetPrintBackend()->SetDefaultPrinterName(kInvalidPrinterName);
+  EXPECT_EQ(GetPrintBackend()->GetDefaultPrinterName(), kNewDefaultPrinterName);
+
+  // Verify that re-adding a printer that was previously the default with null
+  // basic info results in no default printer anymore.
+  GetPrintBackend()->AddValidPrinter(kNewDefaultPrinterName, /*caps=*/nullptr,
+                                     /*info=*/nullptr);
+  EXPECT_TRUE(GetPrintBackend()->GetDefaultPrinterName().empty());
+}
+
+TEST_F(TestPrintBackendTest, PrinterBasicInfo) {
+  PrinterBasicInfo printer_info;
+
+  AddPrinters();
+
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterBasicInfo(kDefaultPrinterName,
+                                                     &printer_info));
+  EXPECT_EQ(printer_info.printer_name, kDefaultPrinterName);
+  EXPECT_EQ(printer_info.printer_status, kDefaultPrinterStatus);
+  EXPECT_TRUE(printer_info.is_default);
+
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterBasicInfo(kAlternatePrinterName,
+                                                     &printer_info));
+  EXPECT_EQ(printer_info.printer_name, kAlternatePrinterName);
+  EXPECT_EQ(printer_info.printer_status, kAlternatePrinterStatus);
+  EXPECT_FALSE(printer_info.is_default);
+
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterBasicInfo(kInvalidPrinterName,
+                                                      &printer_info));
+
+  // Changing default should be reflected on next query.
+  GetPrintBackend()->SetDefaultPrinterName(kAlternatePrinterName);
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterBasicInfo(kAlternatePrinterName,
+                                                     &printer_info));
+  EXPECT_TRUE(printer_info.is_default);
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterBasicInfo(kDefaultPrinterName,
+                                                     &printer_info));
+  EXPECT_FALSE(printer_info.is_default);
+
+  // Printers added with null basic info fail to get data on a query.
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterBasicInfo(kNullDataPrinterName,
+                                                      &printer_info));
+
+  // Verify that (re)adding a printer with null basic info results in a failure
+  // the next time when trying to get the basic info.
+  GetPrintBackend()->AddValidPrinter(kAlternatePrinterName, /*caps=*/nullptr,
+                                     /*info=*/nullptr);
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterBasicInfo(kAlternatePrinterName,
+                                                      &printer_info));
+}
+
+TEST_F(TestPrintBackendTest, GetPrinterSemanticCapsAndDefaults) {
+  PrinterSemanticCapsAndDefaults caps;
+
+  // Should fail when there are no printers in the environment.
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kDefaultPrinterName, &caps));
+
+  AddPrinters();
+
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kDefaultPrinterName, &caps));
+  EXPECT_EQ(caps.copies_max, kDefaultCopiesMax);
+
+  EXPECT_TRUE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kAlternatePrinterName, &caps));
+  EXPECT_EQ(caps.copies_max, kAlternateCopiesMax);
+
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kInvalidPrinterName, &caps));
+
+  // Printers added with null capabilities fail to get data on a query.
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kNullDataPrinterName, &caps));
+
+  // Verify that (re)adding a printer with null capabilities results in a
+  // failure the next time when trying to get capabilities.
+  GetPrintBackend()->AddValidPrinter(kAlternatePrinterName, /*caps=*/nullptr,
+                                     /*info=*/nullptr);
+  EXPECT_FALSE(GetPrintBackend()->GetPrinterSemanticCapsAndDefaults(
+      kAlternatePrinterName, &caps));
+}
+
+TEST_F(TestPrintBackendTest, IsValidPrinter) {
+  PrinterSemanticCapsAndDefaults caps;
+
+  // Should fail when there are no printers in the environment.
+  EXPECT_FALSE(GetPrintBackend()->IsValidPrinter(kDefaultPrinterName));
+
+  AddPrinters();
+
+  EXPECT_TRUE(GetPrintBackend()->IsValidPrinter(kDefaultPrinterName));
+  EXPECT_TRUE(GetPrintBackend()->IsValidPrinter(kAlternatePrinterName));
+  EXPECT_FALSE(GetPrintBackend()->IsValidPrinter(kInvalidPrinterName));
+
+  // Verify that still shows as valid printer even if basic info and
+  // capabilities were originally null.
+  EXPECT_TRUE(GetPrintBackend()->IsValidPrinter(kNullDataPrinterName));
+
+  // Verify that (re)adding a printer with null info and capabilities still
+  // shows as valid.
+  GetPrintBackend()->AddValidPrinter(kAlternatePrinterName, /*caps=*/nullptr,
+                                     /*info=*/nullptr);
+  EXPECT_TRUE(GetPrintBackend()->IsValidPrinter(kAlternatePrinterName));
+}
+
+}  // namespace printing