Add Postscript Printing

Add Postscript generation support to Chromium. See https://codereview.chromium.org/2612243005/ and https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
change printing type from boolean for GDI to an enum in PdfRenderSettings:
https://codereview.chromium.org/2640033005/#

Review-Url: https://codereview.chromium.org/2633573002
Cr-Commit-Position: refs/heads/master@{#447440}
Committed: https://chromium.googlesource.com/chromium/src/+/58bc882e11a2ef3d8f446dff391b953bc08809bf

[rbpotter, 2017-01-12 23:01:35]

Description was changed from

==========
Add Postscript generation hookins to Chromium to support PS Printing.

BUG=614988
==========

to

==========
Add Postscript generation hookins to Chromium to support PS Printing.

BUG=614988
==========

[rbpotter, 2017-01-13 01:25:28]

Patchset #2 (id:20001) has been deleted

[rbpotter, 2017-01-13 01:29:59]

Description was changed from

==========
Add Postscript generation hookins to Chromium to support PS Printing.

BUG=614988
==========

to

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

BUG=614988
==========

[rbpotter, 2017-01-13 01:30:00]

rbpotter@chromium.org changed reviewers:
+ dsinclair@chromium.org, vitalybuka@chromium.org

[rbpotter, 2017-01-13 01:33:18]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 01:34:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-13 01:39:17]

Chromium patchset to add postscript generation. This is for Windows only. Pdfium
patches have already landed - see issue description for links.

[commit-bot: I haz the power, 2017-01-13 01:51:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 01:51:07]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[rbpotter, 2017-01-13 01:59:07]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 01:59:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-13 02:21:29]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 02:22:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 04:13:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 04:13:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[rbpotter, 2017-01-13 16:58:22]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 16:59:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 19:36:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 19:36:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dsinclair, 2017-01-17 17:10:00]

https://codereview.chromium.org/2633573002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2633573002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15596: +        Enable postscript generation
in place of emf generation when printing to postscript capable printers.
Should PostScript have casing and EMF be upper case?

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:304: converter_.reset();
Shoudl this be .reset(PdfToPostScriptConverter::CreateDefault()) so the convert
is ready to go again instead of being a nullptr?

We'll currently crash if Start is called a second time with this state object,
or is that just not allowed?

https://codereview.chromium.org/2633573002/diff/100001/chrome/utility/printin...
File chrome/utility/printing_handler.h (left):

https://codereview.chromium.org/2633573002/diff/100001/chrome/utility/printin...
chrome/utility/printing_handler.h:50: #endif  // ENABLE_PRINT_PREVIEW
Why remove the comments?

[Vitaly Buka (NO REVIEWS), 2017-01-17 18:42:22]

Why are we doing this?
This is a large patch but attached issues seems irrelevant.

[Vitaly Buka (NO REVIEWS), 2017-01-17 18:59:38]

On 2017/01/17 18:42:22, Vitaly Buka wrote:
> Why are we doing this?
> This is a large patch but attached issues seems irrelevant.

I see now why this is relevant.

[Vitaly Buka (NO REVIEWS), 2017-01-17 19:29:16]

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:537: public:
To much of duplicated code.
Could you just do public LazyEmf and override SafePlayback.

Maybe you can create separate CL with refactoring, so this CL has less
duplicated code?

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.h (right):

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.h:19: public:
I'd recommend to submit patches as small as possible. Even such walk-by fixes
should be separate tiny patches. With git it's easy to do so.

It will simplify reviews, and make later debugging (using bisect) much easier.

