Unify printer behavior among all platforms.

Move platform-specific code to corresponding components by adding
display_name in PrinterBasicInfo.

Bug: 921093, 947171
Change-Id: I9526b77fcc507b8e2eca54e49cea6b8d764a9a03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1538431
Commit-Queue: Nikita Podguzov <nikitapodguzov@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664182}

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

@@ -47,21 +47,15 @@ using chromeos::PrinterClass;
 // We only support sending username for named users but just in case.
 const char kUsernamePlaceholder[] = "chronos";
 
-// Store the name used in CUPS, Printer#id in |printer_name|, the description
-// as the system_driverinfo option value, and the Printer#display_name in
-// the |printer_description| field.  This will match how Mac OS X presents
-// printer information.
 PrinterBasicInfo ToBasicInfo(const chromeos::Printer& printer) {
   PrinterBasicInfo basic_info;
 
-  // TODO(skau): Unify Mac with the other platforms for display name
-  // presentation so I can remove this strange code.
-  basic_info.options[kDriverInfoTagName] = printer.description();
   basic_info.options[kCUPSEnterprisePrinter] =
       (printer.source() == chromeos::Printer::SRC_POLICY) ? kValueTrue
                                                           : kValueFalse;
   basic_info.printer_name = printer.id();
-  basic_info.printer_description = printer.display_name();
+  basic_info.display_name = printer.display_name();
+  basic_info.printer_description = printer.description();
   return basic_info;
 }
 

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

@@ -59,16 +59,20 @@ void RecordGetCapability(std::unique_ptr<base::Value>* capabilities_out,
   capabilities_out->reset(capability.DeepCopy());
 }
 
