From fd714c1f5cdbe98e7bf4bc47b312e648300b2496 Mon Sep 17 00:00:00 2001 From: Alan Screen Date: Fri, 24 Mar 2023 21:03:29 +0000 Subject: [PATCH] Alter UpdatePrintSettings API to use context ID After https://crrev.com/1117252, UpdatePrintSettings() is no longer called as a regular query for Print Preview. It is now only used prior to invoking the system print dialog or at the start of printing a document from Print Preview. This significantly changes the use case for it, as now it can always require a context ID when it is used out-of-process. This change in API requires an OOP printing context to be established earlier. It also affects StartPrinting() since the settings are now only longer needed at this time when the system print dialog is invoked from in the browser. This facilitates some cleanup, since the Mojo PrintTargetType is no longer necessary. It is now sufficient just to know if the print job is from a system dialog. Bug: 1414968 Change-Id: Ib5e50263df54b550216525dbd10a83e6ee3efff4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4256086 Reviewed-by: Lei Zhang Commit-Queue: Alan Screen Reviewed-by: Nasko Oskov Cr-Commit-Position: refs/heads/main@{#1121921} --- .../printing/print_backend_browsertest.cc | 101 +++++++++++------- .../printing/print_backend_service_manager.cc | 30 ++++-- .../printing/print_backend_service_manager.h | 34 +++--- .../print_backend_service_test_impl.cc | 5 +- .../print_backend_service_test_impl.h | 1 + .../browser/printing/print_job_worker_oop.cc | 22 ++-- .../browser/printing/print_job_worker_oop.h | 9 +- .../printing/print_view_manager_base.cc | 5 + chrome/browser/printing/printer_query_oop.cc | 93 +++++++++------- chrome/browser/printing/printer_query_oop.h | 10 +- ...system_access_process_print_browsertest.cc | 80 +++++++++----- .../printing/print_backend_service_impl.cc | 69 ++++-------- .../printing/print_backend_service_impl.h | 10 +- .../public/mojom/print_backend_service.mojom | 32 +++--- printing/printing_context.cc | 11 ++ 15 files changed, 291 insertions(+), 221 deletions(-) diff --git a/chrome/browser/printing/print_backend_browsertest.cc b/chrome/browser/printing/print_backend_browsertest.cc index d235830a30a91d..d10a1869b0192e 100644 --- a/chrome/browser/printing/print_backend_browsertest.cc +++ b/chrome/browser/printing/print_backend_browsertest.cc @@ -187,15 +187,37 @@ class PrintBackendBrowserTest : public InProcessBrowserTest { return kContextId; } - mojom::ResultCode StartPrintingAndWait(const PrintSettings& print_settings) { - const uint32_t context_id = EstablishPrintingContextAndWait(); - mojom::ResultCode result; + mojom::PrintSettingsResultPtr UpdatePrintSettingsAndWait( + uint32_t context_id, + const PrintSettings& print_settings) { + base::Value::Dict job_settings = + PrintSettingsToJobSettingsDebug(print_settings); + job_settings.Set(kSettingPrinterType, + static_cast(mojom::PrinterType::kLocal)); // Safe to use base::Unretained(this) since waiting locally on the callback // forces a shorter lifetime than `this`. + mojom::PrintSettingsResultPtr settings; + GetPrintBackendService()->UpdatePrintSettings( + context_id, std::move(job_settings), + base::BindOnce(&PrintBackendBrowserTest::CapturePrintSettings, + base::Unretained(this), std::ref(settings))); + WaitUntilCallbackReceived(); + return settings; + } + + mojom::ResultCode StartPrintingAndWait(uint32_t context_id, + const PrintSettings& print_settings) { + UpdatePrintSettingsAndWait(context_id, print_settings); + + // Safe to use base::Unretained(this) since waiting locally on the callback + // forces a shorter lifetime than `this`. + mojom::ResultCode result; GetPrintBackendService()->StartPrinting( context_id, kTestDocumentCookie, u"document name", - mojom::PrintTargetType::kDirectToDevice, print_settings, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + /*settings=*/absl::nullopt, +#endif base::BindOnce(&PrintBackendBrowserTest::CaptureResult, base::Unretained(this), std::ref(result))); WaitUntilCallbackReceived(); @@ -571,23 +593,17 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, UpdatePrintSettings) { AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); - mojom::PrintSettingsResultPtr settings; + // Isolated call has no corresponding cleanup of the printing context. + SkipPersistentContextsCheckOnShutdown(); + + const uint32_t context_id = EstablishPrintingContextAndWait(); PrintSettings print_settings; print_settings.set_device_name(kDefaultPrinterName16); print_settings.set_dpi(kPrintSettingsOverrideDpi); - base::Value::Dict job_settings = - PrintSettingsToJobSettingsDebug(print_settings); - job_settings.Set(kSettingPrinterType, - static_cast(mojom::PrinterType::kLocal)); - // Safe to use base::Unretained(this) since waiting locally on the callback - // forces a shorter lifetime than `this`. - GetPrintBackendService()->UpdatePrintSettings( - std::move(job_settings), - base::BindOnce(&PrintBackendBrowserTest::CapturePrintSettings, - base::Unretained(this), std::ref(settings))); - WaitUntilCallbackReceived(); + mojom::PrintSettingsResultPtr settings = + UpdatePrintSettingsAndWait(context_id, print_settings); ASSERT_TRUE(settings->is_settings()); EXPECT_EQ(settings->get_settings().copies(), kPrintSettingsCopies); EXPECT_EQ(settings->get_settings().dpi(), kPrintSettingsOverrideDpi); @@ -604,38 +620,25 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, UpdatePrintSettings) { // Updating for an invalid printer should not return print settings. print_settings.set_device_name(kInvalidPrinterName16); - job_settings = PrintSettingsToJobSettingsDebug(print_settings); - job_settings.Set(kSettingPrinterType, - static_cast(mojom::PrinterType::kLocal)); - GetPrintBackendService()->UpdatePrintSettings( - std::move(job_settings), - base::BindOnce(&PrintBackendBrowserTest::CapturePrintSettings, - base::Unretained(this), std::ref(settings))); - WaitUntilCallbackReceived(); + + settings = UpdatePrintSettingsAndWait(context_id, print_settings); ASSERT_TRUE(settings->is_result_code()); EXPECT_EQ(settings->get_result_code(), mojom::ResultCode::kFailed); } -IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, StartPrintingValidPrinter) { +IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, StartPrinting) { LaunchService(); AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); - PrintSettings print_settings; - print_settings.set_device_name(kDefaultPrinterName16); - - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kSuccess); -} - -IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, StartPrintingInvalidPrinter) { - LaunchService(); - AddDefaultPrinter(); - SetPrinterNameForSubsequentContexts(kDefaultPrinterName); + const uint32_t context_id = EstablishPrintingContextAndWait(); PrintSettings print_settings; - print_settings.set_device_name(kInvalidPrinterName16); + print_settings.set_device_name(kDefaultPrinterName16); + ASSERT_TRUE(UpdatePrintSettingsAndWait(context_id, print_settings)); - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kFailed); + EXPECT_EQ(StartPrintingAndWait(context_id, print_settings), + mojom::ResultCode::kSuccess); } #if BUILDFLAG(IS_WIN) @@ -644,10 +647,14 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, RenderPrintedPage) { AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); + const uint32_t context_id = EstablishPrintingContextAndWait(); + PrintSettings print_settings; print_settings.set_device_name(kDefaultPrinterName16); + ASSERT_TRUE(UpdatePrintSettingsAndWait(context_id, print_settings)); - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kSuccess); + ASSERT_EQ(StartPrintingAndWait(context_id, print_settings), + mojom::ResultCode::kSuccess); absl::optional result = RenderPageAndWait(); EXPECT_EQ(result, mojom::ResultCode::kSuccess); @@ -662,10 +669,14 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, RenderPrintedDocument) { AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); + const uint32_t context_id = EstablishPrintingContextAndWait(); + PrintSettings print_settings; print_settings.set_device_name(kDefaultPrinterName16); + ASSERT_TRUE(UpdatePrintSettingsAndWait(context_id, print_settings)); - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kSuccess); + ASSERT_EQ(StartPrintingAndWait(context_id, print_settings), + mojom::ResultCode::kSuccess); absl::optional result = RenderDocumentAndWait(); EXPECT_EQ(result, mojom::ResultCode::kSuccess); @@ -677,10 +688,14 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, DocumentDone) { AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); + const uint32_t context_id = EstablishPrintingContextAndWait(); + PrintSettings print_settings; print_settings.set_device_name(kDefaultPrinterName16); + ASSERT_TRUE(UpdatePrintSettingsAndWait(context_id, print_settings)); - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kSuccess); + ASSERT_EQ(StartPrintingAndWait(context_id, print_settings), + mojom::ResultCode::kSuccess); // TODO(crbug.com/1008222) Include Windows coverage for RenderDocument() // path once XPS print pipeline is enabled. @@ -699,10 +714,14 @@ IN_PROC_BROWSER_TEST_F(PrintBackendBrowserTest, Cancel) { AddDefaultPrinter(); SetPrinterNameForSubsequentContexts(kDefaultPrinterName); + const uint32_t context_id = EstablishPrintingContextAndWait(); + PrintSettings print_settings; print_settings.set_device_name(kDefaultPrinterName16); + ASSERT_TRUE(UpdatePrintSettingsAndWait(context_id, print_settings)); - EXPECT_EQ(StartPrintingAndWait(print_settings), mojom::ResultCode::kSuccess); + EXPECT_EQ(StartPrintingAndWait(context_id, print_settings), + mojom::ResultCode::kSuccess); CancelAndWait(); } diff --git a/chrome/browser/printing/print_backend_service_manager.cc b/chrome/browser/printing/print_backend_service_manager.cc index 20491466d5b44a..40e9ea23389bad 100644 --- a/chrome/browser/printing/print_backend_service_manager.cc +++ b/chrome/browser/printing/print_backend_service_manager.cc @@ -277,7 +277,7 @@ PrintBackendServiceManager::EstablishPrintingContext( CallbackContext context; auto& service = printer_name.empty() ? GetServiceAndCallbackContextForQueryWithUiClient( - client_id, printer_name, context) + client_id, kEmptyPrinterName, context) : GetServiceAndCallbackContextForPrintDocumentClient( client_id, printer_name, context); @@ -341,16 +341,20 @@ void PrintBackendServiceManager::AskUserForSettings( #endif // BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) void PrintBackendServiceManager::UpdatePrintSettings( - absl::optional client_id, + ClientId client_id, const std::string& printer_name, + ContextId context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback) { + // A blank `printer_name` indicates the destination is unknown, which occurs + // when initiating a system print dialog. When printing a document the + // destination must be known. CallbackContext context; - auto& service = - client_id.has_value() - ? GetServiceAndCallbackContextForQueryWithUiClient( - *client_id, printer_name, context) - : GetServiceAndCallbackContextForQuery(printer_name, context); + auto& service = printer_name.empty() + ? GetServiceAndCallbackContextForQueryWithUiClient( + client_id, printer_name, context) + : GetServiceAndCallbackContextForPrintDocumentClient( + client_id, printer_name, context); SaveCallback(GetRemoteSavedUpdatePrintSettingsCallbacks(context.is_sandboxed), context.remote_id, context.saved_callback_id, @@ -360,7 +364,7 @@ void PrintBackendServiceManager::UpdatePrintSettings( LogCallToRemote("UpdatePrintSettings", context); service->UpdatePrintSettings( - std::move(job_settings), + *context_id, std::move(job_settings), base::BindOnce(&PrintBackendServiceManager::OnDidUpdatePrintSettings, base::Unretained(this), std::move(context))); } @@ -371,8 +375,9 @@ void PrintBackendServiceManager::StartPrinting( ContextId context_id, int document_cookie, const std::u16string& document_name, - mojom::PrintTargetType target_type, - const PrintSettings& settings, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + absl::optional settings, +#endif mojom::PrintBackendService::StartPrintingCallback callback) { CallbackContext context; auto& service = GetServiceAndCallbackContextForPrintDocumentClient( @@ -386,7 +391,10 @@ void PrintBackendServiceManager::StartPrinting( LogCallToRemote("StartPrinting", context); service->StartPrinting( - *context_id, document_cookie, document_name, target_type, settings, + *context_id, document_cookie, document_name, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + settings, +#endif base::BindOnce(&PrintBackendServiceManager::OnDidStartPrinting, base::Unretained(this), std::move(context))); } diff --git a/chrome/browser/printing/print_backend_service_manager.h b/chrome/browser/printing/print_backend_service_manager.h index 1249ed60286a50..753a073c4bc4a0 100644 --- a/chrome/browser/printing/print_backend_service_manager.h +++ b/chrome/browser/printing/print_backend_service_manager.h @@ -156,32 +156,34 @@ class PrintBackendServiceManager { bool is_scripted, mojom::PrintBackendService::AskUserForSettingsCallback callback); #endif - // `UpdatePrintSettings()` can be used in two different scenarios: - // - For Print Preview, where the desire is to use the appropriate - // service based on the `printer_name` indicated. - // - System printing, where a particular PrintBackendService instance - // is desired, and is selected based upon a ClientId. - // When `client_id` is null, then the `printer_name` will be used to select - // the service. Otherwise the `client_id` value will be used, similar to - // other methods related for supporting the system print dialog and for - // printing the document. - // TODO(crbug.com/1414968): Remove use of optional for `client_id` once - // this the callers are updated to take advantage of `UpdatePrintSettings()` - // no longer being needed for Print Preview queries after - // https://crrev.com/1117252. + // `UpdatePrintSettings()` can be used prior to initiating a system print + // dialog or right before starting to print a document. The first requires a + // `client_id` of `kQueryWithUi` type, while the latter requires a the ID to + // be of type `kPrintDocument`. + // The destination printer is still unknown when initiating a system print + // dialog, so `printer_name` will be empty in this case. The destination + // must be known when starting to print a document. `UpdatePrintSettings()` + // uses this insight to know what kind of client type is to be expected for + // the provided `client_id`. The function will CHECK if the `client_id` + // is not registered for the expected type. void UpdatePrintSettings( - absl::optional client_id, + ClientId client_id, const std::string& printer_name, + ContextId context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback); + // `StartPrinting()` initiates the printing of a document. The optional + // `settings` is used in the case where a system print dialog is invoked + // from in the browser, and this provides those settings for printing. void StartPrinting( ClientId client_id, const std::string& printer_name, ContextId context_id, int document_cookie, const std::u16string& document_name, - mojom::PrintTargetType target_type, - const PrintSettings& settings, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + absl::optional settings, +#endif mojom::PrintBackendService::StartPrintingCallback callback); #if BUILDFLAG(IS_WIN) void RenderPrintedPage( diff --git a/chrome/browser/printing/print_backend_service_test_impl.cc b/chrome/browser/printing/print_backend_service_test_impl.cc index cb3b0e0186d571..2aa81266ab5c9b 100644 --- a/chrome/browser/printing/print_backend_service_test_impl.cc +++ b/chrome/browser/printing/print_backend_service_test_impl.cc @@ -144,6 +144,7 @@ void PrintBackendServiceTestImpl::FetchCapabilities( } void PrintBackendServiceTestImpl::UpdatePrintSettings( + uint32_t context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback) { if (terminate_receiver_) { @@ -151,8 +152,8 @@ void PrintBackendServiceTestImpl::UpdatePrintSettings( return; } - PrintBackendServiceImpl::UpdatePrintSettings(std::move(job_settings), - std::move(callback)); + PrintBackendServiceImpl::UpdatePrintSettings( + context_id, std::move(job_settings), std::move(callback)); } #if BUILDFLAG(IS_WIN) diff --git a/chrome/browser/printing/print_backend_service_test_impl.h b/chrome/browser/printing/print_backend_service_test_impl.h index 4cf1eae445192e..f10b3ddf87b10b 100644 --- a/chrome/browser/printing/print_backend_service_test_impl.h +++ b/chrome/browser/printing/print_backend_service_test_impl.h @@ -97,6 +97,7 @@ class PrintBackendServiceTestImpl : public PrintBackendServiceImpl { const std::string& printer_name, mojom::PrintBackendService::FetchCapabilitiesCallback callback) override; void UpdatePrintSettings( + uint32_t context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback) override; diff --git a/chrome/browser/printing/print_job_worker_oop.cc b/chrome/browser/printing/print_job_worker_oop.cc index ed9338216060ee..632a59ae5b0b03 100644 --- a/chrome/browser/printing/print_job_worker_oop.cc +++ b/chrome/browser/printing/print_job_worker_oop.cc @@ -55,13 +55,13 @@ PrintJobWorkerOop::PrintJobWorkerOop( absl::optional client_id, absl::optional context_id, PrintJob* print_job, - mojom::PrintTargetType print_target_type) + bool print_from_system_dialog) : PrintJobWorkerOop(std::move(printing_context_delegate), std::move(printing_context), client_id, context_id, print_job, - print_target_type, + print_from_system_dialog, /*simulate_spooling_memory_errors=*/false) {} PrintJobWorkerOop::PrintJobWorkerOop( @@ -70,7 +70,7 @@ PrintJobWorkerOop::PrintJobWorkerOop( absl::optional client_id, absl::optional context_id, PrintJob* print_job, - mojom::PrintTargetType print_target_type, + bool print_from_system_dialog, bool simulate_spooling_memory_errors) : PrintJobWorker(std::move(printing_context_delegate), std::move(printing_context), @@ -78,7 +78,7 @@ PrintJobWorkerOop::PrintJobWorkerOop( simulate_spooling_memory_errors_(simulate_spooling_memory_errors), service_manager_client_id_(client_id), printing_context_id_(context_id), - print_target_type_(print_target_type) {} + print_from_system_dialog_(print_from_system_dialog) {} PrintJobWorkerOop::~PrintJobWorkerOop() { DCHECK(!service_manager_client_id_.has_value()); @@ -327,7 +327,7 @@ bool PrintJobWorkerOop::TryRestartPrinting() { service_mgr.SetPrinterDriverFoundToRequireElevatedPrivilege(device_name_); #if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) - if (print_target_type_ == mojom::PrintTargetType::kSystemDialog) { + if (print_from_system_dialog_) { // Cannot print from process where system dialog was displayed. Another // print attempt where dialog is used again will be required. PRINTER_LOG(ERROR) << "Failure during system printing, unable to retry."; @@ -429,10 +429,18 @@ void PrintJobWorkerOop::SendStartPrinting(const std::string& device_name, SendEstablishPrintingContext(); } +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + absl::optional settings; + if (print_from_system_dialog_) { + settings = document_oop_->settings(); + } +#endif service_mgr.StartPrinting( *service_manager_client_id_, device_name, *printing_context_id_, - document_cookie, document_name_, print_target_type_, - document_oop_->settings(), + document_cookie, document_name_, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + settings, +#endif base::BindOnce(&PrintJobWorkerOop::OnDidStartPrinting, ui_weak_factory_.GetWeakPtr())); } diff --git a/chrome/browser/printing/print_job_worker_oop.h b/chrome/browser/printing/print_job_worker_oop.h index 0c6c77137e87f8..e51d45f5f8d249 100644 --- a/chrome/browser/printing/print_job_worker_oop.h +++ b/chrome/browser/printing/print_job_worker_oop.h @@ -43,7 +43,7 @@ class PrintJobWorkerOop : public PrintJobWorker { absl::optional client_id, absl::optional context_id, PrintJob* print_job, - mojom::PrintTargetType print_target_type); + bool print_from_system_dialog); PrintJobWorkerOop(const PrintJobWorkerOop&) = delete; PrintJobWorkerOop& operator=(const PrintJobWorkerOop&) = delete; ~PrintJobWorkerOop() override; @@ -59,7 +59,7 @@ class PrintJobWorkerOop : public PrintJobWorker { absl::optional client_id, absl::optional context_id, PrintJob* print_job, - mojom::PrintTargetType print_target_type, + bool print_from_system_dialog, bool simulate_spooling_memory_errors); // Local callback wrappers for Print Backend Service mojom call. Virtual to @@ -137,9 +137,8 @@ class PrintJobWorkerOop : public PrintJobWorker { // to avoid any potential confusion between them. scoped_refptr document_oop_; - // The type of target to print to. Used only from the UI thread. - mojom::PrintTargetType print_target_type_ = - mojom::PrintTargetType::kDirectToDevice; + // Indicates if the print job was initiated from the print system dialog. + const bool print_from_system_dialog_; #if BUILDFLAG(IS_WIN) // Number of pages that have completed printing. diff --git a/chrome/browser/printing/print_view_manager_base.cc b/chrome/browser/printing/print_view_manager_base.cc index 9c9a56c230c97a..7df5210a39cd01 100644 --- a/chrome/browser/printing/print_view_manager_base.cc +++ b/chrome/browser/printing/print_view_manager_base.cc @@ -241,6 +241,11 @@ void PrintViewManagerBase::PrintForPrintPreview( std::move(callback)); std::unique_ptr printer_query = queue_->CreatePrinterQuery(rfh->GetGlobalId()); +#if BUILDFLAG(ENABLE_OOP_PRINTING) + if (query_with_ui_client_id_.has_value()) { + printer_query->SetClientId(*query_with_ui_client_id_); + } +#endif auto* printer_query_ptr = printer_query.get(); printer_query_ptr->SetSettings( std::move(job_settings), diff --git a/chrome/browser/printing/printer_query_oop.cc b/chrome/browser/printing/printer_query_oop.cc index f03f09f4818d59..e07caaa5d55652 100644 --- a/chrome/browser/printing/printer_query_oop.cc +++ b/chrome/browser/printing/printer_query_oop.cc @@ -19,23 +19,6 @@ namespace printing { -namespace { - -mojom::PrintTargetType DeterminePrintTargetType( - const base::Value::Dict& job_settings) { -#if BUILDFLAG(IS_MAC) - if (job_settings.contains(kSettingOpenPDFInPreview)) { - return mojom::PrintTargetType::kExternalPreview; - } -#endif - if (job_settings.FindBool(kSettingShowSystemDialog).value_or(false)) { - return mojom::PrintTargetType::kSystemDialog; - } - return mojom::PrintTargetType::kDirectToDevice; -} - -} // namespace - PrinterQueryOop::PrinterQueryOop(content::GlobalRenderFrameHostId rfh_id) : PrinterQuery(rfh_id) {} @@ -132,7 +115,8 @@ void PrinterQueryOop::UseDefaultSettings(SettingsCallback callback) { // Any settings selected from the system dialog could need to be retained // for printing, so establish a printing context. CHECK(!context_id_.has_value()); - SendEstablishPrintingContext(*query_with_ui_client_id_); + SendEstablishPrintingContext(*query_with_ui_client_id_, + /*printer_name=*/std::string()); SendUseDefaultSettings(std::move(callback)); #else // `PrintingContextLinux::UseDefaultSettings()` is to be called prior to @@ -149,7 +133,7 @@ void PrinterQueryOop::GetSettingsWithUI(uint32_t document_page_count, SettingsCallback callback) { // Save the print target type from the settings, since this will be needed // later when printing is started. - print_target_type_ = mojom::PrintTargetType::kSystemDialog; + print_from_system_dialog_ = true; #if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) SendAskUserForSettings(document_page_count, has_selection, is_scripted, @@ -178,19 +162,47 @@ void PrinterQueryOop::UpdatePrintSettings(base::Value::Dict new_settings, // Do not take a const reference, as `new_settings` will be modified below. std::string device_name = *new_settings.FindString(kSettingDeviceName); - // Save the print target type from the settings, since this will be needed + // Remember if this is from system print dialog, since this will be needed // later when printing is started. - print_target_type_ = DeterminePrintTargetType(new_settings); - - // TODO(crbug.com/1414968): Establish a context once `UpdatePrintSettings()` - // API to PrintBackendService is updated to always require a context. + print_from_system_dialog_ = + new_settings.FindBool(kSettingShowSystemDialog).value_or(false); - VLOG(1) << "Updating print settings via service for " << device_name; + // A device name is required for printing documents. If the device name is + // empty then this is for a system print dialog, for which a destination is + // not yet known. PrintBackendServiceManager& service_mgr = PrintBackendServiceManager::GetInstance(); + PrintBackendServiceManager::ClientId client_id; + std::string printer_name; + if (print_from_system_dialog_) { + CHECK(!print_document_client_id_.has_value()); + client_id = *query_with_ui_client_id_; +#if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) && BUILDFLAG(IS_WIN) + // `PrintingContextWin::UpdatePrintSettings()` is special because it can + // invoke `AskUserForSettings()` and cause a system dialog to be displayed. + // Running a dialog causes an exit to webpage-initiated fullscreen. + // http://crbug.com/728276 + content::WebContents* web_contents = GetWebContents(); + if (web_contents && web_contents->IsFullscreen()) { + web_contents->ExitFullscreen(true); + } +#endif + } else { + // Print the document from Print Preview. + CHECK(!query_with_ui_client_id_.has_value()); + CHECK(!print_document_client_id_.has_value()); + + print_document_client_id_ = + service_mgr.RegisterPrintDocumentClient(device_name); + client_id = *print_document_client_id_; + printer_name = device_name; + } + SendEstablishPrintingContext(client_id, printer_name); + + VLOG(1) << "Updating print settings via service for " << device_name; service_mgr.UpdatePrintSettings( - query_with_ui_client_id_, device_name, std::move(new_settings), + client_id, printer_name, *context_id_, std::move(new_settings), base::BindOnce(&PrinterQueryOop::OnDidUpdatePrintSettings, weak_factory_.GetWeakPtr(), device_name, std::move(callback))); @@ -215,19 +227,22 @@ void PrinterQueryOop::OnDidUpdatePrintSettings( result = mojom::ResultCode::kSuccess; printing_context()->ApplyPrintSettings(print_settings->get_settings()); - // Query work completed, next step will be to print. - // TODO(crbug.com/1414968): Registration for printing a document will - // need to be made before calling `UpdatePrintSettings()` once it requires - // a context ID. - print_document_client_id_ = - PrintBackendServiceManager::GetInstance().RegisterPrintDocumentClient( - device_name); + if (query_with_ui_client_id_.has_value()) { + // Use the same PrintBackendService for querying and printing, so that the + // same device context can be used with both. + CHECK(!print_document_client_id_.has_value()); + print_document_client_id_ = + PrintBackendServiceManager::GetInstance() + .RegisterPrintDocumentClientReusingClientRemote( + *query_with_ui_client_id_); + } } InvokeSettingsCallback(std::move(callback), result); } void PrinterQueryOop::SendEstablishPrintingContext( - PrintBackendServiceManager::ClientId client_id) { + PrintBackendServiceManager::ClientId client_id, + const std::string& printer_name) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); DCHECK(features::kEnableOopPrintDriversJobPrint.Get()); @@ -242,11 +257,10 @@ void PrinterQueryOop::SendEstablishPrintingContext( PrintBackendServiceManager& service_mgr = PrintBackendServiceManager::GetInstance(); - context_id_ = service_mgr.EstablishPrintingContext( - client_id, /*printer_name=*/std::string() + context_id_ = service_mgr.EstablishPrintingContext(client_id, printer_name #if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) - , - parent_view + , + parent_view #endif ); } @@ -300,7 +314,8 @@ std::unique_ptr PrinterQueryOop::CreatePrintJobWorker( PrintJob* print_job) { return std::make_unique( std::move(printing_context_delegate_), std::move(printing_context_), - print_document_client_id_, context_id_, print_job, print_target_type_); + print_document_client_id_, context_id_, print_job, + print_from_system_dialog_); } } // namespace printing diff --git a/chrome/browser/printing/printer_query_oop.h b/chrome/browser/printing/printer_query_oop.h index d7e22cc07e620b..f387912568dd24 100644 --- a/chrome/browser/printing/printer_query_oop.h +++ b/chrome/browser/printing/printer_query_oop.h @@ -62,7 +62,8 @@ class PrinterQueryOop : public PrinterQuery { // Mojo support to send messages from UI thread. void SendEstablishPrintingContext( - PrintBackendServiceManager::ClientId client_id); + PrintBackendServiceManager::ClientId client_id, + const std::string& printer_name); void SendUpdatePrintSettings(const std::string& printer_name, base::Value::Dict new_settings, SettingsCallback callback); @@ -83,9 +84,7 @@ class PrinterQueryOop : public PrinterQuery { return print_document_client_id_; } - mojom::PrintTargetType print_target_type() const { - return print_target_type_; - } + bool print_from_system_dialog() const { return print_from_system_dialog_; } const absl::optional& context_id() const { @@ -93,8 +92,7 @@ class PrinterQueryOop : public PrinterQuery { } private: - mojom::PrintTargetType print_target_type_ = - mojom::PrintTargetType::kDirectToDevice; + bool print_from_system_dialog_ = false; absl::optional query_with_ui_client_id_; absl::optional print_document_client_id_; diff --git a/chrome/browser/printing/system_access_process_print_browsertest.cc b/chrome/browser/printing/system_access_process_print_browsertest.cc index 4a8e93493ab998..c4927e664d214a 100644 --- a/chrome/browser/printing/system_access_process_print_browsertest.cc +++ b/chrome/browser/printing/system_access_process_print_browsertest.cc @@ -151,10 +151,10 @@ class TestPrintJobWorkerOop : public PrintJobWorkerOop { TestPrintJobWorkerOop( std::unique_ptr printing_context_delegate, std::unique_ptr printing_context, - PrintBackendServiceManager::ClientId client_id, + absl::optional client_id, absl::optional context_id, PrintJob* print_job, - mojom::PrintTargetType print_target_type, + bool print_from_system_dialog, bool simulate_spooling_memory_errors, TestPrintJobWorkerOop::PrintCallbacks* callbacks) : PrintJobWorkerOop(std::move(printing_context_delegate), @@ -162,7 +162,7 @@ class TestPrintJobWorkerOop : public PrintJobWorkerOop { client_id, context_id, print_job, - print_target_type, + print_from_system_dialog, simulate_spooling_memory_errors), callbacks_(callbacks) {} TestPrintJobWorkerOop(const TestPrintJobWorkerOop&) = delete; @@ -254,8 +254,9 @@ class TestPrinterQueryOop : public PrinterQueryOop { PrintJob* print_job) override { return std::make_unique( std::move(printing_context_delegate_), std::move(printing_context_), - *print_document_client_id(), context_id(), print_job, - print_target_type(), simulate_spooling_memory_errors_, callbacks_); + print_document_client_id(), context_id(), print_job, + print_from_system_dialog(), simulate_spooling_memory_errors_, + callbacks_); } bool simulate_spooling_memory_errors_; @@ -1302,14 +1303,6 @@ IN_PROC_BROWSER_TEST_F(SystemAccessProcessSandboxedServicePrintBrowserTest, IN_PROC_BROWSER_TEST_P(SystemAccessProcessPrintBrowserTest, SystemPrintFromPrintPreview) { - // TODO(crbug.com/1393505) Enable OOP test coverage once underlying - // printing stack updates to support this scenario with out-of-process are - // in place. - if (GetParam() != PrintBackendFeatureVariation::kInBrowserProcess) { - GTEST_SKIP() << "Skipping test for out-of-process, which is known to crash " - "when transitioning to system print from print preview"; - } - AddPrinter("printer1"); SetPrinterNameForSubsequentContexts("printer1"); @@ -1342,22 +1335,59 @@ IN_PROC_BROWSER_TEST_P(SystemAccessProcessPrintBrowserTest, SetNumExpectedMessages(/*num=*/4); #endif // BUILDFLAG(IS_WIN) } else { - // TODO(crbug.com/1393505) Fill in expected events once the printing stack - // updates to support this scenario with out-of-process are in place. +#if BUILDFLAG(IS_WIN) + // Once the transition to system print is initiated, the expected events + // are: + // 1. A print job is started. + // 2. Rendering for 1 page of document of content. + // 3. Completes with document done. + // 4. Wait for the one print job to be destroyed, to ensure printing + // finished cleanly before completing the test. + SetNumExpectedMessages(/*num=*/4); +#else + // Once the transition to system print is initiated, the expected events + // are: + // 1. A print job is started. + // 2. Rendering for 1 page of document of content. + // 3. Completes with document done. + // 4. Wait until all processing for DidPrintDocument is known to have + // completed, to ensure printing finished cleanly before completing the + // test. + // 5. Wait for the one print job to be destroyed, to ensure printing + // finished cleanly before completing the test. + SetNumExpectedMessages(/*num=*/5); +#endif // BUILDFLAG(IS_WIN) } SystemPrintFromPreviewOnceReadyAndLoaded(/*wait_for_callback=*/true); -#if !BUILDFLAG(IS_WIN) if (GetParam() == PrintBackendFeatureVariation::kInBrowserProcess) { +#if !BUILDFLAG(IS_WIN) EXPECT_TRUE(did_get_settings_with_ui()); EXPECT_EQ(did_print_document_count(), 1); +#endif + EXPECT_EQ(*MakeUserModifiedPrintSettings("printer1"), + *document_print_settings()); } else { - // TODO(crbug.com/1393505) Fill in expectations once the printing stack - // updates to support this scenario with out-of-process are in place. - } + EXPECT_EQ(start_printing_result(), mojom::ResultCode::kSuccess); +#if BUILDFLAG(IS_WIN) + // TODO(crbug.com/1008222) Include Windows coverage of + // RenderPrintedDocument() once XPS print pipeline is added. + EXPECT_EQ(render_printed_page_result(), mojom::ResultCode::kSuccess); + EXPECT_EQ(render_printed_page_count(), 1); +#else + EXPECT_EQ(render_printed_document_result(), mojom::ResultCode::kSuccess); #endif - ASSERT_EQ(*MakeUserModifiedPrintSettings("printer1"), - *document_print_settings()); + EXPECT_EQ(document_done_result(), mojom::ResultCode::kSuccess); +#if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + EXPECT_EQ(*MakeUserModifiedPrintSettings("printer1"), + *document_print_settings()); +#else + // TODO(crbug.com/1414968): Update the expectation once system print + // settings are properly reflected at start of job print. + EXPECT_NE(*MakeUserModifiedPrintSettings("printer1"), + *document_print_settings()); +#endif + } EXPECT_EQ(error_dialog_shown_count(), 0u); EXPECT_EQ(print_job_destruction_count(), 1); } @@ -1410,12 +1440,12 @@ IN_PROC_BROWSER_TEST_F(SystemAccessProcessSandboxedServicePrintBrowserTest, #if BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) EXPECT_EQ(use_default_settings_result(), mojom::ResultCode::kSuccess); EXPECT_EQ(ask_user_for_settings_result(), mojom::ResultCode::kSuccess); - ASSERT_EQ(*MakeUserModifiedPrintSettings("printer1"), + EXPECT_EQ(*MakeUserModifiedPrintSettings("printer1"), *document_print_settings()); #else - // TODO(crbug.com/1414968) Correct expectation once system print settings - // are properly reflected at start of job print. - ASSERT_NE(*MakeUserModifiedPrintSettings("printer1"), + // TODO(crbug.com/1414968): Update the expectation once system print + // settings are properly reflected at start of job print. + EXPECT_NE(*MakeUserModifiedPrintSettings("printer1"), *document_print_settings()); #endif EXPECT_EQ(start_printing_result(), mojom::ResultCode::kSuccess); diff --git a/chrome/services/printing/print_backend_service_impl.cc b/chrome/services/printing/print_backend_service_impl.cc index c40bab2b8321b3..3d34fa67dee5c5 100644 --- a/chrome/services/printing/print_backend_service_impl.cc +++ b/chrome/services/printing/print_backend_service_impl.cc @@ -159,12 +159,10 @@ class DocumentContainer { public: DocumentContainer(std::unique_ptr context_delegate, std::unique_ptr context, - scoped_refptr document, - mojom::PrintTargetType target_type) + scoped_refptr document) : context_delegate_(std::move(context_delegate)), context_(std::move(context)), - document_(document), - target_type_(target_type) {} + document_(document) {} ~DocumentContainer() = default; @@ -192,9 +190,6 @@ class DocumentContainer { scoped_refptr document_; - // Parameter required for the delayed call to `UpdatePrinterSettings()`. - mojom::PrintTargetType target_type_; - // Ensure all interactions for this document are issued from the same runner. SEQUENCE_CHECKER(sequence_checker_); }; @@ -204,31 +199,7 @@ mojom::ResultCode DocumentContainer::StartPrintingReadyDocument() { DVLOG(1) << "Start printing for document " << document_->cookie(); - // With out-of-process printing the printer settings no longer get updated - // from `PrintingContext::UpdatePrintSettings()`, so we need to apply that - // now to our new context. - // TODO(crbug.com/1245679): Drop calls to update context once - // `UpdatePrinterSettings()` supports use with an established context. - PrintingContext::PrinterSettings printer_settings { -#if BUILDFLAG(IS_MAC) - .external_preview = - target_type_ == mojom::PrintTargetType::kExternalPreview, -#endif - .show_system_dialog = target_type_ == mojom::PrintTargetType::kSystemDialog, -#if BUILDFLAG(IS_WIN) - .page_count = 0, -#endif - }; - context_->ApplyPrintSettings(document_->settings()); - mojom::ResultCode result = context_->UpdatePrinterSettings(printer_settings); - if (result != mojom::ResultCode::kSuccess) { - DLOG(ERROR) << "Failure updating printer settings for document " - << document_->cookie() << ", error: " << result; - context_->Cancel(); - return result; - } - - result = context_->NewDocument(document_->name()); + mojom::ResultCode result = context_->NewDocument(document_->name()); if (result != mojom::ResultCode::kSuccess) { DLOG(ERROR) << "Failure initializing new document " << document_->cookie() << ", error: " << result; @@ -675,6 +646,7 @@ void PrintBackendServiceImpl::AskUserForSettings( #endif // BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) void PrintBackendServiceImpl::UpdatePrintSettings( + uint32_t context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback) { DCHECK(print_backend_); @@ -698,12 +670,8 @@ void PrintBackendServiceImpl::UpdatePrintSettings( } #endif // BUILDFLAG(IS_LINUX) && BUILDFLAG(USE_CUPS) - // Use a one-time `PrintingContext` to do the update to print settings. - // Intentionally do not cache this context here since the process model does - // not guarantee that that printing will return to this same process when - // `StartPrinting()` gets called. - std::unique_ptr context = - PrintingContext::Create(&context_delegate_, /*skip_system_calls=*/false); + PrintingContext* context = GetPrintingContext(context_id); + CHECK(context) << "No context found for id " << context_id; mojom::ResultCode result = context->UpdatePrintSettings(std::move(job_settings)); @@ -712,16 +680,17 @@ void PrintBackendServiceImpl::UpdatePrintSettings( return; } - std::move(callback).Run(mojom::PrintSettingsResult::NewSettings( - *context->TakeAndResetSettings())); + std::move(callback).Run( + mojom::PrintSettingsResult::NewSettings(context->settings())); } void PrintBackendServiceImpl::StartPrinting( uint32_t context_id, int document_cookie, const std::u16string& document_name, - mojom::PrintTargetType target_type, - const PrintSettings& settings, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + const absl::optional& settings, +#endif mojom::PrintBackendService::StartPrintingCallback callback) { #if BUILDFLAG(IS_CHROMEOS) && BUILDFLAG(USE_CUPS) CupsConnectionPool* connection_pool = CupsConnectionPool::GetInstance(); @@ -747,18 +716,22 @@ void PrintBackendServiceImpl::StartPrinting( std::unique_ptr context_container = std::move(item->second); persistent_printing_contexts_.erase(item); +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + if (settings) { + // Apply the settings from the in-browser system dialog to the context. + context_container->context->ApplyPrintSettings(*settings); + } +#endif + // Save all the document settings for use through the print job, until the // time that this document can complete printing. Track the order of // received documents with position in `documents_`. - // TODO(crbug.com/1414968): Use `context_container->context->settings()` - // instead of `settings` once `UpdatePrintSettings()` uses an established - // context. auto document = base::MakeRefCounted( - std::make_unique(settings), document_name, - document_cookie); + std::make_unique(context_container->context->settings()), + document_name, document_cookie); base::SequenceBound document_container( GetPrintingTaskRunner(), std::move(context_container->delegate), - std::move(context_container->context), document, target_type); + std::move(context_container->context), document); documents_.push_back(std::make_unique( document_cookie, std::move(document_container), std::move(callback))); DocumentHelper& document_helper = *documents_.back(); diff --git a/chrome/services/printing/print_backend_service_impl.h b/chrome/services/printing/print_backend_service_impl.h index 68c0b10a91206b..de215093473f5b 100644 --- a/chrome/services/printing/print_backend_service_impl.h +++ b/chrome/services/printing/print_backend_service_impl.h @@ -176,6 +176,7 @@ class PrintBackendServiceImpl : public mojom::PrintBackendService { mojom::PrintBackendService::AskUserForSettingsCallback callback) override; #endif void UpdatePrintSettings( + uint32_t context_id, base::Value::Dict job_settings, mojom::PrintBackendService::UpdatePrintSettingsCallback callback) override; @@ -183,8 +184,9 @@ class PrintBackendServiceImpl : public mojom::PrintBackendService { uint32_t context_id, int document_cookie, const std::u16string& document_name, - mojom::PrintTargetType target_type, - const PrintSettings& settings, +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + const absl::optional& settings, +#endif mojom::PrintBackendService::StartPrintingCallback callback) override; #if BUILDFLAG(IS_WIN) void RenderPrintedPage( @@ -255,10 +257,6 @@ class PrintBackendServiceImpl : public mojom::PrintBackendService { base::flat_map> persistent_printing_contexts_; - // TODO(crbug.com/1414968): Delete this once callers switch to complete - // local contexts or using `persistent_printing_contexts_`. - PrintingContextDelegate context_delegate_; - // Want all callbacks and document helper sequence manipulations to be made // from main thread, not a thread runner. SEQUENCE_CHECKER(main_sequence_checker_); diff --git a/chrome/services/printing/public/mojom/print_backend_service.mojom b/chrome/services/printing/public/mojom/print_backend_service.mojom index 6bd29baace8a9e..e119ff03e079db 100644 --- a/chrome/services/printing/public/mojom/print_backend_service.mojom +++ b/chrome/services/printing/public/mojom/print_backend_service.mojom @@ -58,14 +58,6 @@ union PrintSettingsResult { ResultCode result_code; }; -// The type of printing interface to target. -enum PrintTargetType { - kDirectToDevice, - kSystemDialog, - [EnableIf=is_mac] - kExternalPreview, -}; - // Hosts the PrintBackendService but does so without sandboxing the service - // this is required if print drivers need UI access or cannot otherwise // operate in the normal sandbox. There is a 1:1 relationship and `service` @@ -170,11 +162,15 @@ interface PrintBackendService { bool is_scripted) => (PrintSettingsResult settings); - // Generates a print settings object based upon the job settings used with - // the device settings. The driver to be used is expected to be identified - // in the `job_settings` map by an entry with key - // `printing::kSettingDeviceName`. - UpdatePrintSettings(mojo_base.mojom.DictionaryValue job_settings) + // Updates the indicated printing context based upon the job settings used + // with the device settings. The driver to be used is expected to be + // identified in the `job_settings` map by an entry with key + // `printing::kSettingDeviceName`. `context_id` must be for a context + // previously created by a call to `EstablishPrintingContext()`. The updated + // print settings remain in the context for use when a system dialog is + // initiated and/or printing of a document is started. + UpdatePrintSettings(uint32 context_id, + mojo_base.mojom.DictionaryValue job_settings) => (PrintSettingsResult settings); // Submit the document identified by `document_cookie` to be printed. @@ -186,11 +182,17 @@ interface PrintBackendService { // Starting the device initialization may be delayed if it needs to wait for // prior print jobs to complete before a connection to the system to become // available. + // For platforms which do not support an out-of-process system print dialog, + // when doing system print the settings are specified from the system dialog + // that is invoked from the browser process. The settings are collected from + // that and then are provided at start of printing via `settings`. This is + // optional since such settings are not necessary when printing from Print + // Preview, where the print settings are provided via `UpdatePrintSettings()`. StartPrinting(uint32 context_id, int32 document_cookie, mojo_base.mojom.String16 document_name, - PrintTargetType target_type, - PrintSettings settings) + [EnableIfNot=enable_oop_basic_print_dialog] + PrintSettings? settings) => (ResultCode result_code); // Render the indicated page as part of print job for `document_cookie`. diff --git a/printing/printing_context.cc b/printing/printing_context.cc index 3a9e75c229f028..a6ce93f23cf915 100644 --- a/printing/printing_context.cc +++ b/printing/printing_context.cc @@ -203,6 +203,17 @@ mojom::ResultCode PrintingContext::UpdatePrintSettingsFromPOD( void PrintingContext::ApplyPrintSettings(const PrintSettings& settings) { *settings_ = settings; + +#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG) + if (skip_system_calls()) { + return; + } + + // TODO(crbug.com/1414968): System print dialog settings from the browser + // require platform-specific handling to be applied to device context. + NOTIMPLEMENTED() + << "Apply system dialog settings to device context not supported yet"; +#endif } } // namespace printing