Printing on Windows via PDF

Printing on Windows via PDF

Currently:
- Based on #if, switches renderer rendering to PDF for preview
- Add PdfToEmfConvert based on PWGRasterConverter
- Various plumbing.

Preview works the same as other PDF based platforms. For getting
to EMF, the when PRINTING_WIN_USES_PDF_AS_METAFILE is on, the
renderer generates a PDF instead of EMF directly (this contains
all the pages in the desired range). This is returned to
PrintViewManagerBase, where it's passed to PdfToEmfConverter which
uses the sandboxed utility process and pdf.dll to convert via
RenderPDFPageToDC. The utility process renderers one emf page at
at time (into numbered files) to avoid having a very large emf file.

As this uses pdf.dll this is currently off-by-default controlled by
the gyp variable win_pdf_metafile_for_printing.

R=vitalybuka@chromium.org
BUG=170859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271772

[Vitaly Buka (NO REVIEWS), 2014-04-24 22:47:57]

Approach looks good, maybe the simplest way to achieve results.

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:557:
scoped_ptr<ServiceUtilityProcessHost> utility_host(
ServiceUtilityProcessHost is for use in Service process only.

Take a look at PwgUtilityProcessHostClient. you can clone for EMF
or move to separate file and extend with ConvertToPwg, ConvertToEMF

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:562: page_ranges)) {
Message ChromeUtilityMsg_RenderPDFPagesToMetafile should be fixed, to use
PlatformFileForTransit. I'll do that right now. it should not significant
conflicts for you.

https://codereview.chromium.org/255543006/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.h (right):

https://codereview.chromium.org/255543006/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.h:212: const gfx::Size&
canvas_size,
Maybe be this branch is PRINTING_USE_EMF_METAFIL || MACOSX and the first is the
rest.

https://codereview.chromium.org/255543006/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.h:221: #if defined(OS_WIN) &&
PRINTING_USE_EMF_METAFILE
I don't see how PRINTING_USE_EMF_METAFILE can be used on other then win
platforms

https://codereview.chromium.org/255543006/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper_win.cc (right):

https://codereview.chromium.org/255543006/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper_win.cc:22: 
Maybe it's worth additional file, like *helper_pdf_win.cc

https://codereview.chromium.org/255543006/diff/40001/printing/metafile_impl.h
File printing/metafile_impl.h (right):

https://codereview.chromium.org/255543006/diff/40001/printing/metafile_impl.h...
printing/metafile_impl.h:10: #if defined(OS_POSIX) || (defined(OS_WIN) &&
!defined(GOOGLE_CHROME_BUILD))
POSIX does not use metafile

I guess this defile should be WIN Chromium only