-Printer CreateTestPrinter(const std::string& id, const std::string& name) {
+Printer CreateTestPrinter(const std::string& id,
+                          const std::string& name,
+                          const std::string& description) {
   Printer printer;
   printer.set_id(id);
   printer.set_display_name(name);
+  printer.set_description(description);
   return printer;
 }
 
 Printer CreateEnterprisePrinter(const std::string& id,
-                                const std::string& name) {
-  Printer printer = CreateTestPrinter(id, name);
+                                const std::string& name,
+                                const std::string& description) {
+  Printer printer = CreateTestPrinter(id, name, description);
   printer.set_source(Printer::SRC_POLICY);
   return printer;
 }
@@ -152,10 +156,12 @@ TEST_F(LocalPrinterHandlerChromeosTest, GetPrinters) {
   std::unique_ptr<base::ListValue> printers;
   bool is_done = false;
 
-  Printer saved_printer = CreateTestPrinter("printer1", "saved");
+  Printer saved_printer =
+      CreateTestPrinter("printer1", "saved", "description1");
   Printer enterprise_printer =
-      CreateEnterprisePrinter("printer2", "enterprise");
-  Printer automatic_printer = CreateTestPrinter("printer3", "automatic");
+      CreateEnterprisePrinter("printer2", "enterprise", "description2");
+  Printer automatic_printer =
+      CreateTestPrinter("printer3", "automatic", "description3");
 
   printers_manager_.AddPrinter(saved_printer, PrinterClass::kSaved);
   printers_manager_.AddPrinter(enterprise_printer, PrinterClass::kEnterprise);
@@ -174,31 +180,28 @@ TEST_F(LocalPrinterHandlerChromeosTest, GetPrinters) {
       {
         "cupsEnterprisePrinter": false,
         "deviceName": "printer1",
-        "printerDescription": "",
+        "printerDescription": "description1",
         "printerName": "saved",
         "printerOptions": {
-          "cupsEnterprisePrinter": "false",
-          "system_driverinfo": ""
+          "cupsEnterprisePrinter": "false"
         }
       },
       {
         "cupsEnterprisePrinter": true,
         "deviceName": "printer2",
-        "printerDescription": "",
+        "printerDescription": "description2",
         "printerName": "enterprise",
         "printerOptions": {
-          "cupsEnterprisePrinter": "true",
-          "system_driverinfo": ""
+          "cupsEnterprisePrinter": "true"
         }
       },
       {
         "cupsEnterprisePrinter": false,
         "deviceName": "printer3",
-        "printerDescription": "",
+        "printerDescription": "description3",
         "printerName": "automatic",
         "printerOptions": {
-          "cupsEnterprisePrinter": "false",
-          "system_driverinfo": ""
+          "cupsEnterprisePrinter": "false"
         }
       }
     ]
@@ -213,7 +216,8 @@ TEST_F(LocalPrinterHandlerChromeosTest, GetPrinters) {
 // Tests that fetching capabilities for an existing installed printer is
 // successful.
 TEST_F(LocalPrinterHandlerChromeosTest, StartGetCapabilityValidPrinter) {
-  Printer saved_printer = CreateTestPrinter("printer1", "saved");
+  Printer saved_printer =
+      CreateTestPrinter("printer1", "saved", "description1");
   printers_manager_.AddPrinter(saved_printer, PrinterClass::kSaved);
   printers_manager_.InstallPrinter("printer1");
 
@@ -238,7 +242,8 @@ TEST_F(LocalPrinterHandlerChromeosTest, StartGetCapabilityValidPrinter) {
 // Test that printers which have not yet been installed are installed with
 // SetUpPrinter before their capabilities are fetched.
 TEST_F(LocalPrinterHandlerChromeosTest, StartGetCapabilityPrinterNotInstalled) {
-  Printer discovered_printer = CreateTestPrinter("printer1", "discovered");
+  Printer discovered_printer =
+      CreateTestPrinter("printer1", "discovered", "description1");
   // NOTE: The printer |discovered_printer| is not installed using
   // InstallPrinter.
   printers_manager_.AddPrinter(discovered_printer, PrinterClass::kDiscovered);

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

@@ -48,11 +48,9 @@ void PrintersToValues(const PrinterList& printer_list,
     auto printer_info = std::make_unique<base::DictionaryValue>();
     printer_info->SetString(kSettingDeviceName, printer.printer_name);
 
-    const auto printer_name_description = GetPrinterNameAndDescription(printer);
-    const std::string& printer_name = printer_name_description.first;
-    const std::string& printer_description = printer_name_description.second;
-    printer_info->SetString(kSettingPrinterName, printer_name);
-    printer_info->SetString(kSettingPrinterDescription, printer_description);
+    printer_info->SetString(kSettingPrinterName, printer.display_name);
+    printer_info->SetString(kSettingPrinterDescription,
+                            printer.printer_description);
 
     auto options = std::make_unique<base::DictionaryValue>();
     for (const auto opt_it : printer.options)
@@ -67,7 +65,7 @@ void PrintersToValues(const PrinterList& printer_list,
 
     printers->Append(std::move(printer_info));
 
-    VLOG(1) << "Found printer " << printer_name << " with device name "
+    VLOG(1) << "Found printer " << printer.display_name << " with device name "
             << printer.printer_name;
   }
 }

components/printing/browser/printer_capabilities.cc

@@ -118,26 +118,6 @@ std::string GetUserFriendlyName(const std::string& printer_name) {
 }
 #endif
 
-std::pair<std::string, std::string> GetPrinterNameAndDescription(
-    const PrinterBasicInfo& printer) {
-#if defined(OS_MACOSX) || defined(OS_CHROMEOS)
-  // On Mac, |printer.printer_description| specifies the printer name and
-  // |printer.printer_name| specifies the device name / printer queue name.
-  // Chrome OS emulates the Mac behavior.
-  const std::string& real_name = printer.printer_description;
-  std::string real_description;
-  const auto it = printer.options.find(kDriverNameTagName);
-  if (it != printer.options.end())
-    real_description = it->second;
-  return std::make_pair(real_name, real_description);
-#elif defined(OS_WIN)
-  return std::make_pair(GetUserFriendlyName(printer.printer_name),
-                        printer.printer_description);
-#else
-  return std::make_pair(printer.printer_name, printer.printer_description);
-#endif
-}
-
 base::Value GetSettingsOnBlockingPool(
     const std::string& device_name,
     const PrinterBasicInfo& basic_info,
@@ -148,15 +128,12 @@ base::Value GetSettingsOnBlockingPool(
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
 
-  const auto printer_name_description =
-      GetPrinterNameAndDescription(basic_info);
-
   base::Value printer_info(base::Value::Type::DICTIONARY);
   printer_info.SetKey(kSettingDeviceName, base::Value(device_name));
   printer_info.SetKey(kSettingPrinterName,
-                      base::Value(printer_name_description.first));
+                      base::Value(basic_info.display_name));
   printer_info.SetKey(kSettingPrinterDescription,
-                      base::Value(printer_name_description.second));
+                      base::Value(basic_info.printer_description));
   printer_info.SetKey(
       kCUPSEnterprisePrinter,
       base::Value(

printing/BUILD.gn

@@ -320,6 +320,12 @@ test("printing_unittests") {
       sources += [ "backend/cups_ipp_util_unittest.cc" ]
     } else {
       sources += [ "backend/cups_helper_unittest.cc" ]
+      if (is_linux) {
+        sources += [ "backend/print_backend_cups_linux_unittest.cc" ]
+      }
+      if (is_mac) {
+        sources += [ "backend/print_backend_cups_mac_unittest.cc" ]
+      }
     }
   }
 }

printing/backend/print_backend.h

@@ -22,15 +22,18 @@ class DictionaryValue;
 // This is the interface for platform-specific code for a print backend
 namespace printing {
 
-// Note: There are raw values. The |printer_name| and |printer_description|
-// require further interpretation on Windows, Mac and Chrome OS. See existing
-// callers for examples.
 struct PRINTING_EXPORT PrinterBasicInfo {
   PrinterBasicInfo();
   PrinterBasicInfo(const PrinterBasicInfo& other);
   ~PrinterBasicInfo();
 
+  // The name of the printer as understood by OS.
   std::string printer_name;
+
+  // The name of the printer as shown in Print Preview.
+  // For Windows SetGetDisplayNameFunction() can be used to set the setter of
+  // this field.
+  std::string display_name;
   std::string printer_description;
   int printer_status = 0;
   int is_default = false;

printing/backend/print_backend_cups.cc

@@ -18,6 +18,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "printing/backend/cups_helper.h"
 #include "printing/backend/print_backend_consts.h"
 #include "url/gurl.h"
@@ -30,8 +31,19 @@ const char kCUPSPrinterInfoOpt[] = "printer-info";
 const char kCUPSPrinterStateOpt[] = "printer-state";
 const char kCUPSPrinterTypeOpt[] = "printer-type";
 
-bool PrinterBasicInfoFromCUPS(const cups_dest_t& printer,
-                              PrinterBasicInfo* printer_info) {
+}  // namespace
+
+PrintBackendCUPS::PrintBackendCUPS(const GURL& print_server_url,
+                                   http_encryption_t encryption,
+                                   bool blocking)
+    : print_server_url_(print_server_url),
+      cups_encryption_(encryption),
+      blocking_(blocking) {}
+
+// static
+bool PrintBackendCUPS::PrinterBasicInfoFromCUPS(
+    const cups_dest_t& printer,
+    PrinterBasicInfo* printer_info) {
   // CUPS can have 'printers' that are actually scanners. (not MFC)
   // At least on Mac. Check for scanners and skip them.
   const char* type_str =
@@ -47,8 +59,6 @@ bool PrinterBasicInfoFromCUPS(const cups_dest_t& printer,
 
   const char* info =
       cupsGetOption(kCUPSPrinterInfoOpt, printer.num_options, printer.options);
-  if (info)
-    printer_info->printer_description = info;
 
   const char* state =
       cupsGetOption(kCUPSPrinterStateOpt, printer.num_options, printer.options);
@@ -65,18 +75,24 @@ bool PrinterBasicInfoFromCUPS(const cups_dest_t& printer,
     printer_info->options[printer.options[opt_index].name] =
         printer.options[opt_index].value;
   }
+
+#if defined(OS_MACOSX)
+  // On Mac, "printer-info" option specifies the printer name and
+  // "printer-make-and-model" specifies the printer description.
+  if (info)
+    printer_info->display_name = info;
+  if (drv_info)
+    printer_info->printer_description = drv_info;
+#else
+  // On Linux destination name specifies the printer name and "printer-info"
+  // specifies the printer description.
+  printer_info->display_name = printer.name;
+  if (info)
+    printer_info->printer_description = info;
+#endif
   return true;
 }
 
-}  // namespace
-
-PrintBackendCUPS::PrintBackendCUPS(const GURL& print_server_url,
-                                   http_encryption_t encryption,
-                                   bool blocking)
-    : print_server_url_(print_server_url),
-      cups_encryption_(encryption),
-      blocking_(blocking) {}
-
 bool PrintBackendCUPS::EnumeratePrinters(PrinterList* printer_list) {
   DCHECK(printer_list);
   printer_list->clear();

printing/backend/print_backend_cups.h

@@ -10,6 +10,7 @@
 #include "base/files/file_util.h"
 #include "printing/backend/cups_helper.h"
 #include "printing/backend/print_backend.h"
+#include "printing/printing_export.h"
 #include "url/gurl.h"
 
 namespace printing {
@@ -20,6 +21,11 @@ class PrintBackendCUPS : public PrintBackend {
                    http_encryption_t encryption,
                    bool blocking);
 
+  // This static function is exposed here for use in the tests.
+  PRINTING_EXPORT static bool PrinterBasicInfoFromCUPS(
+      const cups_dest_t& printer,
+      PrinterBasicInfo* printer_info);
+
  private:
   ~PrintBackendCUPS() override {}
 

printing/backend/print_backend_cups_linux_unittest.cc

+// Copyright 2019 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 <cups/cups.h>
+
+#include "printing/backend/print_backend.h"
+#include "printing/backend/print_backend_cups.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace printing {
+
+TEST(PrintBackendCupsLinuxTest, PrinterBasicInfoFromCUPS) {
+  const char kName[] = "printer";
+  cups_dest_t* printer = nullptr;
+  ASSERT_EQ(1, cupsAddDest(kName, nullptr, 0, &printer));
+
+  int num_options = 0;
+  cups_option_t* options = nullptr;
+  num_options =
+      cupsAddOption("printer-info", "description", num_options, &options);
+  printer->num_options = num_options;
+  printer->options = options;
+
+  PrinterBasicInfo printer_info;
+  PrintBackendCUPS::PrinterBasicInfoFromCUPS(*printer, &printer_info);
+  cupsFreeDests(1, printer);
+
+  EXPECT_EQ("printer", printer_info.printer_name);
+  EXPECT_EQ("printer", printer_info.display_name);
+  EXPECT_EQ("description", printer_info.printer_description);
+}
+
+}  // namespace printing

printing/backend/print_backend_cups_mac_unittest.cc

+// Copyright 2019 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 <cups/cups.h>
+
+#include "printing/backend/print_backend.h"
+#include "printing/backend/print_backend_cups.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace printing {
+
+TEST(PrintBackendCupsMacTest, PrinterBasicInfoFromCUPS) {
+  const char kName[] = "printer";
+  cups_dest_t* printer = nullptr;
+  ASSERT_EQ(1, cupsAddDest(kName, nullptr, 0, &printer));
+
+  int num_options = 0;
+  cups_option_t* options = nullptr;
+  num_options = cupsAddOption("printer-info", "name", num_options, &options);
+  num_options = cupsAddOption("printer-make-and-model", "description",
+                              num_options, &options);
+  printer->num_options = num_options;
+  printer->options = options;
+
+  PrinterBasicInfo printer_info;
+  PrintBackendCUPS::PrinterBasicInfoFromCUPS(*printer, &printer_info);
+  cupsFreeDests(1, printer);
+
+  EXPECT_EQ("printer", printer_info.printer_name);
+  EXPECT_EQ("name", printer_info.display_name);
+  EXPECT_EQ("description", printer_info.printer_description);
+}
+
+}  // namespace printing

printing/backend/win_helper.cc

@@ -347,6 +347,12 @@ bool InitBasicPrinterInfo(HANDLE printer, PrinterBasicInfo* printer_info) {
     return false;
 
   printer_info->printer_name = base::WideToUTF8(info_2.get()->pPrinterName);
+  if (g_get_display_name_func) {
+    printer_info->display_name =
+        g_get_display_name_func(printer_info->printer_name);
+  } else {
+    printer_info->display_name = printer_info->printer_name;
+  }
   if (info_2.get()->pComment) {
     printer_info->printer_description =
         base::WideToUTF8(info_2.get()->pComment);