Distinguish between printer paper sizes and custom paper sizes.

In printing::PrinterSemanticCapsAndDefaults, add an additional field to
hold |user_defined_papers| and separate it from |papers|. Then the code
that convert printer capabilities to CDD can treat |user_defined_papers|
differently and avoid deduplicating them.

With this, when Macs have custom paper sizes support enabled, all the
custom paper sizes show up in Print Preview. Whereas right now, the
custom paper sizes that match non-custom sizes are missing.

With the implementation changes, the user defined papers are now moved
through the call stack into the |user_defined_papers| field, instead of
being passed by reference and then copied.

Bug: 1062179
Change-Id: I9095f560846bad366999b52cc1a20ff2bf1ab69c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2131179
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755651}

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

@@ -80,8 +80,8 @@ base::Value FetchCapabilitiesAsync(const std::string& device_name,
   }
 
   return GetSettingsOnBlockingTaskRunner(
-      device_name, basic_info, user_defined_papers,
-      /* has_secure_protocol */ false, print_backend);
+      device_name, basic_info, std::move(user_defined_papers),
+      /*has_secure_protocol=*/false, print_backend);
 }
 
 std::string GetDefaultPrinterAsync(const std::string& locale) {

components/printing/browser/printer_capabilities.cc

@@ -95,7 +95,7 @@ void PopulateAdvancedCapsLocalization(
 // an empty dictionary if a dictionary could not be generated.
 base::Value GetPrinterCapabilitiesOnBlockingTaskRunner(
     const std::string& device_name,
-    const PrinterSemanticCapsAndDefaults::Papers& user_defined_papers,
+    PrinterSemanticCapsAndDefaults::Papers user_defined_papers,
     bool has_secure_protocol,
     scoped_refptr<PrintBackend> backend) {
   DCHECK(!device_name.empty());
@@ -126,8 +126,7 @@ base::Value GetPrinterCapabilitiesOnBlockingTaskRunner(
     PopulateAllPaperDisplayNames(&info);
 #endif  // BUILDFLAG(PRINT_MEDIA_L10N_ENABLED)
 
-  info.papers.insert(info.papers.end(), user_defined_papers.begin(),
-                     user_defined_papers.end());
+  info.user_defined_papers = std::move(user_defined_papers);
 
 #if defined(OS_CHROMEOS)
   if (!has_secure_protocol)
@@ -166,7 +165,7 @@ std::string GetUserFriendlyName(const std::string& printer_name) {
 base::Value GetSettingsOnBlockingTaskRunner(
     const std::string& device_name,
     const PrinterBasicInfo& basic_info,
-    const PrinterSemanticCapsAndDefaults::Papers& user_defined_papers,
+    PrinterSemanticCapsAndDefaults::Papers user_defined_papers,
     bool has_secure_protocol,
     scoped_refptr<PrintBackend> print_backend) {
   SCOPED_UMA_HISTOGRAM_TIMER("Printing.PrinterCapabilities");
@@ -193,10 +192,10 @@ base::Value GetSettingsOnBlockingTaskRunner(
 
   base::Value printer_info_capabilities(base::Value::Type::DICTIONARY);
   printer_info_capabilities.SetKey(kPrinter, std::move(printer_info));
-  printer_info_capabilities.SetKey(kSettingCapabilities,
-                                   GetPrinterCapabilitiesOnBlockingTaskRunner(
-                                       device_name, user_defined_papers,
-                                       has_secure_protocol, print_backend));
+  printer_info_capabilities.SetKey(
+      kSettingCapabilities, GetPrinterCapabilitiesOnBlockingTaskRunner(
+                                device_name, std::move(user_defined_papers),
+                                has_secure_protocol, print_backend));
   return printer_info_capabilities;
 }
 

components/printing/browser/printer_capabilities.h

@@ -38,7 +38,7 @@ std::pair<std::string, std::string> GetPrinterNameAndDescription(
 base::Value GetSettingsOnBlockingTaskRunner(
     const std::string& device_name,
     const PrinterBasicInfo& basic_info,
-    const PrinterSemanticCapsAndDefaults::Papers& user_defined_papers,
+    PrinterSemanticCapsAndDefaults::Papers user_defined_papers,
     bool has_secure_protocol,
     scoped_refptr<PrintBackend> print_backend);
 

components/printing/browser/printer_capabilities_unittest.cc

@@ -77,15 +77,15 @@ class PrinterCapabilitiesTest : public testing::Test {
   base::Value GetSettingsOnBlockingTaskRunnerAndWaitForResults(
       const std::string& printer_name,
       const PrinterBasicInfo& basic_info,
-      const PrinterSemanticCapsAndDefaults::Papers& papers) {
+      PrinterSemanticCapsAndDefaults::Papers papers) {
     base::RunLoop run_loop;
     base::Value settings;
 
     base::PostTaskAndReplyWithResult(
         blocking_task_runner_.get(), FROM_HERE,
         base::BindOnce(&GetSettingsOnBlockingTaskRunner, printer_name,
-                       basic_info, papers, /*has_secure_protocol=*/false,
-                       test_backend_),
+                       basic_info, std::move(papers),
+                       /*has_secure_protocol=*/false, test_backend_),
         base::BindOnce(&GetSettingsDone, run_loop.QuitClosure(), &settings));
 
     run_loop.Run();
@@ -109,8 +109,8 @@ TEST_F(PrinterCapabilitiesTest, NonNullForMissingPrinter) {
   PrinterSemanticCapsAndDefaults::Papers no_user_defined_papers;
 
   base::Value settings_dictionary =
-      GetSettingsOnBlockingTaskRunnerAndWaitForResults(printer_name, basic_info,
-                                                       no_user_defined_papers);
+      GetSettingsOnBlockingTaskRunnerAndWaitForResults(
+          printer_name, basic_info, std::move(no_user_defined_papers));
 
   ASSERT_FALSE(settings_dictionary.DictEmpty());
 }
@@ -126,8 +126,8 @@ TEST_F(PrinterCapabilitiesTest, ProvidedCapabilitiesUsed) {
   print_backend()->AddValidPrinter(printer_name, std::move(caps));
 
   base::Value settings_dictionary =
-      GetSettingsOnBlockingTaskRunnerAndWaitForResults(printer_name, basic_info,
-                                                       no_user_defined_papers);
+      GetSettingsOnBlockingTaskRunnerAndWaitForResults(
+          printer_name, basic_info, std::move(no_user_defined_papers));
 
   // Verify settings were created.
   ASSERT_FALSE(settings_dictionary.DictEmpty());
@@ -156,8 +156,8 @@ TEST_F(PrinterCapabilitiesTest, NullCapabilitiesExcluded) {
   print_backend()->AddValidPrinter(printer_name, nullptr);
 
   base::Value settings_dictionary =
-      GetSettingsOnBlockingTaskRunnerAndWaitForResults(printer_name, basic_info,
-                                                       no_user_defined_papers);
+      GetSettingsOnBlockingTaskRunnerAndWaitForResults(
+          printer_name, basic_info, std::move(no_user_defined_papers));
 
   // Verify settings were created.
   ASSERT_FALSE(settings_dictionary.DictEmpty());
@@ -185,8 +185,8 @@ TEST_F(PrinterCapabilitiesTest, UserDefinedPapers) {
   user_defined_papers.push_back({"bar", "vendor", {600, 600}});
 
   base::Value settings_dictionary =
-      GetSettingsOnBlockingTaskRunnerAndWaitForResults(printer_name, basic_info,
-                                                       user_defined_papers);
+      GetSettingsOnBlockingTaskRunnerAndWaitForResults(
+          printer_name, basic_info, std::move(user_defined_papers));
 
   // Verify settings were created.
   ASSERT_FALSE(settings_dictionary.DictEmpty());
@@ -227,8 +227,8 @@ TEST_F(PrinterCapabilitiesTest, HasNotSecureProtocol) {
   print_backend()->AddValidPrinter(printer_name, std::move(caps));
 
   base::Value settings_dictionary =
-      GetSettingsOnBlockingTaskRunnerAndWaitForResults(printer_name, basic_info,
-                                                       no_user_defined_papers);
+      GetSettingsOnBlockingTaskRunnerAndWaitForResults(
+          printer_name, basic_info, std::move(no_user_defined_papers));
 
   // Verify settings were created.
   ASSERT_FALSE(settings_dictionary.DictEmpty());

components/printing/common/cloud_print_cdd_conversion.cc

@@ -58,6 +58,17 @@ printer::TypedValueVendorCapability::ValueType ToCloudValueType(
 }
 #endif  // defined(OS_CHROMEOS)
 
+printer::Media ConvertPaperToMedia(
+    const printing::PrinterSemanticCapsAndDefaults::Paper& paper) {
+  gfx::Size paper_size = paper.size_um;
+  if (paper_size.width() > paper_size.height())
+    paper_size.SetSize(paper_size.height(), paper_size.width());
+  printer::Media new_media(paper.display_name, paper.vendor_id,
+                           paper_size.width(), paper_size.height());
+  new_media.MatchBySize();
+  return new_media;
+}
+
 printer::MediaCapability GetMediaCapabilities(
     const printing::PrinterSemanticCapsAndDefaults& semantic_info) {
   printer::MediaCapability media_capabilities;
@@ -70,12 +81,7 @@ printer::MediaCapability GetMediaCapabilities(
   default_media.MatchBySize();
 
   for (const auto& paper : semantic_info.papers) {
-    gfx::Size paper_size = paper.size_um;
-    if (paper_size.width() > paper_size.height())
-      paper_size.SetSize(paper_size.height(), paper_size.width());
-    printer::Media new_media(paper.display_name, paper.vendor_id,
-                             paper_size.width(), paper_size.height());
-    new_media.MatchBySize();
+    printer::Media new_media = ConvertPaperToMedia(paper);
     if (!new_media.IsValid())
       continue;
 
@@ -90,6 +96,16 @@ printer::MediaCapability GetMediaCapabilities(
   if (!is_default_set && default_media.IsValid())
     media_capabilities.AddDefaultOption(default_media, true);
 
+  // Allow user defined paper sizes to be repeats of existing paper sizes.
+  // Do not allow user defined paper sizes to be the default, for now.
+  // TODO(thestig): Figure out the default paper policy here.
+  for (const auto& paper : semantic_info.user_defined_papers) {
+    printer::Media new_media = ConvertPaperToMedia(paper);
+    if (!new_media.IsValid())
+      continue;
+
+    media_capabilities.AddOption(new_media);
+  }
   return media_capabilities;
 }
 

printing/backend/print_backend.h

@@ -113,6 +113,7 @@ struct PRINTING_EXPORT PrinterSemanticCapsAndDefaults {
   };
   using Papers = std::vector<Paper>;
   Papers papers;
+  Papers user_defined_papers;
   Paper default_paper;
 
   std::vector<gfx::Size> dpis;