[Vitaly Buka (NO REVIEWS), 2014-04-25 05:25:47]

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:562: page_ranges)) {
It's going to be harder then I thought, Win32 metafile works only with file
path, not with file handles.
So I will not change right now, to avoid introducing new source of regressions
here.

On 2014/04/24 22:47:58, Vitaly Buka wrote:
> Message ChromeUtilityMsg_RenderPDFPagesToMetafile should be fixed, to use
> PlatformFileForTransit. I'll do that right now. it should not significant
> conflicts for you.

[scottmg, 2014-04-25 20:47:18]

Thanks!

This is almost working now; the pdf is rendered, converted to emf in the utility
mostly successfully (? it's a bit messed up see
https://dl.dropboxusercontent.com/u/116514/output.emf for printing example.com,
and there's a hardcoded dpi/render area).

It's currently failing in PrintedDocument::RenderPrintedPage.
page.metafile()->SafePlayback is failing with "invalid function" as the error,
but I'm not sure why yet. Perhaps because of whatever is malformed in the emf
during conversion.

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:557:
scoped_ptr<ServiceUtilityProcessHost> utility_host(
On 2014/04/24 22:47:58, Vitaly Buka wrote:
> ServiceUtilityProcessHost is for use in Service process only.
> 
> Take a look at PwgUtilityProcessHostClient. you can clone for EMF
> or move to separate file and extend with ConvertToPwg, ConvertToEMF

Thanks for the pointer, that seems to be mostly working.

https://codereview.chromium.org/255543006/diff/40001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:562: page_ranges)) {
On 2014/04/25 05:25:48, Vitaly Buka wrote:
> It's going to be harder then I thought, Win32 metafile works only with file
> path, not with file handles.
> So I will not change right now, to avoid introducing new source of regressions
> here.
> 
> On 2014/04/24 22:47:58, Vitaly Buka wrote:
> > Message ChromeUtilityMsg_RenderPDFPagesToMetafile should be fixed, to use
> > PlatformFileForTransit. I'll do that right now. it should not significant
> > conflicts for you.
> 

I gave it a shot, basically using GetEnhMetaFileBits and adding a
PlatformFileForTransit version of the message in addition to the path one.

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:228: /* XXX
Not sure what the correct way to get a printer DC is yet.

[Vitaly Buka (NO REVIEWS), 2014-04-25 22:35:24]

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:137: PrintedDocument*
document = print_job_->document();
this metafile could be multi paged. it's likely incorrect to use SetPage for
that here.

So maybe single string with appended metafiles (size+page_data) is solution
here.

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:228: /* XXX
you don't need dc here, params.printable_area is probably what you need
On 2014/04/25 20:47:18, scottmg wrote:
> Not sure what the correct way to get a printer DC is yet.

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:737:
CHECK_EQ(static_cast<int>(final_size),
This could be huge, metafile keeps images uncompressed.
browser<->renderer uses metafile per page, and there is bugs when it can't
allocate +200MB of shared memory for page.
You can just write stream size+page_data,size+page_data,size+page_data.

[Vitaly Buka (NO REVIEWS), 2014-04-25 22:42:15]

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:137: PrintedDocument*
document = print_job_->document();
On 2014/04/25 22:35:24, Vitaly Buka wrote:
> this metafile could be multi paged. it's likely incorrect to use SetPage for
> that here.
> 
> So maybe single string with appended metafiles (size+page_data) is solution
> here.

single string -> single stream of concatenated pages saved to |emf_file|

[scottmg, 2014-04-25 22:45:10]

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:137: PrintedDocument*
document = print_job_->document();
On 2014/04/25 22:35:24, Vitaly Buka wrote:
> this metafile could be multi paged. it's likely incorrect to use SetPage for
> that here.
> 
> So maybe single string with appended metafiles (size+page_data) is solution
> here.

This is only called from OnDidPrintPage, where it's used like this. Is it
potentially multi-page when the pdf is one page, but then that converts to
multiple emf pages?

https://codereview.chromium.org/255543006/diff/90001/chrome/browser/printing/...
chrome/browser/printing/print_view_manager_base.cc:228: /* XXX
On 2014/04/25 22:35:24, Vitaly Buka wrote:
> you don't need dc here, params.printable_area is probably what you need

Ah! Thanks.

> On 2014/04/25 20:47:18, scottmg wrote:
> > Not sure what the correct way to get a printer DC is yet.
>

[scottmg, 2014-04-25 23:07:18]

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:737:
CHECK_EQ(static_cast<int>(final_size),
On 2014/04/25 22:35:24, Vitaly Buka wrote:
> This could be huge, metafile keeps images uncompressed.
> browser<->renderer uses metafile per page, and there is bugs when it can't
> allocate +200MB of shared memory for page.
> You can just write stream size+page_data,size+page_data,size+page_data.

Sorry, I'm not sure what you mean here. You mean converting an EMF one page at a
time here, and then merging them somehow? The only EMF data retrieval function
seems to be for the whole file.

I'm looking at a spec here http://www.schnarff.com/file-formats/emf/[MS-EMF].pdf
it looks a bit involved to merge multiple EMF (or would at least need some
record parsing it looks like).

Or do you mean it needs to do the kMetafileMaxSize path and use
RasterizeMetafile instead?

[Vitaly Buka (NO REVIEWS), 2014-04-28 06:47:32]

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:737:
CHECK_EQ(static_cast<int>(final_size),
It's not about rasterize.

You don't need to try to create single readable metafile like in spec.
Just concatenate them, and split on other side spooling to printers:

for (int page_number = iter->from; page_number <= iter->to; ++page_number) {
  printing::Emf metafile;
  metafile.Init();
  
  ... print one page ...
  
  base::WritePlatformFile(size);
  base::WritePlatformFile(metafile_file, 0, final_buffer.get(), final_size));
}


Ideal approach is to start Utility process in batch mode, open PDF doc once
and then request each page with separate IPC message. So we will have only one
page on disk in time, and we would able to spool the first page sooner.

I recently noticed that the same in cloud print implemented even without batch
mode. It has parameter page_range, but it always single page. Without batch mode
it just re-opens PDF for each page, inefficient.

[Vitaly Buka (NO REVIEWS), 2014-04-28 06:49:06]

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/90001/chrome/utility/chrome_co...
chrome/utility/chrome_content_utility_client.cc:737:
CHECK_EQ(static_cast<int>(final_size),
base::WritePlatformFile -> base::WritePlatformFileAtCurrentPos to append

On 2014/04/28 06:47:33, Vitaly Buka wrote:
> It's not about rasterize.
> 
> You don't need to try to create single readable metafile like in spec.
> Just concatenate them, and split on other side spooling to printers:
> 
> for (int page_number = iter->from; page_number <= iter->to; ++page_number) {
>   printing::Emf metafile;
>   metafile.Init();
>   
>   ... print one page ...
>   
>   base::WritePlatformFile(size);
>   base::WritePlatformFile(metafile_file, 0, final_buffer.get(), final_size));
> }
> 
> 
> Ideal approach is to start Utility process in batch mode, open PDF doc once
> and then request each page with separate IPC message. So we will have only one
> page on disk in time, and we would able to spool the first page sooner.
> 
> I recently noticed that the same in cloud print implemented even without batch
> mode. It has parameter page_range, but it always single page. Without batch
mode
> it just re-opens PDF for each page, inefficient.
>

[scottmg, 2014-05-13 22:37:13]

OK, this finally works enough to be worth reviewing now. I'm sorry it got so
large. I'm not sure if there's a good way to break it up, but I'm happy to do
that if you think there's one.

The #ifs aren't the prettiest. I feel like we could probably stand to do some
overhaul to get the platforms to all be much more similar, but I don't really
want to try that change here.

I'm also not sure about tests. printing_unittests seems fine, but it's not
testing too much. Are there end-to-end tests I should be testing/adding to?

[Vitaly Buka (NO REVIEWS), 2014-05-13 23:54:32]

no need to break it up

lgtm with some nits and some questions

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:200:
RunCallback(highest_rendered_page_number, scale_factor);
why do you need highest_rendered_page_number if only the first page was requesed

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:228: // gdiplus.dll, change how
rendering happens, and not be able to correctly
so --no-sandbox will fail printing?

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:141: for
(std::vector<base::FilePath>::const_iterator i(emf_files.begin());
I guess for (size_t i = ) is more appropriate here

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:202: kMaxRasterSizeInPixels);
missaligned

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:222: params.actual_shrink,
missaligned

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:229: base::RefCountedBytes*
bytes = new base::RefCountedBytes(
scoped_refptr<base::RefCountedBytes>

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:190: IPC::PlatformFileForTransit,  //
PDF file
Why not just ChromeUtilityMsg_RenderPDFPagesToMetafile ?

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
File chrome/renderer/printing/print_web_view_helper_pdf_win.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
chrome/renderer/printing/print_web_view_helper_pdf_win.cc:7: #if
defined(PRINTING_WIN_USES_PDF_AS_METAFILE)
maybe in GYP file?

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
File chrome/renderer/printing/print_web_view_helper_win.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
chrome/renderer/printing/print_web_view_helper_win.cc:8: 
GYP file?

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:712:
metafile.InitToFile(metafile_path);
maybe just update this function to use file per page?
metafile_path.InsertBeforeExtensionASCII(base::StringPrintf(".%d", i)));

https://codereview.chromium.org/255543006/diff/400001/printing/metafile_impl.h
File printing/metafile_impl.h (right):

https://codereview.chromium.org/255543006/diff/400001/printing/metafile_impl....
printing/metafile_impl.h:12: // PDF DLL" not GOOGLE_CHROME_BUILD specifically,
or perhaps a gyp setting?
GYP is better

[scottmg, 2014-05-14 00:34:48]

Thanks for the quick review!

Couple questions for follow up.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:200:
RunCallback(highest_rendered_page_number, scale_factor);
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> why do you need highest_rendered_page_number if only the first page was
requesed

Ah, oops, that one-page code above was old/unused. Deleted it. The new message
ChromeUtilityMsg_RenderPDFToMetafile converts all pages in the pdf (which will
only contain the pages requested by preview) and outputs each page to a separate
metafile.

The highest_rendered_page_number is just used to build the vector of filenames
to pass back to PrintViewManagerBase::OnPdfToEmfConverted.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:228: // gdiplus.dll, change how
rendering happens, and not be able to correctly
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> so --no-sandbox will fail printing?

Yes! This took me far too long to figure out as I had --no-sandbox to make
attaching/debugging easier and didn't think that it'd be the cause of the
problems I was seeing (weird scale issues).

It would be best to add a flag or something to pdf.dll to force it not to load
gdiplus.dll, but I'm not sure how/where we carry/submit patches on that.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:141: for
(std::vector<base::FilePath>::const_iterator i(emf_files.begin());
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> I guess for (size_t i = ) is more appropriate here

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:202: kMaxRasterSizeInPixels);
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> missaligned

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:222: params.actual_shrink,
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> missaligned

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:229: base::RefCountedBytes*
bytes = new base::RefCountedBytes(
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> scoped_refptr<base::RefCountedBytes>

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:190: IPC::PlatformFileForTransit,  //
PDF file
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> Why not just ChromeUtilityMsg_RenderPDFPagesToMetafile ?

A few (not great) reasons:
- I don't know how many pages are in the pdf to create the vector<PageRange>
- IPC::PlatformFileForTransit
- I was nervous about breaking cloud print as I had a hard time testing it.

Maybe an empty PageRange could correspond to all pages in
ChromeUtilityMsg_RenderPDFPagesToMetafile?

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
File chrome/renderer/printing/print_web_view_helper_pdf_win.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
chrome/renderer/printing/print_web_view_helper_pdf_win.cc:7: #if
defined(PRINTING_WIN_USES_PDF_AS_METAFILE)
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> maybe in GYP file?
> 

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
File chrome/renderer/printing/print_web_view_helper_win.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/renderer/printin...
chrome/renderer/printing/print_web_view_helper_win.cc:8: 
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> GYP file?

Done.

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:712:
metafile.InitToFile(metafile_path);
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> maybe just update this function to use file per page?
> metafile_path.InsertBeforeExtensionASCII(base::StringPrintf(".%d", i)));

At the moment, print_system_win.cc expects all pages to be in the same emf, so
that would need to be changed. (As it happens, it's only actually one at a time
despite the comment ;)
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/service/clo...
)

It would also need to understand the addition of .%d as now it expects the data
to show up that file rather than receiving the output.

I would also need to have an empty range mean "all pages" as PdfToEmfConverter
doesn't know how many pages are in the pdf until they're converted by the
utility.

Do you prefer those changes/does that sound right?

https://codereview.chromium.org/255543006/diff/400001/printing/metafile_impl.h
File printing/metafile_impl.h (right):

https://codereview.chromium.org/255543006/diff/400001/printing/metafile_impl....
printing/metafile_impl.h:12: // PDF DLL" not GOOGLE_CHROME_BUILD specifically,
or perhaps a gyp setting?
On 2014/05/13 23:54:33, Vitaly Buka wrote:
> GYP is better

Done. win_pdf_metafile_for_printing=1 in GYP_DEFINES.

[Vitaly Buka (NO REVIEWS), 2014-05-14 03:51:17]

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:228: // gdiplus.dll, change how
rendering happens, and not be able to correctly
I guess we need to check this in PDF module.
We can address this in separate CL later.

On 2014/05/14 00:34:48, scottmg wrote:
> On 2014/05/13 23:54:33, Vitaly Buka wrote:
> > so --no-sandbox will fail printing?
> 
> Yes! This took me far too long to figure out as I had --no-sandbox to make
> attaching/debugging easier and didn't think that it'd be the cause of the
> problems I was seeing (weird scale issues).
> 
> It would be best to add a flag or something to pdf.dll to force it not to load
> gdiplus.dll, but I'm not sure how/where we carry/submit patches on that.

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:190: IPC::PlatformFileForTransit,  //
PDF file
Cloud you please put TODO on me, and I'll merge this and cloud print code path.

On 2014/05/14 00:34:48, scottmg wrote:
> On 2014/05/13 23:54:33, Vitaly Buka wrote:
> > Why not just ChromeUtilityMsg_RenderPDFPagesToMetafile ?
> 
> A few (not great) reasons:
> - I don't know how many pages are in the pdf to create the vector<PageRange>
> - IPC::PlatformFileForTransit
> - I was nervous about breaking cloud print as I had a hard time testing it.
> 
> Maybe an empty PageRange could correspond to all pages in
> ChromeUtilityMsg_RenderPDFPagesToMetafile?

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:650:
highest_rendered_page_number, scale_factor));
Actually with this approach we could send success message for every page, so
browser can start spooling early.

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:712:
metafile.InitToFile(metafile_path);
This sounds right. 
You can fix this or just TODO on me.

On 2014/05/14 00:34:48, scottmg wrote:
> On 2014/05/13 23:54:33, Vitaly Buka wrote:
> > maybe just update this function to use file per page?
> > metafile_path.InsertBeforeExtensionASCII(base::StringPrintf(".%d", i)));
> 
> At the moment, print_system_win.cc expects all pages to be in the same emf, so
> that would need to be changed. (As it happens, it's only actually one at a
time
> despite the comment ;)
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/service/clo...
> )
> 
> It would also need to understand the addition of .%d as now it expects the
data
> to show up that file rather than receiving the output.
> 
> I would also need to have an empty range mean "all pages" as PdfToEmfConverter
> doesn't know how many pages are in the pdf until they're converted by the
> utility.
> 
> Do you prefer those changes/does that sound right?

https://codereview.chromium.org/255543006/diff/440001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.h (right):

https://codereview.chromium.org/255543006/diff/440001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.h:15: #include
"printing/metafile_impl.h"
I guess you needed this only for defines. Now it's in GYP

[scottmg, 2014-05-15 18:01:30]

Sorry for the delay, was out yesterday. Please take another look.

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
File chrome/common/chrome_utility_messages.h (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/common/chrome_ut...
chrome/common/chrome_utility_messages.h:190: IPC::PlatformFileForTransit,  //
PDF file
On 2014/05/14 03:51:18, Vitaly Buka wrote:
> Cloud you please put TODO on me, and I'll merge this and cloud print code
path.

I took a try at merging, ptal.

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/400001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:712:
metafile.InitToFile(metafile_path);
On 2014/05/14 03:51:18, Vitaly Buka wrote:
> This sounds right. 
> You can fix this or just TODO on me.

OK, I tried to merge the messages, cloud print and converter code paths.

It now always inserts the %d for page number. The success message now passes
back a vector<PageRange> so that the file names can be correctly determined. I
also pass in IPC::PlatformFileForTransit as they were different before.

For now, I made it CHECK that only one page at a time is returned in the cloud
print case and added a TODO there, as it's been like that for a long time
anyway, and the calling code in print_system_win would need to be reorganized a
bit.

I also changed "Metafile" to "Metafiles" in the message names to make a it bit
more clear that multiple would be generated.

I didn't rebase, so 19->20 is the diffs.

> On 2014/05/14 00:34:48, scottmg wrote:
> > On 2014/05/13 23:54:33, Vitaly Buka wrote:
> > > maybe just update this function to use file per page?
> > > metafile_path.InsertBeforeExtensionASCII(base::StringPrintf(".%d", i)));
> > 
> > At the moment, print_system_win.cc expects all pages to be in the same emf,
so
> > that would need to be changed. (As it happens, it's only actually one at a
> time
> > despite the comment ;)
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/service/clo...
> > )
> > 
> > It would also need to understand the addition of .%d as now it expects the
> data
> > to show up that file rather than receiving the output.
> > 
> > I would also need to have an empty range mean "all pages" as
PdfToEmfConverter
> > doesn't know how many pages are in the pdf until they're converted by the
> > utility.
> > 
> > Do you prefer those changes/does that sound right?
>

https://codereview.chromium.org/255543006/diff/440001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.h (right):

https://codereview.chromium.org/255543006/diff/440001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.h:15: #include
"printing/metafile_impl.h"
On 2014/05/14 03:51:18, Vitaly Buka wrote:
> I guess you needed this only for defines. Now it's in GYP

Done.

[Vitaly Buka (NO REVIEWS), 2014-05-15 19:23:33]

lgtm

https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:586: Send(new
ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_Succeeded(
So how about sending ChromeUtilityHostMsg_RenderPDFPageToMetafiles_Succeeded
message for each page, so we can start spool sooner?

[scottmg, 2014-05-15 19:43:03]

Thanks, adding some other OWNERS:

jschuh: chrome/common/chrome_utility_messages.h
thestig: chrome/utility/chrome_content_utility_client.*
raymes: content/renderer/pepper/pepper_plugin_instance_impl.cc

https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:586: Send(new
ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_Succeeded(
On 2014/05/15 19:23:34, Vitaly Buka wrote:
> So how about sending ChromeUtilityHostMsg_RenderPDFPageToMetafiles_Succeeded
> message for each page, so we can start spool sooner?

That should help for PdfToEmfConverter I think. It won't matter for cloud print
because the range is always 1 page here.

[Vitaly Buka (NO REVIEWS), 2014-05-15 20:23:44]

On 2014/05/15 19:43:03, scottmg wrote:
> Thanks, adding some other OWNERS:
> 
> jschuh: chrome/common/chrome_utility_messages.h
> thestig: chrome/utility/chrome_content_utility_client.*
> raymes: content/renderer/pepper/pepper_plugin_instance_impl.cc
> 
>
https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
> File chrome/utility/chrome_content_utility_client.cc (right):
> 
>
https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
> chrome/utility/chrome_content_utility_client.cc:586: Send(new
> ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_Succeeded(
> On 2014/05/15 19:23:34, Vitaly Buka wrote:
> > So how about sending ChromeUtilityHostMsg_RenderPDFPageToMetafiles_Succeeded
> > message for each page, so we can start spool sooner?
> 
> That should help for PdfToEmfConverter I think. It won't matter for cloud
print
> because the range is always 1 page here.

when we fix TODO in service_utility_process_host.cc, cloud print also would
benefit from this

[jschuh, 2014-05-15 20:31:19]

ipc security lgtm (notes: no change in attack surface)

[scottmg, 2014-05-15 20:47:59]

On 2014/05/15 20:23:44, Vitaly Buka wrote:
> On 2014/05/15 19:43:03, scottmg wrote:
> > Thanks, adding some other OWNERS:
> > 
> > jschuh: chrome/common/chrome_utility_messages.h
> > thestig: chrome/utility/chrome_content_utility_client.*
> > raymes: content/renderer/pepper/pepper_plugin_instance_impl.cc
> > 
> >
>
https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
> > File chrome/utility/chrome_content_utility_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/255543006/diff/500001/chrome/utility/chrome_c...
> > chrome/utility/chrome_content_utility_client.cc:586: Send(new
> > ChromeUtilityHostMsg_RenderPDFPagesToMetafiles_Succeeded(
> > On 2014/05/15 19:23:34, Vitaly Buka wrote:
> > > So how about sending
ChromeUtilityHostMsg_RenderPDFPageToMetafiles_Succeeded
> > > message for each page, so we can start spool sooner?
> > 
> > That should help for PdfToEmfConverter I think. It won't matter for cloud
> print
> > because the range is always 1 page here.
> 
> when we fix TODO in service_utility_process_host.cc, cloud print also would
> benefit from this

Agreed. I added a todo here for now, as I'm not entirely confident there's no
ordering considerations in PrintViewManagerBase from getting multiple of these
instead of only one. I will take a look as a follow up CL if that's OK with you.

[Vitaly Buka (NO REVIEWS), 2014-05-15 20:51:03]

lgtm

[raymes, 2014-05-16 01:00:39]

On 2014/05/15 20:51:03, Vitaly Buka wrote:
> lgtm

rubberstamp lgtm

[Lei Zhang, 2014-05-20 03:39:28]

lgtm

Mostly nits below. I was OOO for a few days, thus the delayed review.

https://codereview.chromium.org/255543006/diff/630001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/255543006/diff/630001/build/common.gypi#newco...
build/common.gypi:431: # requires internal_pdf to be enabled.
|internal_pdf| is likely going away this week, FYI

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:52: return temp_dir_.path();
I guess this is more of a criticism of the original Pwg code. When
FileHandlers::Init() calls temp_dir_.CreateUniqueTempDir() and that fails, you
are probably not getting the right path heere and in the other GetFooPath()
functions above. Feel free to better handle this later.

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.h:16: class Size;
nit: not needed

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:18: #include
"chrome/browser/printing/pdf_to_emf_converter.h"
put this and other newly added includes in a #if
defined(WIN_PDF_METAFILE_FOR_PRINTING) section?

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:235: const int printer_dpi =
600;
nit: kPrinterDpi

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
chrome/service/service_utility_process_host.cc:110: metafile_path_ =
scratch_metafile_dir_->path().AppendASCII("output.emf");
Share "output.emf" as a const with
chrome/browser/printing/pdf_to_emf_converter.cc ?

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
chrome/service/service_utility_process_host.h:143: const
std::vector<printing::PageRange> page_ranges,
nit: const ref?

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:628: CHECK(page_ranges);
DCHECK, put it as the first thing in this method

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:665: for (iter =
page_ranges->begin(); iter != page_ranges->end(); ++iter) {
(issue with existing code) Do we care about overlapping ranges?

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:85:
std::vector<printing::PageRange>* page_ranges,
update comment above

[scottmg, 2014-05-20 17:16:24]

Thanks!

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.cc:52: return temp_dir_.path();
On 2014/05/20 03:39:29, Lei Zhang wrote:
> I guess this is more of a criticism of the original Pwg code. When
> FileHandlers::Init() calls temp_dir_.CreateUniqueTempDir() and that fails, you
> are probably not getting the right path heere and in the other GetFooPath()
> functions above. Feel free to better handle this later.

I didn't force CreateUniqueTempDir to fail to test this, but the intention was
that the IsValid check would abort when back on the UI thread at line 208.

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/pdf_to_emf_converter.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/pdf_to_emf_converter.h:16: class Size;
On 2014/05/20 03:39:29, Lei Zhang wrote:
> nit: not needed

Done.

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:18: #include
"chrome/browser/printing/pdf_to_emf_converter.h"
On 2014/05/20 03:39:29, Lei Zhang wrote:
> put this and other newly added includes in a #if
> defined(WIN_PDF_METAFILE_FOR_PRINTING) section?

Done.

https://codereview.chromium.org/255543006/diff/630001/chrome/browser/printing...
chrome/browser/printing/print_view_manager_base.cc:235: const int printer_dpi =
600;
On 2014/05/20 03:39:29, Lei Zhang wrote:
> nit: kPrinterDpi

Done.

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
File chrome/service/service_utility_process_host.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
chrome/service/service_utility_process_host.cc:110: metafile_path_ =
scratch_metafile_dir_->path().AppendASCII("output.emf");
On 2014/05/20 03:39:29, Lei Zhang wrote:
> Share "output.emf" as a const with
> chrome/browser/printing/pdf_to_emf_converter.cc ?

It doesn't matter if they're the same or not, so I'm not sure it's worth forcing
them to be.

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
File chrome/service/service_utility_process_host.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/service/service_...
chrome/service/service_utility_process_host.h:143: const
std::vector<printing::PageRange> page_ranges,
On 2014/05/20 03:39:29, Lei Zhang wrote:
> nit: const ref?

Oops! Done.

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.cc (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:628: CHECK(page_ranges);
On 2014/05/20 03:39:29, Lei Zhang wrote:
> DCHECK, put it as the first thing in this method

Done.

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.cc:665: for (iter =
page_ranges->begin(); iter != page_ranges->end(); ++iter) {
On 2014/05/20 03:39:29, Lei Zhang wrote:
> (issue with existing code) Do we care about overlapping ranges?

Yeah, I wondered about that too. I guess it's possible the user might actually
want to print pages repeatedly via overlapping ranges in the UI.

[[ In practice this code is less generally used than it appears it could be, in
particular there are only two ranges ever given, the empty range resulting in
all pages being printed per the change above, and a range consisting of one page
(from cloud print).

I believe the future intention was to pass larger batches for cloud print, which
should be possible now that all the pages wouldn't be rendered into one huge
output file. ]]

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
File chrome/utility/chrome_content_utility_client.h (right):

https://codereview.chromium.org/255543006/diff/630001/chrome/utility/chrome_c...
chrome/utility/chrome_content_utility_client.h:85:
std::vector<printing::PageRange>* page_ranges,
On 2014/05/20 03:39:29, Lei Zhang wrote:
> update comment above

Done.

[scottmg, 2014-05-20 17:59:49]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-05-20 18:02:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/255543006/670001

[commit-bot: I haz the power, 2014-05-20 22:02:29]

Change committed as 271772