Do not communicate with printer driver on Print Preview refresh

PrintViewManagerBase::UpdatePrintSettings() is used only for Print
Preview, to re-render the pages. UpdatePrintSettings() makes calls to
printer drivers to fetch settings, but it really only needs the
rendering parameters already provided from the job settings. Remove
the calls to the printer driver and use only the rendering parameters
directly.

To do so, some of the following changes occurred:
1. Change PrintViewManagerBase::UpdatePrintSettings() signature, since
   the call can no longer be cancelled.
2. In print_browsertest.cc and test_print_view_manager.h, change the
   UpdatePrintSettings test to snoop PrintPagesParams instead. Check
   advanced settings in a different test that has direct access to
   PrintSettings.
3. In print_render_frame_helper_browsertest.cc, add parameters to the
   print settings dictionary needed for
   PrintRenderFrameHelper::UpdatePrintSettings() to succeed. Modify the
   tests and test setup to handle these parameters.

Fixed: 879284
Change-Id: Ibd085821532733c1a891782b21667567bb9e7428
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3893234
Reviewed-by: Alan Screen <awscreen@chromium.org>
Commit-Queue: Andy Phan <andyphan@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1117252}

chrome/browser/printing/print_browsertest.cc

@@ -142,6 +142,20 @@ namespace {
 constexpr int kTestPrinterCapabilitiesMaxCopies = 99;
 constexpr int kDefaultDocumentCookie = 1234;
 
+#if !BUILDFLAG(IS_CHROMEOS)
+constexpr gfx::Size kPhysicalSize = gfx::Size(612, 792);
+constexpr gfx::Rect kPrintableArea = gfx::Rect(0, 0, 612, 792);
+
+// The default margins are set to 1.0cm in //printing/print_settings.cc, which
+// is about 28 printer units. The resulting content size is 556 x 736.
+constexpr gfx::Size kExpectedContentSize = gfx::Size(556, 736);
+#endif  // !BUILDFLAG(IS_CHROMEOS)
+
+const PrinterSemanticCapsAndDefaults::Paper kTestPaper{
+    /*display_name=*/"Letter", /*vendor_id=*/"45",
+    /*size_um=*/gfx::Size(215900, 279400),
+    /*printable_area_um=*/gfx::Rect(0, 0, 215900, 279400)};
+
 #if BUILDFLAG(ENABLE_PRINT_CONTENT_ANALYSIS)
 constexpr char kFakeDmToken[] = "fake-dm-token";
 #endif  // BUILDFLAG(ENABLE_PRINT_CONTENT_ANALYSIS)
@@ -759,6 +773,7 @@ class PrintBrowserTest : public InProcessBrowserTest {
     default_caps->copies_max = kTestPrinterCapabilitiesMaxCopies;
     default_caps->dpis = kTestPrinterCapabilitiesDefaultDpis;
     default_caps->default_dpi = kTestPrinterCapabilitiesDpi;
+    default_caps->papers.push_back(kTestPaper);
     test_print_backend_->AddValidPrinter(
         printer_name, std::move(default_caps),
         std::make_unique<PrinterBasicInfo>(printer_info));
@@ -2834,26 +2849,15 @@ IN_PROC_BROWSER_TEST_P(SystemAccessProcessPrintBrowserTest,
 
   PrintAndWaitUntilPreviewIsReady();
 
-  EXPECT_EQ(rendered_page_count(), 3u);
+  EXPECT_EQ(3u, rendered_page_count());
 
-  ASSERT_TRUE(print_view_manager.snooped_settings());
-  EXPECT_EQ(print_view_manager.snooped_settings()->copies(),
-            kTestPrintSettingsCopies);
-#if BUILDFLAG(IS_LINUX) && BUILDFLAG(USE_CUPS)
-  // Collect just the keys to compare the info options vs. advanced settings.
-  std::vector<std::string> advanced_setting_keys;
-  std::vector<std::string> print_info_options_keys;
-  const PrintSettings::AdvancedSettings& advanced_settings =
-      print_view_manager.snooped_settings()->advanced_settings();
-  for (const auto& advanced_setting : advanced_settings) {
-    advanced_setting_keys.push_back(advanced_setting.first);
-  }
-  for (const auto& option : kTestDummyPrintInfoOptions) {
-    print_info_options_keys.push_back(option.first);
-  }
-  EXPECT_THAT(advanced_setting_keys,
-              testing::UnorderedElementsAreArray(print_info_options_keys));
-#endif  // BUILDFLAG(IS_LINUX) && BUILDFLAG(USE_CUPS)
+  const mojom::PrintPagesParamsPtr& snooped_params =
+      print_view_manager.snooped_params();
+  ASSERT_TRUE(snooped_params);
+  EXPECT_EQ(kTestPrinterCapabilitiesDpi, snooped_params->params->dpi);
+  EXPECT_EQ(kPhysicalSize, snooped_params->params->page_size);
+  EXPECT_EQ(kPrintableArea, snooped_params->params->printable_area);
+  EXPECT_EQ(kExpectedContentSize, snooped_params->params->content_size);
 }
 
 #if BUILDFLAG(ENABLE_OOP_PRINTING)
@@ -2893,6 +2897,24 @@ IN_PROC_BROWSER_TEST_P(SystemAccessProcessServicePrintBrowserTest,
   EXPECT_EQ(document_done_result(), mojom::ResultCode::kSuccess);
   EXPECT_EQ(error_dialog_shown_count(), 0u);
   EXPECT_EQ(print_job_destruction_count(), 1);
+
+#if BUILDFLAG(IS_LINUX) && BUILDFLAG(USE_CUPS)
+  absl::optional<PrintSettings> settings = document_print_settings();
+  ASSERT_TRUE(settings);
+  // Collect just the keys to compare the info options vs. advanced settings.
+  std::vector<std::string> advanced_setting_keys;
+  std::vector<std::string> print_info_options_keys;
+  const PrintSettings::AdvancedSettings& advanced_settings =
+      settings->advanced_settings();
+  for (const auto& advanced_setting : advanced_settings) {
+    advanced_setting_keys.push_back(advanced_setting.first);
+  }
+  for (const auto& option : kTestDummyPrintInfoOptions) {
+    print_info_options_keys.push_back(option.first);
+  }
+  EXPECT_THAT(advanced_setting_keys,
+              testing::UnorderedElementsAreArray(print_info_options_keys));
+#endif  // BUILDFLAG(IS_LINUX) && BUILDFLAG(USE_CUPS)
 }
 
 IN_PROC_BROWSER_TEST_P(SystemAccessProcessServicePrintBrowserTest,

chrome/browser/printing/print_view_manager_base.cc

@@ -59,6 +59,8 @@
 
 #if BUILDFLAG(ENABLE_PRINT_PREVIEW)
 #include "chrome/browser/printing/print_view_manager.h"
+#include "printing/print_settings.h"
+#include "printing/print_settings_conversion.h"
 #endif
 
 #if BUILDFLAG(ENABLE_OOP_PRINTING)
@@ -114,42 +116,6 @@ mojom::PrintPagesParamsPtr CreateEmptyPrintPagesParamsPtr() {
   return params;
 }
 
-#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
-#if BUILDFLAG(IS_WIN)
-void NotifySystemDialogCancelled(base::WeakPtr<PrintViewManagerBase> manager) {
-  if (manager)
-    manager->SystemDialogCancelled();
-}
-#endif  // BUILDFLAG(IS_WIN)
-
-void OnDidUpdatePrintSettings(
-    scoped_refptr<PrintQueriesQueue> queue,
-    std::unique_ptr<PrinterQuery> printer_query,
-    mojom::PrintManagerHost::UpdatePrintSettingsCallback callback,
-    base::WeakPtr<PrintViewManagerBase> manager) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(printer_query);
-  mojom::PrintPagesParamsPtr params = CreateEmptyPrintPagesParamsPtr();
-  if (printer_query->last_status() == mojom::ResultCode::kSuccess) {
-    RenderParamsFromPrintSettings(printer_query->settings(),
-                                  params->params.get());
-    params->params->document_cookie = printer_query->cookie();
-    params->pages = printer_query->settings().ranges();
-  }
-  bool canceled = printer_query->last_status() == mojom::ResultCode::kCanceled;
-#if BUILDFLAG(IS_WIN)
-  if (canceled)
-    NotifySystemDialogCancelled(std::move(manager));
-#endif
-
-  std::move(callback).Run(std::move(params), canceled);
-
-  if (printer_query->cookie() && printer_query->settings().dpi()) {
-    queue->QueuePrinterQuery(std::move(printer_query));
-  }
-}
-#endif  // BUILDFLAG(ENABLE_PRINT_PREVIEW)
-
 void OnDidScriptedPrint(
     scoped_refptr<PrintQueriesQueue> queue,
     std::unique_ptr<PrinterQuery> printer_query,
@@ -355,16 +321,6 @@ void PrintViewManagerBase::StartLocalPrintJob(
                 settings.page_setup_device_units().printable_area().origin());
   std::move(callback).Run(base::Value());
 }
-
-void PrintViewManagerBase::UpdatePrintSettingsReply(
-    mojom::PrintManagerHost::UpdatePrintSettingsCallback callback,
-    mojom::PrintPagesParamsPtr params,
-    bool canceled) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  set_cookie(params->params->document_cookie);
-  std::move(callback).Run(std::move(params), canceled);
-}
-
 #endif  // BUILDFLAG(ENABLE_PRINT_PREVIEW)
 
 void PrintViewManagerBase::GetDefaultPrintSettingsReply(
@@ -603,17 +559,29 @@ void PrintViewManagerBase::UpdatePrintSettings(
     UpdatePrintSettingsCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!printing_enabled_.GetValue()) {
-    UpdatePrintSettingsReply(std::move(callback),
-                             CreateEmptyPrintPagesParamsPtr(), false);
+    std::move(callback).Run(CreateEmptyPrintPagesParamsPtr());
+    return;
+  }
+
+  absl::optional<int> printer_type_value =
+      job_settings.FindInt(kSettingPrinterType);
+  if (!printer_type_value) {
+    std::move(callback).Run(CreateEmptyPrintPagesParamsPtr());
     return;
   }
 
-  if (!job_settings.FindInt(kSettingPrinterType)) {
-    UpdatePrintSettingsReply(std::move(callback),
-                             CreateEmptyPrintPagesParamsPtr(), false);
+  mojom::PrinterType printer_type =
+      static_cast<mojom::PrinterType>(*printer_type_value);
+  if (printer_type != mojom::PrinterType::kExtension &&
+      printer_type != mojom::PrinterType::kPdf &&
+      printer_type != mojom::PrinterType::kLocal) {
+    std::move(callback).Run(CreateEmptyPrintPagesParamsPtr());
     return;
   }
 
+  // `job_settings` does not yet contain the rasterized PDF dpi, so if the user
+  // has the print preference set, fetch it for use in
+  // `PrintSettingsFromJobSettings()`.
   content::BrowserContext* context =
       web_contents() ? web_contents()->GetBrowserContext() : nullptr;
   PrefService* prefs =
@@ -624,17 +592,21 @@ void PrintViewManagerBase::UpdatePrintSettings(
       job_settings.Set(kSettingRasterizePdfDpi, value);
   }
 
-  auto callback_wrapper =
-      base::BindOnce(&PrintViewManagerBase::UpdatePrintSettingsReply,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback));
-  std::unique_ptr<PrinterQuery> printer_query =
-      queue_->CreatePrinterQuery(content::GlobalRenderFrameHostId());
-  auto* printer_query_ptr = printer_query.get();
-  printer_query_ptr->SetSettings(
-      std::move(job_settings),
-      base::BindOnce(&OnDidUpdatePrintSettings, queue_,
-                     std::move(printer_query), std::move(callback_wrapper),
-                     weak_ptr_factory_.GetWeakPtr()));
+  std::unique_ptr<PrintSettings> print_settings =
+      PrintSettingsFromJobSettings(job_settings);
+  if (!print_settings) {
+    std::move(callback).Run(CreateEmptyPrintPagesParamsPtr());
+    return;
+  }
+
+  mojom::PrintPagesParamsPtr settings = mojom::PrintPagesParams::New();
+  settings->pages = GetPageRangesFromJobSettings(job_settings);
+  settings->params = mojom::PrintParams::New();
+  RenderParamsFromPrintSettings(*print_settings, settings->params.get());
+  settings->params->document_cookie = PrintSettings::NewCookie();
+  set_cookie(settings->params->document_cookie);
+
+  std::move(callback).Run(std::move(settings));
 }
 #endif  // BUILDFLAG(ENABLE_PRINT_PREVIEW)
 

chrome/browser/printing/test_print_view_manager.cc

@@ -27,29 +27,20 @@ namespace printing {
 namespace {
 
 void OnDidUpdatePrintSettings(
-    std::unique_ptr<PrintSettings>& snooped_settings,
-    scoped_refptr<PrintQueriesQueue> queue,
-    std::unique_ptr<PrinterQuery> printer_query,
-    mojom::PrintManagerHost::UpdatePrintSettingsCallback callback) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(printer_query);
-  auto params = mojom::PrintPagesParams::New();
-  params->params = mojom::PrintParams::New();
-  if (printer_query->last_status() == mojom::ResultCode::kSuccess) {
-    RenderParamsFromPrintSettings(printer_query->settings(),
-                                  params->params.get());
-    params->params->document_cookie = printer_query->cookie();
-    params->pages = printer_query->settings().ranges();
-    snooped_settings =
-        std::make_unique<PrintSettings>(printer_query->settings());
-  }
-  bool canceled = printer_query->last_status() == mojom::ResultCode::kCanceled;
-
-  std::move(callback).Run(std::move(params), canceled);
-
-  if (printer_query->cookie() && printer_query->settings().dpi()) {
-    queue->QueuePrinterQuery(std::move(printer_query));
-  }
+    mojom::PrintPagesParamsPtr& snooped_params,
+    mojom::PrintManagerHost::UpdatePrintSettingsCallback callback,
+    mojom::PrintPagesParamsPtr settings) {
+  snooped_params = mojom::PrintPagesParams::New();
+  auto params = mojom::PrintParams::New();
+
+  // Copy over any relevant fields that we want to snoop.
+  params->dpi = settings->params->dpi;
+  params->page_size = settings->params->page_size;
+  params->content_size = settings->params->content_size;
+  params->printable_area = settings->params->printable_area;
+  snooped_params->params = std::move(params);
+
+  std::move(callback).Run(std::move(settings));
 }
 
 }  // namespace
@@ -119,14 +110,10 @@ void TestPrintViewManager::PrintPreviewAllowedForTesting() {
 void TestPrintViewManager::UpdatePrintSettings(
     base::Value::Dict job_settings,
     UpdatePrintSettingsCallback callback) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  std::unique_ptr<PrinterQuery> printer_query =
-      queue_->CreatePrinterQuery(content::GlobalRenderFrameHostId());
-  auto* printer_query_ptr = printer_query.get();
-  printer_query_ptr->SetSettings(
+  PrintViewManagerBase::UpdatePrintSettings(
       std::move(job_settings),
-      base::BindOnce(&OnDidUpdatePrintSettings, std::ref(snooped_settings_),
-                     queue_, std::move(printer_query), std::move(callback)));
+      base::BindOnce(&OnDidUpdatePrintSettings, std::ref(snooped_params_),
+                     std::move(callback)));
 }
 
 }  // namespace printing

