Skip to content

Commit

Permalink
Alter UpdatePrintSettings API to use context ID
Browse files Browse the repository at this point in the history 
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 <thestig@chromium.org>
Commit-Queue: Alan Screen <awscreen@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1121921}
  • Loading branch information
Alan Screen authored and Chromium LUCI CQ committed Mar 24, 2023
1 parent fb290bb commit fd714c1
Show file tree
Hide file tree
Showing 15 changed files with 291 additions and 221 deletions.
101 changes: 60 additions & 41 deletions chrome/browser/printing/print_backend_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(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();
Expand Down Expand Up @@ -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<int>(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);
Expand All @@ -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<int>(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)
Expand All @@ -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<mojom::ResultCode> result = RenderPageAndWait();
EXPECT_EQ(result, mojom::ResultCode::kSuccess);
Expand All @@ -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<mojom::ResultCode> result = RenderDocumentAndWait();
EXPECT_EQ(result, mojom::ResultCode::kSuccess);
Expand All @@ -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.
Expand All @@ -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();
}
Expand Down
30 changes: 19 additions & 11 deletions chrome/browser/printing/print_backend_service_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -341,16 +341,20 @@ void PrintBackendServiceManager::AskUserForSettings(
#endif // BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG)

void PrintBackendServiceManager::UpdatePrintSettings(
absl::optional<ClientId> 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,
Expand All @@ -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)));
}
Expand All @@ -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<PrintSettings> settings,
#endif
mojom::PrintBackendService::StartPrintingCallback callback) {
CallbackContext context;
auto& service = GetServiceAndCallbackContextForPrintDocumentClient(
Expand All @@ -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)));
}
Expand Down
34 changes: 18 additions & 16 deletions chrome/browser/printing/print_backend_service_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ClientId> 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<PrintSettings> settings,
#endif
mojom::PrintBackendService::StartPrintingCallback callback);
#if BUILDFLAG(IS_WIN)
void RenderPrintedPage(
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/printing/print_backend_service_test_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,16 @@ void PrintBackendServiceTestImpl::FetchCapabilities(
}

void PrintBackendServiceTestImpl::UpdatePrintSettings(
uint32_t context_id,
base::Value::Dict job_settings,
mojom::PrintBackendService::UpdatePrintSettingsCallback callback) {
if (terminate_receiver_) {
TerminateConnection();
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)
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/printing/print_backend_service_test_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
22 changes: 15 additions & 7 deletions chrome/browser/printing/print_job_worker_oop.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ PrintJobWorkerOop::PrintJobWorkerOop(
absl::optional<PrintBackendServiceManager::ClientId> client_id,
absl::optional<PrintBackendServiceManager::ContextId> 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(
Expand All @@ -70,15 +70,15 @@ PrintJobWorkerOop::PrintJobWorkerOop(
absl::optional<PrintBackendServiceManager::ClientId> client_id,
absl::optional<PrintBackendServiceManager::ContextId> 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),
print_job),
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());
Expand Down Expand Up @@ -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.";
Expand Down Expand Up @@ -429,10 +429,18 @@ void PrintJobWorkerOop::SendStartPrinting(const std::string& device_name,
SendEstablishPrintingContext();
}

#if !BUILDFLAG(ENABLE_OOP_BASIC_PRINT_DIALOG)
absl::optional<PrintSettings> 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()));
}
Expand Down
Loading

0 comments on commit fd714c1

Please sign in to comment.