Refactor PrintSettingsFromJobSettings

Refactor PrintSettingsFromJobSettings to return a unique pointer.
Also add unit tests for the print settings conversion functions.

Test: ./printing_unittests
Change-Id: I4afd3e7ff466948cea9a372293dbff93ca2a12bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2337953
Auto-Submit: Pranav Batra <batrapranav@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804008}

chrome/browser/printing/test_printer_query.cc

@@ -52,11 +52,13 @@ void TestPrinterQuery::SetSettings(base::Value new_settings,
 #if defined(OS_WIN)
   DCHECK(printer_type_);
 #endif
-  auto settings = std::make_unique<PrintSettings>();
-  PrintingContext::Result result =
-      PrintSettingsFromJobSettings(new_settings, settings.get())
-          ? PrintingContext::OK
-          : PrintingContext::FAILED;
+  std::unique_ptr<PrintSettings> settings =
+      PrintSettingsFromJobSettings(new_settings);
+  PrintingContext::Result result = PrintingContext::OK;
+  if (!settings) {
+    settings = std::make_unique<PrintSettings>();
+    result = PrintingContext::FAILED;
+  }
 
   float device_microns_per_device_unit =
       static_cast<float>(kMicronsPerInch) / settings->device_units_per_inch();

printing/print_settings_conversion.cc

@@ -96,12 +96,13 @@ PageRanges GetPageRangesFromJobSettings(const base::Value& job_settings) {
   return page_ranges;
 }
 
-bool PrintSettingsFromJobSettings(const base::Value& job_settings,
-                                  PrintSettings* settings) {
+std::unique_ptr<PrintSettings> PrintSettingsFromJobSettings(
+    const base::Value& job_settings) {
+  auto settings = std::make_unique<PrintSettings>();
   base::Optional<bool> display_header_footer =
       job_settings.FindBoolKey(kSettingHeaderFooterEnabled);
   if (!display_header_footer.has_value())
-    return false;
+    return nullptr;
 
   settings->set_display_header_footer(display_header_footer.value());
   if (settings->display_header_footer()) {
@@ -110,7 +111,7 @@ bool PrintSettingsFromJobSettings(const base::Value& job_settings,
     const std::string* url =
         job_settings.FindStringKey(kSettingHeaderFooterURL);
     if (!title || !url)
-      return false;
+      return nullptr;
 
     settings->set_title(base::UTF8ToUTF16(*title));
     settings->set_url(base::UTF8ToUTF16(*url));
@@ -121,7 +122,7 @@ bool PrintSettingsFromJobSettings(const base::Value& job_settings,
   base::Optional<bool> selection_only =
       job_settings.FindBoolKey(kSettingShouldPrintSelectionOnly);
   if (!backgrounds.has_value() || !selection_only.has_value())
-    return false;
+    return nullptr;
 
   settings->set_should_print_backgrounds(backgrounds.value());
   settings->set_selection_only(selection_only.value());
@@ -178,16 +179,16 @@ bool PrintSettingsFromJobSettings(const base::Value& job_settings,
       !duplex_mode.has_value() || !landscape.has_value() ||
       !scale_factor.has_value() || !rasterize_pdf.has_value() ||
       !pages_per_sheet.has_value()) {
-    return false;
+    return nullptr;
   }
+
 #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS)
   base::Optional<int> dpi_horizontal =
       job_settings.FindIntKey(kSettingDpiHorizontal);
   base::Optional<int> dpi_vertical =
       job_settings.FindIntKey(kSettingDpiVertical);
   if (!dpi_horizontal.has_value() || !dpi_vertical.has_value())
-    return false;
-
+    return nullptr;
   settings->set_dpi_xy(dpi_horizontal.value(), dpi_vertical.value());
 #endif
 
@@ -235,7 +236,7 @@ bool PrintSettingsFromJobSettings(const base::Value& job_settings,
     settings->set_pin_value(*pin_value);
 #endif
 
-  return true;
+  return settings;
 }
 
 void PrintSettingsToJobSettingsDebug(const PrintSettings& settings,

printing/print_settings_conversion.h

@@ -20,9 +20,9 @@ class PrintSettings;
 PRINTING_EXPORT PageRanges
 GetPageRangesFromJobSettings(const base::Value& job_settings);
 
-PRINTING_EXPORT bool PrintSettingsFromJobSettings(
-    const base::Value& job_settings,
-    PrintSettings* print_settings);
+// Returns nullptr on failure.
+PRINTING_EXPORT std::unique_ptr<PrintSettings> PrintSettingsFromJobSettings(
+    const base::Value& job_settings);
 
 // Use for debug only, because output is not completely consistent with format
 // of |PrintSettingsFromJobSettings| input.

printing/print_settings_conversion_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/json/json_reader.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "printing/print_settings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -47,16 +48,34 @@ const char kPrinterSettings[] = R"({
 
 }  // namespace
 
+TEST(PrintSettingsConversionTest, ConversionTest_InvalidSettings) {
+  base::Optional<base::Value> value = base::JSONReader::Read("{}");
+  ASSERT_TRUE(value.has_value());
+  EXPECT_FALSE(PrintSettingsFromJobSettings(value.value()));
+}
+
 TEST(PrintSettingsConversionTest, ConversionTest) {
   base::Optional<base::Value> value = base::JSONReader::Read(kPrinterSettings);
   ASSERT_TRUE(value.has_value());
-  PrintSettings settings;
-  bool success = PrintSettingsFromJobSettings(value.value(), &settings);
-  ASSERT_TRUE(success);
+  std::unique_ptr<PrintSettings> settings =
+      PrintSettingsFromJobSettings(value.value());
+  ASSERT_TRUE(settings);
 #if defined(OS_CHROMEOS)
-  EXPECT_TRUE(settings.send_user_info());
-  EXPECT_EQ("username@domain.net", settings.username());
-  EXPECT_EQ("0000", settings.pin_value());
+  EXPECT_TRUE(settings->send_user_info());
+  EXPECT_EQ("username@domain.net", settings->username());
+  EXPECT_EQ("0000", settings->pin_value());
+#endif
+#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS)
+  EXPECT_EQ(settings->dpi_horizontal(), 300);
+  EXPECT_EQ(settings->dpi_vertical(), 300);
+  value->SetIntKey("dpiVertical", 600);
+  settings = PrintSettingsFromJobSettings(value.value());
+  ASSERT_TRUE(settings);
+  EXPECT_EQ(settings->dpi_horizontal(), 300);
+  EXPECT_EQ(settings->dpi_vertical(), 600);
+  EXPECT_TRUE(value->RemoveKey("dpiVertical"));
+  settings = PrintSettingsFromJobSettings(value.value());
+  EXPECT_FALSE(settings);
 #endif
 }
 
@@ -65,11 +84,11 @@ TEST(PrintSettingsConversionTest, ConversionTest_DontSendUsername) {
   base::Optional<base::Value> value = base::JSONReader::Read(kPrinterSettings);
   ASSERT_TRUE(value.has_value());
   value->SetKey(kSettingSendUserInfo, base::Value(false));
-  PrintSettings settings;
-  bool success = PrintSettingsFromJobSettings(value.value(), &settings);
-  ASSERT_TRUE(success);
-  EXPECT_FALSE(settings.send_user_info());
-  EXPECT_EQ("", settings.username());
+  std::unique_ptr<PrintSettings> settings =
+      PrintSettingsFromJobSettings(value.value());
+  ASSERT_TRUE(settings);
+  EXPECT_FALSE(settings->send_user_info());
+  EXPECT_EQ("", settings->username());
 }
 #endif
 

printing/printing_context.cc

@@ -96,10 +96,14 @@ PrintingContext::Result PrintingContext::UsePdfSettings() {
 PrintingContext::Result PrintingContext::UpdatePrintSettings(
     base::Value job_settings) {
   ResetSettings();
-
-  if (!PrintSettingsFromJobSettings(job_settings, settings_.get())) {
-    NOTREACHED();
-    return OnError();
+  {
+    std::unique_ptr<PrintSettings> settings =
+        PrintSettingsFromJobSettings(job_settings);
+    if (!settings) {
+      NOTREACHED();
+      return OnError();
+    }
+    settings_ = std::move(settings);
   }
 
   PrinterType printer_type = static_cast<PrinterType>(