From 7511633616231fd6c642988bec53afe4be91e2a4 Mon Sep 17 00:00:00 2001 From: Vladislav Kuzkokov Date: Tue, 20 Aug 2019 15:40:14 +0000 Subject: [PATCH] [Printing] Don't create unnecessary PrintSettings instances. Make |PrintingContext::settings_| null before settings are created or after ownership is passed to PrinterQuery. Bug: 964948 Change-Id: I0cff0b5b460ecb3424ff0cb07e5849bed3ffe51e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1678182 Reviewed-by: Lei Zhang Commit-Queue: Vladislav Kuzkokov Cr-Commit-Position: refs/heads/master@{#688541} --- chrome/browser/printing/print_job_unittest.cc | 2 +- chrome/browser/printing/print_job_worker.cc | 3 +- printing/print_settings.cc | 38 +------------------ printing/print_settings.h | 37 +++++++++--------- printing/printing_context.cc | 27 +++++++------ printing/printing_context.h | 13 +++++-- printing/printing_context_android.cc | 3 +- printing/printing_context_chromeos.cc | 2 +- printing/printing_context_linux.cc | 2 +- printing/printing_context_mac.mm | 3 +- printing/printing_context_no_system_dialog.cc | 2 +- .../printing_context_system_dialog_win.cc | 4 +- printing/printing_context_win.cc | 3 +- printing/printing_context_win_unittest.cc | 2 + 14 files changed, 58 insertions(+), 83 deletions(-) diff --git a/chrome/browser/printing/print_job_unittest.cc b/chrome/browser/printing/print_job_unittest.cc index 71811d5def1f52..44e928a9c180d6 100644 --- a/chrome/browser/printing/print_job_unittest.cc +++ b/chrome/browser/printing/print_job_unittest.cc @@ -57,7 +57,7 @@ class TestQuery : public PrinterQuery { auto worker = std::make_unique(); EXPECT_TRUE(worker->Start()); worker->printing_context()->UseDefaultSettings(); - SetSettingsForTest(worker->printing_context()->TakeAndResetSettings()); + SetSettingsForTest(worker->printing_context()->ExtractSettings()); return std::move(worker); } diff --git a/chrome/browser/printing/print_job_worker.cc b/chrome/browser/printing/print_job_worker.cc index 13f9d7af3ae796..7e10857fc8eba6 100644 --- a/chrome/browser/printing/print_job_worker.cc +++ b/chrome/browser/printing/print_job_worker.cc @@ -159,6 +159,7 @@ void PrintJobWorker::GetSettings(bool ask_user_for_settings, DCHECK(task_runner_->RunsTasksInCurrentSequence()); DCHECK_EQ(page_number_, PageNumber::npos()); + printing_context_->ResetSettings(); printing_context_->set_margin_type(margin_type); printing_context_->set_is_modifiable(is_modifiable); @@ -221,7 +222,7 @@ void PrintJobWorker::UpdatePrintSettingsFromPOD( void PrintJobWorker::GetSettingsDone(SettingsCallback callback, PrintingContext::Result result) { - std::move(callback).Run(printing_context_->TakeAndResetSettings(), result); + std::move(callback).Run(printing_context_->ExtractSettings(), result); } void PrintJobWorker::GetSettingsWithUI(int document_page_count, diff --git a/printing/print_settings.cc b/printing/print_settings.cc index 5cc1b554218ca5..4524ea0b2fca04 100644 --- a/printing/print_settings.cc +++ b/printing/print_settings.cc @@ -149,46 +149,10 @@ bool IsColorModelSelected(int color_mode) { // Global SequenceNumber used for generating unique cookie values. static base::AtomicSequenceNumber cookie_seq; -PrintSettings::PrintSettings() { - Clear(); -} +PrintSettings::PrintSettings() = default; PrintSettings::~PrintSettings() = default; -void PrintSettings::Clear() { - ranges_.clear(); - selection_only_ = false; - margin_type_ = DEFAULT_MARGINS; - title_.clear(); - url_.clear(); - display_header_footer_ = false; - should_print_backgrounds_ = false; - collate_ = false; - color_ = UNKNOWN_COLOR_MODEL; - copies_ = 0; - duplex_mode_ = UNKNOWN_DUPLEX_MODE; - device_name_.clear(); - requested_media_ = RequestedMedia(); - page_setup_device_units_.Clear(); - dpi_ = gfx::Size(); - scale_factor_ = 1.0f; - rasterize_pdf_ = false; - landscape_ = false; - supports_alpha_blend_ = true; -#if defined(OS_WIN) - print_text_with_gdi_ = false; - printer_type_ = PrintSettings::PrinterType::TYPE_NONE; -#endif - is_modifiable_ = true; - pages_per_sheet_ = 1; -#if defined(OS_CHROMEOS) - send_user_info_ = false; - username_.clear(); - job_title_.clear(); - pin_value_.clear(); -#endif // defined(OS_CHROMEOS) -} - void PrintSettings::SetPrinterPrintableArea( const gfx::Size& physical_size_device_units, const gfx::Rect& printable_area_device_units, diff --git a/printing/print_settings.h b/printing/print_settings.h index 47c0a636966fcd..fd657cc0667525 100644 --- a/printing/print_settings.h +++ b/printing/print_settings.h @@ -62,9 +62,6 @@ class PRINTING_EXPORT PrintSettings { PrintSettings(); ~PrintSettings(); - // Reinitialize the settings to the default values. - void Clear(); - void SetCustomMargins(const PageMargins& requested_margins_in_points); const PageMargins& requested_custom_margins_in_points() const { return requested_custom_margins_in_points_; @@ -214,32 +211,32 @@ class PRINTING_EXPORT PrintSettings { PageRanges ranges_; // Indicates if the user only wants to print the current selection. - bool selection_only_; + bool selection_only_ = false; // Indicates what kind of margins should be applied to the printable area. - MarginType margin_type_; + MarginType margin_type_ = DEFAULT_MARGINS; // Strings to be printed as headers and footers if requested by the user. base::string16 title_; base::string16 url_; // True if the user wants headers and footers to be displayed. - bool display_header_footer_; + bool display_header_footer_ = false; // True if the user wants to print CSS backgrounds. - bool should_print_backgrounds_; + bool should_print_backgrounds_ = false; // True if the user wants to print with collate. - bool collate_; + bool collate_ = false; // True if the user wants to print with collate. - ColorModel color_; + ColorModel color_ = UNKNOWN_COLOR_MODEL; // Number of copies user wants to print. - int copies_; + int copies_ = 0; // Duplex type user wants to use. - DuplexMode duplex_mode_; + DuplexMode duplex_mode_ = UNKNOWN_DUPLEX_MODE; // Printer device name as opened by the OS. base::string16 device_name_; @@ -256,35 +253,35 @@ class PRINTING_EXPORT PrintSettings { gfx::Size dpi_; // Scale factor - double scale_factor_; + double scale_factor_ = 1.0; // True if PDF should be printed as a raster PDF - bool rasterize_pdf_; + bool rasterize_pdf_ = false; // Is the orientation landscape or portrait. - bool landscape_; + bool landscape_ = false; // True if this printer supports AlphaBlend. - bool supports_alpha_blend_; + bool supports_alpha_blend_ = true; #if defined(OS_WIN) // True to print text with GDI. - bool print_text_with_gdi_; + bool print_text_with_gdi_ = false; - PrinterType printer_type_; + PrinterType printer_type_ = PrinterType::TYPE_NONE; #endif - bool is_modifiable_; + bool is_modifiable_ = true; // If margin type is custom, this is what was requested. PageMargins requested_custom_margins_in_points_; // Number of pages per sheet. - int pages_per_sheet_; + int pages_per_sheet_ = 1; #if defined(OS_CHROMEOS) // Whether to send user info. - bool send_user_info_; + bool send_user_info_ = false; // Username if it's required by the printer. std::string username_; diff --git a/printing/printing_context.cc b/printing/printing_context.cc index cd5c27c87df175..1a3ee651b674d5 100644 --- a/printing/printing_context.cc +++ b/printing/printing_context.cc @@ -20,10 +20,7 @@ const float kCloudPrintMarginInch = 0.25; } PrintingContext::PrintingContext(Delegate* delegate) - : settings_(std::make_unique()), - delegate_(delegate), - in_print_job_(false), - abort_printing_(false) { + : delegate_(delegate), in_print_job_(false), abort_printing_(false) { DCHECK(delegate_); } @@ -46,24 +43,30 @@ const PrintSettings& PrintingContext::settings() const { return *settings_; } -void PrintingContext::ResetSettings() { +void PrintingContext::ResetSettingsImpl(bool create_empty) { ReleaseContext(); - settings_->Clear(); + settings_ = create_empty ? std::make_unique() : nullptr; in_print_job_ = false; abort_printing_ = false; } -std::unique_ptr PrintingContext::TakeAndResetSettings() { - std::unique_ptr result = std::move(settings_); - settings_ = std::make_unique(); - return result; +void PrintingContext::ResetSettings() { + ResetSettingsImpl(true); +} + +void PrintingContext::DeleteSettings() { + ResetSettingsImpl(false); +} + +std::unique_ptr PrintingContext::ExtractSettings() { + return std::move(settings_); } PrintingContext::Result PrintingContext::OnError() { Result result = abort_printing_ ? CANCEL : FAILED; - ResetSettings(); + DeleteSettings(); return result; } @@ -158,7 +161,7 @@ PrintingContext::Result PrintingContext::UpdatePrintSettings( #if defined(OS_CHROMEOS) PrintingContext::Result PrintingContext::UpdatePrintSettingsFromPOD( std::unique_ptr job_settings) { - ResetSettings(); + DeleteSettings(); settings_ = std::move(job_settings); return UpdatePrinterSettings(false /* external_preview */, diff --git a/printing/printing_context.h b/printing/printing_context.h index 6a5a7c90ef5ba8..f17ebf122e4f57 100644 --- a/printing/printing_context.h +++ b/printing/printing_context.h @@ -127,16 +127,19 @@ class PRINTING_EXPORT PrintingContext { const PrintSettings& settings() const; - std::unique_ptr TakeAndResetSettings(); + std::unique_ptr ExtractSettings(); int job_id() const { return job_id_; } - protected: - explicit PrintingContext(Delegate* delegate); - // Reinitializes the settings for object reuse. void ResetSettings(); + // Cleans up object settings. + void DeleteSettings(); + + protected: + explicit PrintingContext(Delegate* delegate); + // Does bookkeeping when an error occurs. PrintingContext::Result OnError(); @@ -156,6 +159,8 @@ class PRINTING_EXPORT PrintingContext { int job_id_; private: + void ResetSettingsImpl(bool create_empty); + DISALLOW_COPY_AND_ASSIGN(PrintingContext); }; diff --git a/printing/printing_context_android.cc b/printing/printing_context_android.cc index 6bad14cd792225..dcd1ae5c87d6ed 100644 --- a/printing/printing_context_android.cc +++ b/printing/printing_context_android.cc @@ -121,6 +121,7 @@ void PrintingContextAndroid::AskUserForSettingsReply( std::move(callback_).Run(FAILED); return; } + ResetSettings(); // We use device name variable to store the file descriptor. This is hacky // but necessary. Since device name is not necessary for the upstream @@ -245,7 +246,7 @@ PrintingContext::Result PrintingContextAndroid::DocumentDone() { return CANCEL; DCHECK(in_print_job_); - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_chromeos.cc b/printing/printing_context_chromeos.cc index b48cbe13e17f54..312bcd5ce6209b 100644 --- a/printing/printing_context_chromeos.cc +++ b/printing/printing_context_chromeos.cc @@ -365,7 +365,7 @@ PrintingContext::Result PrintingContextChromeos::DocumentDone() { return OnError(); } - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_linux.cc b/printing/printing_context_linux.cc index 38d9e8e9527b1e..4bdf17622f8a28 100644 --- a/printing/printing_context_linux.cc +++ b/printing/printing_context_linux.cc @@ -172,7 +172,7 @@ PrintingContext::Result PrintingContextLinux::DocumentDone() { return CANCEL; DCHECK(in_print_job_); - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_mac.mm b/printing/printing_context_mac.mm index 121b01018f4431..5a11075ad91aac 100644 --- a/printing/printing_context_mac.mm +++ b/printing/printing_context_mac.mm @@ -161,6 +161,7 @@ PMPaper MatchPaper(CFArrayRef paper_list, PrintingContext::Result PrintingContextMac::UseDefaultSettings() { DCHECK(!in_print_job_); + ResetSettings(); print_info_.reset([[NSPrintInfo sharedPrintInfo] copy]); settings_->set_ranges(GetPageRangesFromPrintInfo()); InitPrintSettingsFromPrintInfo(); @@ -485,7 +486,7 @@ PMPaper MatchPaper(CFArrayRef paper_list, if (status != noErr) OnError(); - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_no_system_dialog.cc b/printing/printing_context_no_system_dialog.cc index 106314b1399c51..a55a5c8b427f74 100644 --- a/printing/printing_context_no_system_dialog.cc +++ b/printing/printing_context_no_system_dialog.cc @@ -123,7 +123,7 @@ PrintingContext::Result PrintingContextNoSystemDialog::DocumentDone() { return CANCEL; DCHECK(in_print_job_); - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_system_dialog_win.cc b/printing/printing_context_system_dialog_win.cc index a53e4045bdc8a4..b3f8a26442cbb8 100644 --- a/printing/printing_context_system_dialog_win.cc +++ b/printing/printing_context_system_dialog_win.cc @@ -67,7 +67,7 @@ void PrintingContextSystemDialogWin::AskUserForSettings( } if (ShowPrintDialog(&dialog_options) != S_OK) { - ResetSettings(); + DeleteSettings(); std::move(callback).Run(FAILED); return; } @@ -113,7 +113,7 @@ bool PrintingContextSystemDialogWin::InitializeSettingsWithRanges( if (!(GetDeviceCaps(context(), RASTERCAPS) & RC_STRETCHDIB) || !(GetDeviceCaps(context(), RASTERCAPS) & RC_BITMAP64)) { NOTREACHED(); - ResetSettings(); + DeleteSettings(); return false; } diff --git a/printing/printing_context_win.cc b/printing/printing_context_win.cc index 7115067c21c5e4..011da75b1185c9 100644 --- a/printing/printing_context_win.cc +++ b/printing/printing_context_win.cc @@ -71,6 +71,7 @@ PrintingContext::Result PrintingContextWin::UseDefaultSettings() { scoped_refptr backend = PrintBackend::CreateInstance(nullptr); base::string16 default_printer = base::UTF8ToWide(backend->GetDefaultPrinterName()); + ResetSettings(); if (!default_printer.empty()) { ScopedPrinterHandle printer; if (printer.OpenPrinterWithName(default_printer.c_str())) { @@ -321,7 +322,7 @@ PrintingContext::Result PrintingContextWin::DocumentDone() { if (EndDoc(context_) <= 0) return OnError(); - ResetSettings(); + DeleteSettings(); return OK; } diff --git a/printing/printing_context_win_unittest.cc b/printing/printing_context_win_unittest.cc index 277a2ba4d12fa1..f078acd6769bf5 100644 --- a/printing/printing_context_win_unittest.cc +++ b/printing/printing_context_win_unittest.cc @@ -149,6 +149,7 @@ TEST_F(PrintingContextTest, PrintAll) { base::test::TaskEnvironment task_environment; MockPrintingContextWin context(this); + context.ResetSettings(); context.AskUserForSettings( 123, false, false, base::BindOnce(&PrintingContextTest::PrintSettingsCallback, @@ -164,6 +165,7 @@ TEST_F(PrintingContextTest, Color) { base::test::TaskEnvironment task_environment; MockPrintingContextWin context(this); + context.ResetSettings(); context.AskUserForSettings( 123, false, false, base::BindOnce(&PrintingContextTest::PrintSettingsCallback,