Linux: Refactor printing to be more like Windows/Mac....

Linux: Refactor printing to be more linux Windows/Mac.

BUG=41543, 59732
TEST=Printing works on Linux and CrOS, Linux printing respect print dialog settings.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=75475

[Lei Zhang, 2011-02-14 21:52:48]

maruel/sanjeevr: Please review the overall print workflow change.
estade: Please take a look at the changes to PrintDialogGtk.
tony: FYI, since you worked with myhuang a couple summers ago on Linux printing.
sbyer: FYI, since we talked about not modifying the Cloud Print workflow.


Overview:

The Linux print workflow gains the ability to set print settings before
rendering to PDF. We do this by using as much of the Win/Mac print workflow as
possible, instead of the Linux specific AllocateTempFileForPrinting /
TempFileForPrintingWritten messages.

Cloud print retains the existing workflow where we generate the PDF before
showing the print dialog. Cloud print will integrates with print preview in the
future and gain the ability to set printer settings before printing.

Design:

In order to print on Linux, we need to either draw into a Cairo surface or print
a pre-rendered PDF. Cairo drawing contexts are not portable across processes
like Windows with EMF files, thus there is no way to draw into a context in the
renderer, and then transport it to the browser. Thus one has to print a
pre-renderered PDF.

On Mac, the print workflow sends one page PDFs as the per page metadata and the
OS provides a way to merge all the one-page PDF documents into a print job.
There's currently no way to do this on Linux, so the hack here is to send the
entire document as one PDF in the first page. Pages 2-n are just dummy pages
with no data and we send them just to fit the expectations of the print
workflow.

The PDF data sent from the renderer to the browser lives inside a
NativeMetafile. The data needs to be saved to disk on the FILE thread, and then
the GTK print job needs to be created on the UI thread. There's no way to do
that inside the printing/ directory, thus I put that code in
chrome/browser/printing. In order to hook up printing/ to call
chrome/browser/printing, I set a couple function pointers in
PrintingContextCairo to so without directly exposing chrome/ to printing/.

[Evan Stade, 2011-02-14 23:30:54]

what's the point of RefCountedThreadSafe if you never have a ref count more than
1?

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:30: dialog->AddRef();  // Manual
AddRef since PrintingContextCairo is not suppose
nit: supposed
nit: comment above the code, rather than sharing a line (here and elsewhere, but
particularly here where the comment spans multiple lines)

why don't you have it start with a refcount of 1?

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:137: // Fall through.
why is it unexpected to get a cancel?

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:144: Release();  // Printing
cancelled.
from the comment, it seems like this would make more sense inside the CANCEL
case

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:158: temp_file_fd.fd =
open(path_to_pdf_.value().c_str(), O_WRONLY);
seems odd to do this stuff if error is true

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:170: Release();  // Error
encountered, bail out.
comment a little obvious imo

[Lei Zhang, 2011-02-14 23:47:31]

It's ref counted because we pass it around in PostTask.

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:30: dialog->AddRef();  // Manual
AddRef since PrintingContextCairo is not suppose
On 2011/02/14 23:30:54, Evan Stade wrote:
> nit: supposed
> nit: comment above the code, rather than sharing a line (here and elsewhere,
but
> particularly here where the comment spans multiple lines)
> 
> why don't you have it start with a refcount of 1?