https://codereview.chromium.org/2633573002/diff/100001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.h:44: class
PdfToPostScriptConverter {
why not separate pair of h/cc file
git probably would detect copied files, so we can see how they are actually
different

https://codereview.chromium.org/2633573002/diff/100001/printing/print_setting...
File printing/print_settings_initializer_win.cc (right):

https://codereview.chromium.org/2633573002/diff/100001/printing/print_setting...
printing/print_settings_initializer_win.cc:15: bool HasEscapeSupprt(HDC hdc,
DWORD escape) {
Same here.
Extraction of IsTechnology function deserve separate patch.
You are touching working code. If you have some typo here it would be easier to
localize the bug in future.

[Vitaly Buka (NO REVIEWS), 2017-01-17 19:30:15]

Also this patch is going to have a lot of conflicts with
https://codereview.chromium.org/2477283002/

[rbpotter, 2017-01-17 20:20:53]

On 2017/01/17 19:30:15, Vitaly Buka wrote:
> Also this patch is going to have a lot of conflicts with
> https://codereview.chromium.org/2477283002/

FYI: Will be breaking this up into several different patches
per Vitaly's recommendation: one for the print settings
initializer refactor, one to refactor the PdfToEmfConverter,
another for everything in pdf/* and probably another to
switch the gdi printing parameter from a boolean to an enum.
Will then merge changes from all of these small patches and
the patch Vitaly linked above and upload a new patchset,
hopefully at least somewhat smaller.

[rbpotter, 2017-01-19 01:16:09]

Description was changed from

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

BUG=614988
==========

to

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/

BUG=614988
==========

[rbpotter, 2017-01-19 23:52:26]

Description was changed from

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/

BUG=614988
==========

to

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
==========

[rbpotter, 2017-01-20 00:46:45]

Description was changed from

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
==========

to

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
change printing type from boolean for GDI to an enum in PdfRenderSettings:
https://codereview.chromium.org/2640033005/#
==========

[dsinclair, 2017-01-24 13:51:30]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org

[dsinclair, 2017-01-24 13:51:31]

I'm removing myself from the reviewers line for now as I'm not sure if I'm
supposed to be reviewing or not. Please re-add me when you're ready for me to
take another look.

[rbpotter, 2017-01-26 19:59:12]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 20:00:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 20:03:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 20:03:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rbpotter, 2017-01-26 21:40:22]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 21:41:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-26 22:25:15]

All refactors have been merged in, so PTAL at the new postscript additions. Will
also merge in the change to PdfRenderSettings later today.

[commit-bot: I haz the power, 2017-01-26 23:12:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 23:12:04]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Vitaly Buka (NO REVIEWS), 2017-01-27 00:38:49]

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:417: Emf::Enumerator
emf_enum(emf, nullptr, nullptr);
Why postscript is encoded as GDI comment into EMF?
Why not to send as regular postscript file?

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:702: void
PdfToPostScriptUtilityProcessHostClient::OnPageDone(bool success) {
Maybe we can have single PdfConverterUtilityProcessHostClient for both
converters?

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:751:
DCHECK(!utility_client_.get());
There is also
https://cs.chromium.org/chromium/src/chrome/service/service_utility_process_h...

which may also benefit from postscript printing

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:322: is_postscript_ ? 0.0f : scale_factor,
I guess better to remove conditiond and just use
scale_factor
it should be 0 for postscript.

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:324: is_postscript_ ? gfx::Rect() :
pdf_conversion_state_->content_area());
why do you need condition here?
PdfConversionState was created with
gfx::Size(), gfx::Rect(),

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:338: is_postscript_ = true;
looks like is_postscript_ can be removed

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/print_job.h (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.h:165: bool is_postscript_;
I don't see where this is initialized

I'd put initializer here.
bool is_postscript_ = false;

https://codereview.chromium.org/2633573002/diff/140001/printing/print_setting...
File printing/print_settings_initializer_win.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/printing/print_setting...
printing/print_settings_initializer_win.cc:125:
print_settings->set_printer_is_ps2(true);
Looks like printer_is* flags exclusive
probably cleaner to replace all 3 with enum

https://codereview.chromium.org/2633573002/diff/140001/printing/printed_docum...
File printing/printed_document.h (left):

https://codereview.chromium.org/2633573002/diff/140001/printing/printed_docum...
printing/printed_document.h:51: #if defined(OS_WIN)
How is this related to the patch?
PS printing is windows feature, why you enable this for other OSes?

Separate patch?

[rbpotter, 2017-01-27 03:18:57]

Patchset #8 (id:160001) has been deleted

[rbpotter, 2017-01-27 03:19:06]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 03:19:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 03:41:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 03:41:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rbpotter, 2017-01-27 16:21:05]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 16:21:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-27 16:37:21]

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:417: Emf::Enumerator
emf_enum(emf, nullptr, nullptr);
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> Why postscript is encoded as GDI comment into EMF?
> Why not to send as regular postscript file?

This was originally done by thestig@, so I am not sure what the
reason was. I will look into writing new files similar to emf_win.cc/h
for postscript.

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:702: void
PdfToPostScriptUtilityProcessHostClient::OnPageDone(bool success) {
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> Maybe we can have single PdfConverterUtilityProcessHostClient for both
> converters?

Sorry, I am not sure what you mean exactly. Do you think the classes
should be combined entirely? They send different messages and have to
set the metafile differently, which is why I had them separate.
I could split out the message handlers again and have one large
OnMessageReceived function; is that closer to what you are thinking?

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:751:
DCHECK(!utility_client_.get());
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> There is also
>
https://cs.chromium.org/chromium/src/chrome/service/service_utility_process_h...
> 
> which may also benefit from postscript printing

Should we add postscript printing there now or in a later patch?

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/print_job.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:322: is_postscript_ ? 0.0f : scale_factor,
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> I guess better to remove conditiond and just use
> scale_factor
> it should be 0 for postscript.

Done.

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:324: is_postscript_ ? gfx::Rect() :
pdf_conversion_state_->content_area());
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> why do you need condition here?
> PdfConversionState was created with
> gfx::Size(), gfx::Rect(),

Done.

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.cc:338: is_postscript_ = true;
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> looks like is_postscript_ can be removed

Done.

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
File chrome/browser/printing/print_job.h (right):

https://codereview.chromium.org/2633573002/diff/140001/chrome/browser/printin...
chrome/browser/printing/print_job.h:165: bool is_postscript_;
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> I don't see where this is initialized
> 
> I'd put initializer here.
> bool is_postscript_ = false;

Acknowledged.

https://codereview.chromium.org/2633573002/diff/140001/printing/print_setting...
File printing/print_settings_initializer_win.cc (right):

https://codereview.chromium.org/2633573002/diff/140001/printing/print_setting...
printing/print_settings_initializer_win.cc:125:
print_settings->set_printer_is_ps2(true);
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> Looks like printer_is* flags exclusive
> probably cleaner to replace all 3 with enum

Done.

https://codereview.chromium.org/2633573002/diff/140001/printing/printed_docum...
File printing/printed_document.h (left):

https://codereview.chromium.org/2633573002/diff/140001/printing/printed_docum...
printing/printed_document.h:51: #if defined(OS_WIN)
On 2017/01/27 00:38:49, Vitaly Buka wrote:
> How is this related to the patch?
> PS printing is windows feature, why you enable this for other OSes?
> 
> Separate patch?

Done.

[commit-bot: I haz the power, 2017-01-27 17:58:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 17:58:43]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[rbpotter, 2017-01-27 21:56:55]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 21:57:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-27 22:32:49]

I looked into the postscript embedded in metafiles issue.
Pdfium is wired to write output to an HDC, which is
generated by CreateEnhMetafile. This requires writing to
it as a metafile which is why the postscript is written
as comments into the metafile. To change this, we
would need to re-wire PDFium to use a different structure
as an alternative/create a new path for postscript
files. The issue is not the Emf class needing to be
redone as it seems most functions that are called do not
do much other than deal with the underlying HDC. It is
the underlying HDC in the Emf that would need to be 
replaced for postscript, which would require significant
changes to Pdfium (likely a new path/function
RenderPdfPageToPostScript, instead of using the existing
RenderPdfPageToDC that is used now). Let me know if you
think we need to do this; it would have to be a separate
(Pdfium) CL. 

Let me know what you would like me to do with the
UtilityProcessHosts and/or adding postscript printing to
chrome/service.

[Vitaly Buka (NO REVIEWS), 2017-01-27 22:47:00]

On 2017/01/27 22:32:49, rbpotter wrote:
> I looked into the postscript embedded in metafiles issue.
> Pdfium is wired to write output to an HDC, which is
> generated by CreateEnhMetafile. This requires writing to
> it as a metafile which is why the postscript is written
> as comments into the metafile. To change this, we
> would need to re-wire PDFium to use a different structure
> as an alternative/create a new path for postscript
> files. The issue is not the Emf class needing to be
> redone as it seems most functions that are called do not
> do much other than deal with the underlying HDC. It is
> the underlying HDC in the Emf that would need to be 
> replaced for postscript, which would require significant
> changes to Pdfium (likely a new path/function
> RenderPdfPageToPostScript, instead of using the existing
> RenderPdfPageToDC that is used now). Let me know if you
> think we need to do this; it would have to be a separate
> (Pdfium) CL. 

thanks, let keep it this way

> 
> Let me know what you would like me to do with the
> UtilityProcessHosts and/or adding postscript printing to
> chrome/service.

Not sure. Service also uses utility process for PDF to EMF conversion.
So it would be good to use postscript path there if it improves experience.
However it probably low priority and not clear if the team will have time for
that.

[Vitaly Buka (NO REVIEWS), 2017-01-27 22:53:49]

https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:759:
PdfToPostScriptConverterImpl::~PdfToPostScriptConverterImpl() {
how about this way? 
https://codereview.chromium.org/2651313005/

[commit-bot: I haz the power, 2017-01-27 23:52:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 23:52:51]

Dry run: This issue passed the CQ dry run.

[Vitaly Buka (NO REVIEWS), 2017-01-27 23:56:42]

https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:759:
PdfToPostScriptConverterImpl::~PdfToPostScriptConverterImpl() {
On 2017/01/27 22:53:49, Vitaly Buka wrote:
> how about this way? 
> https://codereview.chromium.org/2651313005

I think new messages are not worth of 200 lines of code.
Settings already has this info, and the rest is the same.
https://codereview.chromium.org/2656303005

you can apply them ontop of your patch with
git cl patch 2651313005
git cl patch 2656303005

[rbpotter, 2017-01-28 00:41:54]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 00:42:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-28 00:57:34]

On 2017/01/27 23:56:42, Vitaly Buka wrote:
>
https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
> File chrome/browser/printing/pdf_to_emf_converter.cc (right):
> 
>
https://codereview.chromium.org/2633573002/diff/220001/chrome/browser/printin...
> chrome/browser/printing/pdf_to_emf_converter.cc:759:
> PdfToPostScriptConverterImpl::~PdfToPostScriptConverterImpl() {
> On 2017/01/27 22:53:49, Vitaly Buka wrote:
> > how about this way? 
> > https://codereview.chromium.org/2651313005
> 
> I think new messages are not worth of 200 lines of code.
> Settings already has this info, and the rest is the same.
> https://codereview.chromium.org/2656303005
> 
> you can apply them ontop of your patch with
> git cl patch 2651313005
> git cl patch 2656303005

Thank you for the recommendations. I applied both these patches
and checked that everything still works.

[Vitaly Buka (NO REVIEWS), 2017-01-28 01:23:58]

lgtm

https://codereview.chromium.org/2633573002/diff/240001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/240001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:241: PdfConverterImpl(); // :
weak_ptr_factory_(this) {}
please remove comment

[rbpotter, 2017-01-28 01:26:55]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 01:27:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-28 01:27:34]

rbpotter@chromium.org changed reviewers:
+ rkaplow@chromium.org, thakis@chromium.org, tsepez@chromium.org

[rbpotter, 2017-01-28 01:36:16]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 01:36:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-01-28 02:02:00]

Adding new reviewers:
tsepez@ - Messages
rkaplow@ - histograms
thakis@ - chrome/utility/printing_handler.*

https://codereview.chromium.org/2633573002/diff/240001/chrome/browser/printin...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/2633573002/diff/240001/chrome/browser/printin...
chrome/browser/printing/pdf_to_emf_converter.cc:241: PdfConverterImpl(); // :
weak_ptr_factory_(this) {}
On 2017/01/28 01:23:58, Vitaly Buka wrote:
> please remove comment

Done.

[commit-bot: I haz the power, 2017-01-28 02:39:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 02:39:04]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-01-30 16:35:20]

lgtm

[Tom Sepez, 2017-01-30 18:10:22]

messages LGTM.

[Nico, 2017-01-31 20:17:42]

printing_handler looks basically ok, but like Vitaly I don't understand from
reading the CL description and the linked bug why we want to add a postscript
path. Is there a design doc for that somewhere that was reviewed? Or is this a
smaller change than it sounds?

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
File chrome/utility/printing_handler.cc (right):

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
chrome/utility/printing_handler.cc:118: success =
RenderPdfPageToPostScript(page_number, std::move(emf_file));
add comment why scale factor of 1 is always right in this case

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
chrome/utility/printing_handler.cc:209: metafile.Init();
this function looks identical to RenderPdfPageToMetafile except that this
function here looks at pdf_rendering_settings_.offsets while the other one does
some scaling. why the difference? why the duplication?

[rbpotter, 2017-01-31 23:26:15]

Patchset #14 (id:300001) has been deleted

[rbpotter, 2017-01-31 23:41:08]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 23:42:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 23:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 23:45:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rbpotter, 2017-02-01 00:53:36]

The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 00:54:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rbpotter, 2017-02-01 01:09:00]

The connection to the bug: This bug is a complaint that
Chrome generates larger spool sizes than other programs
when printing, particularly when printing to postscript
printers. We looked into this and found that the reason
for the issue is that Chrome treats postscript printers
the same as other printers, and sends an emf file. This
means chrome does not take advantage of postscript
printers' capabilities to process compressed data and
images, resulting in large spool sizes from sending
uncompressed data/images. Adding a postscript path
allows Chrome to use postscript printer capabilities and
makes the spool size more comparable to the size
generated by other programs like Acrobat. See also
issue 685219.

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
File chrome/utility/printing_handler.cc (right):

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
chrome/utility/printing_handler.cc:118: success =
RenderPdfPageToPostScript(page_number, std::move(emf_file));
On 2017/01/31 20:17:42, Nico wrote:
> add comment why scale factor of 1 is always right in this case

Done.

https://codereview.chromium.org/2633573002/diff/280001/chrome/utility/printin...
chrome/utility/printing_handler.cc:209: metafile.Init();
On 2017/01/31 20:17:41, Nico wrote:
> this function looks identical to RenderPdfPageToMetafile except that this
> function here looks at pdf_rendering_settings_.offsets while the other one
does
> some scaling. why the difference? why the duplication?

I merged these two to eliminate the duplication.
The difference is because the postscript driver does not use the
metafile size since it outputs postscript rather than metafile.
Instead it uses the printable area directly. So we do not need to
scale the HDC to match the metafile, and we need to send the physical
offsets from the printer.

[Nico, 2017-02-01 01:33:35]

Thanks. Code in my file lgtm, but adding a completely new printing output mode
sounds to me like something that should be explained in more places than in just
a code review comment. To me, it feels like something that should have a
(potentially small) design doc that's reviewed by printing/OWNERS. (But that's
just my 2 cents; you have my lgtm for the two files you need it for.)

[commit-bot: I haz the power, 2017-02-01 02:10:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 02:10:15]

Dry run: This issue passed the CQ dry run.

[rbpotter, 2017-02-01 02:11:45]

On 2017/02/01 01:33:35, Nico wrote:
> Thanks. Code in my file lgtm, but adding a completely new printing output mode
> sounds to me like something that should be explained in more places than in
just
> a code review comment. To me, it feels like something that should have a
> (potentially small) design doc that's reviewed by printing/OWNERS. (But that's
> just my 2 cents; you have my lgtm for the two files you need it for.)

Thanks for the review. I will add a design doc for this shortly so that we
have an explanation/record of it. That will also allow for further review
before turning the flag on. Will also link it to this CL.

[rbpotter, 2017-02-01 03:39:15]

The CQ bit was checked by rbpotter@chromium.org

[rbpotter, 2017-02-01 03:39:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
vitalybuka@chromium.org, tsepez@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2633573002/#ps360001
(title: "Merge")

[commit-bot: I haz the power, 2017-02-01 03:39:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 03:44:28]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1485920355129760,
"parent_rev": "1a59e8372660ac6452651b81a1900cecf0e7b7be", "commit_rev":
"077e8a90f1878b1fce2b4d04bd07998ab8c76e11"}

[commit-bot: I haz the power, 2017-02-01 03:48:00]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1485920355129760,
"parent_rev": "f4f2afa36c8c14df3cbaf2f291b7f8c129e2bcbb", "commit_rev":
"58bc882e11a2ef3d8f446dff391b953bc08809bf"}

[commit-bot: I haz the power, 2017-02-01 03:48:40]

Description was changed from

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
change printing type from boolean for GDI to an enum in PdfRenderSettings:
https://codereview.chromium.org/2640033005/#
==========

to

==========
Add Postscript generation support to Chromium. See
https://codereview.chromium.org/2612243005/ and
https://codereview.chromium.org/2615703002/ for Pdfium changes. Added behind a
flag for now.

Refactoring/small CLs landed to make this easier:
Pdfium hooks: https://codereview.chromium.org/2642493002/
print settings initializer refactor:
https://codereview.chromium.org/2637243002/
pdf to emf refactor:
https://codereview.chromium.org/2646523003/
print job refactor:
https://codereview.chromium.org/2643163002/
BUG=614988
change printing type from boolean for GDI to an enum in PdfRenderSettings:
https://codereview.chromium.org/2640033005/#

Review-Url: https://codereview.chromium.org/2633573002
Cr-Commit-Position: refs/heads/master@{#447440}
Committed:
https://chromium.googlesource.com/chromium/src/+/58bc882e11a2ef3d8f446dff391b...
==========

[commit-bot: I haz the power, 2017-02-01 03:48:42]

Committed patchset #16 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/58bc882e11a2ef3d8f446dff391b...