Print support for Windows Metro...

These are the Chrome side changes to support the Print device charm in Metro.

We now expose a print destination interface (not quite complete yet) to bypass the current print flow so that the metafile bits get sent to Metro instead of the print spooler. Other changes will come to make this more complete, e.g., pass in the destination object from the metro module, and expose print settings on the destination interface.


BUG=125675
TEST=Make sure you can still print as usual outside the Metro world.

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

[MAD, 2012-06-15 02:54:29]

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/app/chrome_c...
File chrome/app/chrome_command_ids.h (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/app/chrome_c...
chrome/app/chrome_command_ids.h:79: #define IDC_PRINT_TO_FILE              
35009
I want to change this to PRINT_TO_DELEGATE

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/prin...
File chrome/browser/printing/print_view_manager.h (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/prin...
chrome/browser/printing/print_view_manager.h:38: class Delegate : public
base::RefCountedThreadSafe<PrintJobWorkerOwner> {
Oopss... I'll need to remove this...

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.cc (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:912: OnPrintPages();
To be removed...

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.cc:1225: //  if
(!GetPrintSettingsFromUser(frame, node, expected_page_count,
Need to fix this....

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/renderer/pri...
File chrome/renderer/print_web_view_helper.h (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/renderer/pri...
chrome/renderer/print_web_view_helper.h:190: void OnPrintForFile();
To be removed..

[Albert Bodenhamer, 2012-06-15 18:41:24]

Some minor structural/naming comments, but other than that this looks pretty
good.

tl;dr version:
1: I don't like the word "Delegate" in a class name because it doesn't tell me
WHAT is being delegated or WHY.  I know it's a commonly used convention in
Chrome, but it makes the code much harder to understand.
2: Pull your nested classes out on their own.  It makes interacting with them
much simpler/cleaner.

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:29: #include "base/win/metro.h"
Safe to include this here?

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1621: void Browser::PrintToDelegate() {
Does this need to live in browser?  If you pull MetroPrintingDelegate into its
own file you can just have whoever calls this call
GetActiveTabContents()->print_view_manager()->PrintToDelegate directly.

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1624: class MetroPrintingDelegate : public
printing::PrintedDocument::Delegate {
Pull this into its own .cc/.h

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
File printing/printed_document.cc (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
printing/printed_document.cc:56: "Delegate is only supported on Windows for
now";
Is there any reason you need this?  The basic delegate concept is portable.  The
fact that the only implementer of this interface is Metro isn't something that
needs to be enforced is it?

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
File printing/printed_document.h (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
printing/printed_document.h:37: class Delegate: public
base::RefCountedThreadSafe<Delegate> {
Perhaps "PrintDestinationInterface" instead of "Delegate"?  I'd go as far as to
pull it into its own header.

[MAD, 2012-06-19 14:24:10]

Thanks Albert!

I'll start working on the final version now, and then ask for a more formal
review when done.

BYE
MAD...

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:29: #include "base/win/metro.h"
On 2012/06/15 18:41:24, Albert Bodenhamer wrote:
> Safe to include this here?

Right, I need to wrap it within #if defined (OS_WIN). Thanks!

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1621: void Browser::PrintToDelegate() {
Good point, will move it to a more sensible place... Thanks!

https://chromiumcodereview.appspot.com/10483006/diff/2001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1624: class MetroPrintingDelegate : public
printing::PrintedDocument::Delegate {
Yep, will do...

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
File printing/printed_document.cc (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
printing/printed_document.cc:56: "Delegate is only supported on Windows for
now";
Yeah, I wanted to avoid people trying to use it on other platforms where it's
not implemented... Will change that...

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
File printing/printed_document.h (right):

https://chromiumcodereview.appspot.com/10483006/diff/2001/printing/printed_do...
printing/printed_document.h:37: class Delegate: public
base::RefCountedThreadSafe<Delegate> {
Good idea... Will do...

[MAD, 2012-06-28 15:48:14]

Addressed all comments... and cleaned up some more!

This is now ready for a full formal code review...

Thanks!

BYE
MAD

http://codereview.chromium.org/10483006/diff/2001/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

http://codereview.chromium.org/10483006/diff/2001/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:79: #define IDC_PRINT_TO_FILE              
35009
On 2012/06/15 02:54:29, MAD wrote:
> I want to change this to PRINT_TO_DELEGATE

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.h (right):

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.h:38: class Delegate : public
base::RefCountedThreadSafe<PrintJobWorkerOwner> {
On 2012/06/15 02:54:29, MAD wrote:
> Oopss... I'll need to remove this...

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:29: #include "base/win/metro.h"
On 2012/06/19 14:24:10, MAD wrote:
> On 2012/06/15 18:41:24, Albert Bodenhamer wrote:
> > Safe to include this here?
> 
> Right, I need to wrap it within #if defined (OS_WIN). Thanks!
> 

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1621: void Browser::PrintToDelegate() {
On 2012/06/19 14:24:10, MAD wrote:
> Good point, will move it to a more sensible place... Thanks!

Actually, I would look the interface pointer to come with the message, so we
would need to pass it to the print_view_manager from here (unless we handle the
message somewhere else)... I agree that for now we can move this elsewhere,
we'll adapt it in a subsequent iteration...

http://codereview.chromium.org/10483006/diff/2001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1624: class MetroPrintingDelegate : public
printing::PrintedDocument::Delegate {
On 2012/06/19 14:24:10, MAD wrote:
> Yep, will do...

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.cc (right):

http://codereview.chromium.org/10483006/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:912: OnPrintPages();
On 2012/06/15 02:54:29, MAD wrote:
> To be removed...

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.cc:1225: //  if
(!GetPrintSettingsFromUser(frame, node, expected_page_count,
On 2012/06/15 02:54:29, MAD wrote:
> Need to fix this....

Done.

http://codereview.chromium.org/10483006/diff/2001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/10483006/diff/2001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:190: void OnPrintForFile();
On 2012/06/15 02:54:29, MAD wrote:
> To be removed..

Done.

http://codereview.chromium.org/10483006/diff/2001/printing/printed_document.cc
File printing/printed_document.cc (right):

http://codereview.chromium.org/10483006/diff/2001/printing/printed_document.c...
printing/printed_document.cc:56: "Delegate is only supported on Windows for
now";
On 2012/06/19 14:24:10, MAD wrote:
> Yeah, I wanted to avoid people trying to use it on other platforms where it's
> not implemented... Will change that...

Done.

http://codereview.chromium.org/10483006/diff/2001/printing/printed_document.h
File printing/printed_document.h (right):

http://codereview.chromium.org/10483006/diff/2001/printing/printed_document.h...
printing/printed_document.h:37: class Delegate: public
base::RefCountedThreadSafe<Delegate> {
On 2012/06/19 14:24:10, MAD wrote:
> Good idea... Will do...

Done.

[robertshield, 2012-06-28 17:48:54]

nits and such

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_manager.h (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_manager.h:43: // automatically reset to NULL
when the JOB_DONE event is received.
The last sentence describes implementation, not interface. Should remove from
here.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_manager.h:66: PrintDestinationInterface*
destination() const { return destination_.get(); }
Mention here that this may return NULL.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_worker.cc:74: DCHECK(destination_.get() ==
NULL);
Why is this DCHECK present? 

If it needs to be here, please document in the header that this method must only
be called once.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_worker.h:44: // This superceeds the document's
rendering destination.
superceed -> supercede

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:3358:
command_updater_.UpdateCommandEnabled(IDC_PRINT_TO_DESTINATION,
should this command be enabled under the same is-in-metro check as above?

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
File printing/print_destination_interface.h (right):

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
printing/print_destination_interface.h:18: virtual void SetPageContent(int
page_number,
brief comments on these methods?

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
File printing/print_destination_interface_win.cc (right):

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
printing/print_destination_interface_win.cc:45: return new
PrintDestinationInterfaceWin;
Should this do a base::win::IsMetroProcess() check and return NULL if we're not
a metro process?

[MAD, 2012-06-28 19:05:50]

Thanks, addressed all comments, and fixed a few clang/linux compile/link
failures...

Please take another look!

BYE
MAD...

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_manager.h (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_manager.h:43: // automatically reset to NULL
when the JOB_DONE event is received.
On 2012/06/28 17:48:54, robertshield wrote:
> The last sentence describes implementation, not interface. Should remove from
> here.
Good point, I thought the caller might need to know when the value is reset, but
I guess now, as long as we know it's just for the NEXT print job, it's fine...

Done!

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_manager.h:66: PrintDestinationInterface*
destination() const { return destination_.get(); }
On 2012/06/28 17:48:54, robertshield wrote:
> Mention here that this may return NULL.

Done.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_worker.cc (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_worker.cc:74: DCHECK(destination_.get() ==
NULL);
On 2012/06/28 17:48:54, robertshield wrote:
> Why is this DCHECK present? 
> 
> If it needs to be here, please document in the header that this method must
only
> be called once.
It was just for debugging, forgot to remove... Thanks!
Done.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
File chrome/browser/printing/print_job_worker.h (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/printing/pr...
chrome/browser/printing/print_job_worker.h:44: // This superceeds the document's
rendering destination.
On 2012/06/28 17:48:54, robertshield wrote:
> superceed -> supercede

Done.

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10483006/diff/22001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:3358:
command_updater_.UpdateCommandEnabled(IDC_PRINT_TO_DESTINATION,
On 2012/06/28 17:48:54, robertshield wrote:
> should this command be enabled under the same is-in-metro check as above?

Why? We may have other destinations, not just metro... Right?

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
File printing/print_destination_interface.h (right):

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
printing/print_destination_interface.h:18: virtual void SetPageContent(int
page_number,
On 2012/06/28 17:48:54, robertshield wrote:
> brief comments on these methods?

Done.

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
File printing/print_destination_interface_win.cc (right):

http://codereview.chromium.org/10483006/diff/22001/printing/print_destination...
printing/print_destination_interface_win.cc:45: return new
PrintDestinationInterfaceWin;
On 2012/06/28 17:48:54, robertshield wrote:
> Should this do a base::win::IsMetroProcess() check and return NULL if we're
not
> a metro process?

Done.

[robertshield, 2012-06-28 19:26:54]

lgtm

[MAD, 2012-06-28 19:36:16]

Adding Ben for a chrome/browser/ui OWNERS review since I added a new command for
printing to a specific destination. Feel free to deflect to another OWNER...

Albert, do you think I need another LGTM for the print stuff or is Robert's OK?

Thanks!

BYE
MAD...

[Albert Bodenhamer, 2012-06-28 19:38:01]

LGTM
Minor naming problem, but otherwise this looks great.

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
File printing/print_destination_interface_none.cc (right):

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
printing/print_destination_interface_none.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Provides a no-op implementation, but still an implementation.  should be
"print_destination_none.cc"

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
File printing/print_destination_interface_win.cc (right):

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
printing/print_destination_interface_win.cc:11: class
PrintDestinationInterfaceWin : public PrintDestinationInterface {
This class has implementation.  It isn't an interface.  Perhaps just call it
PrintDestinationWin?

[MAD, 2012-06-28 19:52:08]

Thanks Albert... All done!

Only missing browser/ui OWNERS' lgtm now...

BYE
MAD

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
File printing/print_destination_interface_none.cc (right):

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
printing/print_destination_interface_none.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/06/28 19:38:02, Albert Bodenhamer wrote:
> Provides a no-op implementation, but still an implementation.  should be
> "print_destination_none.cc"

Done.

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
File printing/print_destination_interface_win.cc (right):

http://codereview.chromium.org/10483006/diff/22004/printing/print_destination...
printing/print_destination_interface_win.cc:11: class
PrintDestinationInterfaceWin : public PrintDestinationInterface {
On 2012/06/28 19:38:02, Albert Bodenhamer wrote:
> This class has implementation.  It isn't an interface.  Perhaps just call it
> PrintDestinationWin?

Done.

[Ben Goodger (Google), 2012-06-29 15:48:53]

LGTM for browser/ui

[MAD, 2012-06-29 17:34:12]

On 2012/06/29 15:48:53, Ben Goodger (Google) wrote:
> LGTM for browser/ui

Thanks! CQ'ing...

[commit-bot: I haz the power, 2012-06-29 17:34:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mad@chromium.org/10483006/37023

[commit-bot: I haz the power, 2012-06-29 18:58:35]

Change committed as 144943