Done.

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:81:
gtk_print_unix_dialog_set_embed_page_setup(GTK_PRINT_UNIX_DIALOG(dialog_),
This is GTK 2.18+. Does that mean I need to remove this?

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:137: // Fall through.
On 2011/02/14 23:30:54, Evan Stade wrote:
> why is it unexpected to get a cancel?

Whoops, meant to put a break there.

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:144: Release();  // Printing
cancelled.
On 2011/02/14 23:30:54, Evan Stade wrote:
> from the comment, it seems like this would make more sense inside the CANCEL
> case

Done.

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:158: temp_file_fd.fd =
open(path_to_pdf_.value().c_str(), O_WRONLY);
On 2011/02/14 23:30:54, Evan Stade wrote:
> seems odd to do this stuff if error is true

Done.

http://codereview.chromium.org/6516022/diff/1/chrome/browser/printing/print_d...
chrome/browser/printing/print_dialog_gtk.cc:170: Release();  // Error
encountered, bail out.
On 2011/02/14 23:30:54, Evan Stade wrote:
> comment a little obvious imo

Removed.

[Evan Stade, 2011-02-15 00:40:46]

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:39: scoped_ptr<base::WaitableEvent>
event;
unite these

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:40: event.reset(new
base::WaitableEvent(false, false));
two lines

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:48: event->Wait();  // Wait for
SaveDocumentToDisk() to finish.
can we at least be consistent between comment on line and above line?

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:81:
gtk_print_unix_dialog_set_embed_page_setup(GTK_PRINT_UNIX_DIALOG(dialog_),
if this is 2.18+ then we need a workaround until April

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:186:
UTF16ToASCII(document_name).c_str(),
is the document name guaranteed to be ascii? I suspect this should be ToUTF8

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:195: void
PrintDialogGtk::OnJobCompletedThunk(GtkPrintJob* print_job,
convert this to CHROMEGTK_SIGNAL macro style

[Lei Zhang, 2011-02-15 01:20:26]

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:39: scoped_ptr<base::WaitableEvent>
event;
On 2011/02/15 00:40:46, Evan Stade wrote:
> unite these

O

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:40: event.reset(new
base::WaitableEvent(false, false));
On 2011/02/15 00:40:46, Evan Stade wrote:
> two lines

K

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:48: event->Wait();  // Wait for
SaveDocumentToDisk() to finish.
On 2011/02/15 00:40:46, Evan Stade wrote:
> can we at least be consistent between comment on line and above line?

Yes.

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:81:
gtk_print_unix_dialog_set_embed_page_setup(GTK_PRINT_UNIX_DIALOG(dialog_),
On 2011/02/15 00:40:46, Evan Stade wrote:
> if this is 2.18+ then we need a workaround until April

Done.

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:186:
UTF16ToASCII(document_name).c_str(),
On 2011/02/15 00:40:46, Evan Stade wrote:
> is the document name guaranteed to be ascii? I suspect this should be ToUTF8

Done.

http://codereview.chromium.org/6516022/diff/7002/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:195: void
PrintDialogGtk::OnJobCompletedThunk(GtkPrintJob* print_job,
On 2011/02/15 00:40:46, Evan Stade wrote:
> convert this to CHROMEGTK_SIGNAL macro style

Can't. |user_data| is not the last parameter. All the gtk signal macros have
|user_data| as the last parameter.

If it can be converted, you would have done it already.

[Evan Stade, 2011-02-15 02:03:06]

LGTM

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:44: explicit
PrintDialogGtk(PrintingContextCairo::PrintSettingsCallback* callback,
no longer need explicit

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:75: base::WaitableEvent*
save_document_event_;
comment this

[Lei Zhang, 2011-02-15 02:12:17]

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:44: explicit
PrintDialogGtk(PrintingContextCairo::PrintSettingsCallback* callback,
On 2011/02/15 02:03:06, Evan Stade wrote:
> no longer need explicit

Done.

http://codereview.chromium.org/6516022/diff/13004/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:75: base::WaitableEvent*
save_document_event_;
On 2011/02/15 02:03:06, Evan Stade wrote:
> comment this

Done.

[Marc-Antoine Ruel (Google), 2011-02-15 14:18:23]

Only nits, lgtm

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:30: static void* CreatePrintDialog(
nit: It doesn't need to be public?

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter_gtk.cc:179: #if
defined(OS_CHROMEOS)
In theory it would have been nicer to use render_message_filter_linux.cc and
render_message_filter_chromeos.cc but I don't mind, probably not worth anyway.

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
File printing/printing_context_cairo.cc (right):

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
printing/printing_context_cairo.cc:43: : PrintingContext(app_locale) {
align at +4

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
File printing/printing_context_cairo.h (right):

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
printing/printing_context_cairo.h:24: // Set the function that creates the print
dialog, and the function that
nits: Sets

[Lei Zhang, 2011-02-15 21:20:10]

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.h (right):

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.h:30: static void* CreatePrintDialog(
On 2011/02/15 14:18:23, Marc-Antoine Ruel (Google) wrote:
> nit: It doesn't need to be public?

It does. I put it under private: and chrome/browser/browser_main_posix.cc no
longer compiles.

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/14007/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter_gtk.cc:179: #if
defined(OS_CHROMEOS)
On 2011/02/15 14:18:23, Marc-Antoine Ruel (Google) wrote:
> In theory it would have been nicer to use render_message_filter_linux.cc and
> render_message_filter_chromeos.cc but I don't mind, probably not worth anyway.

This code will likely go away within 6 months anyway. Also,
render_message_filter_gtk.cc is effectively render_message_filter_linux.cc.

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
File printing/printing_context_cairo.cc (right):

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
printing/printing_context_cairo.cc:43: : PrintingContext(app_locale) {
On 2011/02/15 14:18:23, Marc-Antoine Ruel (Google) wrote:
> align at +4

Done.

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
File printing/printing_context_cairo.h (right):

http://codereview.chromium.org/6516022/diff/14007/printing/printing_context_c...
printing/printing_context_cairo.h:24: // Set the function that creates the print
dialog, and the function that
On 2011/02/15 14:18:23, Marc-Antoine Ruel (Google) wrote:
> nits: Sets

Done.

[sanjeevr, 2011-02-17 21:09:48]

A few comments.

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:48: event->Wait();
What thread does this run on? Waiting for the document to be saved here is the
same as actually writing it to disk in this thread. Perhaps have a
callback-based mechanism?

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:271: page,
reinterpret_cast<PrintingContextCairo*>(printing_context_.get()));
Why is this cast done here and not within the implementation of
RenderPrintedPage?

http://codereview.chromium.org/6516022/diff/11014/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:129:
printed_page_params.metafile_data_handle.fd = -1;
Instead of sending these dummy DidPrintPage IPCs, maybe we should have a
separate IPC for Linux? (DidPrintPages perhaps?)

[Lei Zhang, 2011-02-18 00:10:08]

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:48: event->Wait();
On 2011/02/17 21:09:48, sanjeevr wrote:
> What thread does this run on? Waiting for the document to be saved here is the
> same as actually writing it to disk in this thread. Perhaps have a
> callback-based mechanism?

This is on the print worker thread. I understand it's no better than writing the
file on this thread, but I want to do file access on the right thread to keep
the thread restrictions code happy.

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/browser/printing/pri...
chrome/browser/printing/print_job_worker.cc:271: page,
reinterpret_cast<PrintingContextCairo*>(printing_context_.get()));
On 2011/02/17 21:09:48, sanjeevr wrote:
> Why is this cast done here and not within the implementation of
> RenderPrintedPage?

Fixed.

http://codereview.chromium.org/6516022/diff/11014/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/6516022/diff/11014/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:129:
printed_page_params.metafile_data_handle.fd = -1;
On 2011/02/17 21:09:48, sanjeevr wrote:
> Instead of sending these dummy DidPrintPage IPCs, maybe we should have a
> separate IPC for Linux? (DidPrintPages perhaps?)

It's probably more work to handle a new IPC than it is to add just a couple
checks for invalid handles. That is, it will make the browser-side print
workflow even more complicated.

[sanjeevr, 2011-02-18 22:47:08]

LGTM.

I still think you should use a separate IPC, but I am OK with this too :)

[James Hawkins, 2011-02-25 20:53:59]

http://codereview.chromium.org/6516022/diff/19003/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6516022/diff/19003/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:121: printing::PageRange* range =
new printing::PageRange;
You're leaking |range|. There's no need to make |range| dynamic here.