chrome/browser/printing/test_print_view_manager.h

@@ -34,7 +34,9 @@ class TestPrintViewManager : public PrintViewManager {
 
   void WaitUntilPreviewIsShownOrCancelled();
 
-  PrintSettings* snooped_settings() { return snooped_settings_.get(); }
+  const mojom::PrintPagesParamsPtr& snooped_params() const {
+    return snooped_params_;
+  }
 
   const absl::optional<bool>& print_now_result() const {
     return print_now_result_;
@@ -59,7 +61,7 @@ class TestPrintViewManager : public PrintViewManager {
   void UpdatePrintSettings(base::Value::Dict job_settings,
                            UpdatePrintSettingsCallback callback) override;
 
-  std::unique_ptr<PrintSettings> snooped_settings_;
+  mojom::PrintPagesParamsPtr snooped_params_;
   absl::optional<bool> print_now_result_;
   OnDidCreatePrintJobCallback on_did_create_print_job_;
 };

components/BUILD.gn

@@ -949,6 +949,7 @@ if (!is_ios) {
       sources += [ "printing/test/print_render_frame_helper_browsertest.cc" ]
       deps += [
         "//build:chromeos_buildflags",
+        "//components/printing/browser",
         "//components/printing/common:mojo_interfaces",
         "//components/printing/test:test_support",
       ]

components/printing/common/print.mojom

@@ -391,7 +391,7 @@ interface PrintManagerHost {
   [EnableIf=enable_print_preview, Sync]
   UpdatePrintSettings(
       mojo_base.mojom.DictionaryValue job_settings)
-      => (PrintPagesParams current_settings, bool canceled);
+      => (PrintPagesParams current_settings);
 
   // Tells the browser to set up the print preview requested by the script. It
   // runs a nested run loop in the renderer until print preview for

components/printing/renderer/print_render_frame_helper.cc

@@ -2499,22 +2499,12 @@ bool PrintRenderFrameHelper::UpdatePrintSettings(
   }
 
   mojom::PrintPagesParamsPtr settings;
-  bool canceled = false;
-  GetPrintManagerHost()->UpdatePrintSettings(job_settings->Clone(), &settings,
-                                             &canceled);
-
-  // If mojom::PrintManagerHost is disconnected in the browser after calling
-  // UpdatePrintSettings(), |settings| could be null.
+  GetPrintManagerHost()->UpdatePrintSettings(job_settings->Clone(), &settings);
   if (!settings) {
     print_preview_context_.set_error(PREVIEW_ERROR_EMPTY_PRINTER_SETTINGS);
     return false;
   }
 
-  if (canceled) {
-    notify_browser_of_print_failure_ = false;
-    return false;
-  }
-
   settings->params->preview_ui_id = job_settings->FindInt(kPreviewUIID).value();
 
   // Validate expected print preview settings.

components/printing/test/mock_printer.cc

@@ -131,20 +131,6 @@ void MockPrinter::SetDefaultPrintSettings(
   url_ = params.url;
 }
 
-void MockPrinter::UseInvalidSettings() {
-  use_invalid_settings_ = true;
-  printing::mojom::PrintParams empty_param;
-  SetDefaultPrintSettings(empty_param);
-}
-
-void MockPrinter::UseInvalidPageSize() {
-  page_size_.SetSize(0, 0);
-}
-
-void MockPrinter::UseInvalidContentSize() {
-  content_size_.SetSize(0, 0);
-}
-
 void MockPrinter::ScriptedPrint(int cookie,
                                 uint32_t expected_pages_count,
                                 bool has_selection,

components/printing/test/mock_printer.h

@@ -69,9 +69,6 @@ class MockPrinter {
   // Functions that changes settings of a pseudo printer.
   void ResetPrinter();
   void SetDefaultPrintSettings(const printing::mojom::PrintParams& params);
-  void UseInvalidSettings();
-  void UseInvalidPageSize();
-  void UseInvalidContentSize();
 
   // Functions that handle mojo messages.
   printing::mojom::PrintParamsPtr GetDefaultPrintSettings();