Wire up printing on the Mac

Wire up printing on the Mac

Get the printing support class stack building and hooked up on the Mac.
Add support for creating NativeMetafile objects with PDF print data on the renderer side, and passing them to the browser via the existing printing IPC system.
Flip on the simpler printing unit tests (those that don't require PDF -> bitmap conversion).

BUG=13158
TEST=Print on the Mac--it should work!

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

[stuartmorgan, 2009-10-13 19:10:04]

Bugs will be filed for the remaining bits (sheets, fixing the nested message
loop in the render, etc.). Getting the bitmap-based unit tests working is
currently in progress.

[Amanda Walker, 2009-10-13 19:35:49]

LGTM with one typo, but wait for a LGTM from pink as well.  Nice work.

http://codereview.chromium.org/276004/diff/1/10
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/276004/diff/1/10#newcode851
Line 851: // TODO: Get an actually window ref here, to allow a sheet-based print
dialog.
actually -> actual

[pink (ping after 24hrs), 2009-10-13 21:04:51]

LGTM on the whole, lots of small things, but the thread question is the biggest.

http://codereview.chromium.org/276004/diff/1/4
File chrome/browser/printing/print_job.cc (right):

http://codereview.chromium.org/276004/diff/1/4#newcode287
Line 287: #if defined(OS_WIN)
add a todo to come back to this on mac and comment why we can get away with it
for now?

http://codereview.chromium.org/276004/diff/1/5
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/276004/diff/1/5#newcode119
Line 119: message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
so this will now run GetSettingsDone on the UI thread, correct? That's different
from where it runs on windows. Is that ok as well?

http://codereview.chromium.org/276004/diff/1/5#newcode130
Line 130: #if !defined(OS_MACOSX)
I see below why this the case, but it's 150 lines down the page. Is there any
way to make it more clear why there's all the extra ifdefs w/out having to find
the needle in the haystack?

http://codereview.chromium.org/276004/diff/1/6
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/276004/diff/1/6#newcode83
Line 83: // Asks the user for print settings. Must be called on the UI thread.
Perhaps add that this has to be added because it needs to be called on the main
UI thread, while on windows it can be called on a different thread. Your
existing comment implies that, but it took me some time to understand why win
didn't need this.

http://codereview.chromium.org/276004/diff/1/10
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/276004/diff/1/10#newcode360
Line 360: #endif
#elif?

http://codereview.chromium.org/276004/diff/1/11
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/276004/diff/1/11#newcode221
Line 221: #endif
#elif?

http://codereview.chromium.org/276004/diff/1/14
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/276004/diff/1/14#newcode1353
Line 1353: #endif  // defined(OS_WIN)
fix?

http://codereview.chromium.org/276004/diff/1/18
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/276004/diff/1/18#newcode31
Line 31: typedef PdfMetafile NativeMetafile;
add the TODO for the bug about typedef'ing to a common type?

http://codereview.chromium.org/276004/diff/1/20
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/276004/diff/1/20#newcode29
Line 29: // cross-platform file, and the slight divergences resolved/ifdef'd.
file a bug?

http://codereview.chromium.org/276004/diff/1/20#newcode29
Line 29: // cross-platform file, and the slight divergences resolved/ifdef'd.
is it possible to highlight the parts that are mac-specific such that we don't
drop the slight divergences when we do try to unify these into a single shared
routine?

[Amanda Walker, 2009-10-13 21:20:55]

http://codereview.chromium.org/276004/diff/1/5
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/276004/diff/1/5#newcode119
Line 119: message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
No, it'll run it on the PrintJobWorker thread.  message_loop() is this object's
message loop, not the UI loop.

[stuartmorgan, 2009-10-13 22:07:43]

http://codereview.chromium.org/276004/diff/1/4
File chrome/browser/printing/print_job.cc (right):

http://codereview.chromium.org/276004/diff/1/4#newcode287
Line 287: #if defined(OS_WIN)
On 2009/10/13 21:04:51, pink wrote:
> add a todo to come back to this on mac and comment why we can get away with it
> for now?

I don't think we'll ever need to come back to this; I've added a comment
explaining why.

http://codereview.chromium.org/276004/diff/1/5
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/276004/diff/1/5#newcode119
Line 119: message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
On 2009/10/13 21:20:55, Amanda Walker wrote:
> No, it'll run it on the PrintJobWorker thread.  message_loop() is this
object's
> message loop, not the UI loop.

Right; this is a PostTask rather than a straight call specifically to make sure
nothing is different from GetSettingsDone's perspective.

http://codereview.chromium.org/276004/diff/1/5#newcode130
Line 130: #if !defined(OS_MACOSX)
On 2009/10/13 21:04:51, pink wrote:
> I see below why this the case, but it's 150 lines down the page. Is there any
> way to make it more clear why there's all the extra ifdefs w/out having to
find
> the needle in the haystack?

I have a header comment on context() explaining when it's valid for the Mac
also; my feeling was that someone wondering why these are ifdef'd out would
presumably check the documentation for the call. I'd rather not replicate that
comment everywhere here, and that's the only central place I can see to put it.

http://codereview.chromium.org/276004/diff/1/6
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/276004/diff/1/6#newcode83
Line 83: // Asks the user for print settings. Must be called on the UI thread.
On 2009/10/13 21:04:51, pink wrote:
> Perhaps add that this has to be added because it needs to be called on the
main
> UI thread, while on windows it can be called on a different thread. Your
> existing comment implies that, but it took me some time to understand why win
> didn't need this.

Done.

http://codereview.chromium.org/276004/diff/1/10
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/276004/diff/1/10#newcode360
Line 360: #endif
On 2009/10/13 21:04:51, pink wrote:
> #elif?

Again, I was trying to be consistent with the rest of the #if's in this block.

http://codereview.chromium.org/276004/diff/1/10#newcode851
Line 851: // TODO: Get an actually window ref here, to allow a sheet-based print
dialog.
On 2009/10/13 19:35:49, Amanda Walker wrote:
> actually -> actual

Done.

http://codereview.chromium.org/276004/diff/1/11
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/276004/diff/1/11#newcode221
Line 221: #endif
On 2009/10/13 21:04:51, pink wrote:
> #elif?

I'm following the convention for this file, which is to let each platform #if
block stand alone. I'd rather not be inconsistent with, e.g., the WIN->LINUX
block transition directly above.

http://codereview.chromium.org/276004/diff/1/14
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/276004/diff/1/14#newcode1353
Line 1353: #endif  // defined(OS_WIN)
On 2009/10/13 21:04:51, pink wrote:
> fix?

Done.

http://codereview.chromium.org/276004/diff/1/18
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/276004/diff/1/18#newcode31
Line 31: typedef PdfMetafile NativeMetafile;
On 2009/10/13 21:04:51, pink wrote:
> add the TODO for the bug about typedef'ing to a common type?

This is actually cruft from an earlier rev that I missed during cleanup;
removed.

http://codereview.chromium.org/276004/diff/1/20
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/276004/diff/1/20#newcode29
Line 29: // cross-platform file, and the slight divergences resolved/ifdef'd.
On 2009/10/13 21:04:51, pink wrote:
> file a bug?

Will do.

http://codereview.chromium.org/276004/diff/1/20#newcode29
Line 29: // cross-platform file, and the slight divergences resolved/ifdef'd.
On 2009/10/13 21:04:51, pink wrote:
> is it possible to highlight the parts that are mac-specific such that we don't
> drop the slight divergences when we do try to unify these into a single shared
> routine?

That seems prone to getting out of date if tweaks are needed before then; the
comment calls out that there are differences, so anyone merging them
(realistically, probably me) should run the blocks through a diff tool.

I'll mention it in the bug too.