Used the service utility process host to render PDFs in a sandbox for the Win...

Used the service utility process host to render PDFs in a sandbox for the Windows cloud print proxy. Also made the print spooling asynchronous.
BUG=None
TEST=Test cloud print proxy on all supported platforms.


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

[gene, 2010-07-20 18:54:11]

http://codereview.chromium.org/3051006/diff/7001/8001
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/3051006/diff/7001/8001#newcode149
chrome/service/cloud_print/print_system.h:149: virtual bool Spool(const
std::string& print_ticket,
If this interface does not allow multiple spooling, you may indicate it in the
comment.

http://codereview.chromium.org/3051006/diff/7001/8002
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3051006/diff/7001/8002#newcode225
chrome/service/cloud_print/print_system_cups.cc:225: bool res =
ParsePrintTicket(print_ticket, &options);
ParsePrintTicket is a function of the PrintSystemCUPS and is not available here.

http://codereview.chromium.org/3051006/diff/7001/8002#newcode237
chrome/service/cloud_print/print_system_cups.cc:237: int job_id =
PrintFile(printer_name.c_str(),
PrintFile is a function of PrintSystemCUPS, and not accessible from here. You
may move that function to JobSpoolerCUPS.

[sanjeevr, 2010-07-20 20:14:23]

Made the changes.

http://codereview.chromium.org/3051006/diff/7001/8001
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/3051006/diff/7001/8001#newcode149
chrome/service/cloud_print/print_system.h:149: virtual bool Spool(const
std::string& print_ticket,
On 2010/07/20 18:54:11, gene wrote:
> If this interface does not allow multiple spooling, you may indicate it in the
> comment.

Done.

http://codereview.chromium.org/3051006/diff/7001/8002
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3051006/diff/7001/8002#newcode225
chrome/service/cloud_print/print_system_cups.cc:225: bool res =
ParsePrintTicket(print_ticket, &options);
On 2010/07/20 18:54:11, gene wrote:
> ParsePrintTicket is a function of the PrintSystemCUPS and is not available
here.

Passed in a PrintSystemCUPS pointer to this class.

http://codereview.chromium.org/3051006/diff/7001/8002#newcode237
chrome/service/cloud_print/print_system_cups.cc:237: int job_id =
PrintFile(printer_name.c_str(),
On 2010/07/20 18:54:11, gene wrote:
> PrintFile is a function of PrintSystemCUPS, and not accessible from here. You
> may move that function to JobSpoolerCUPS.

Done.

[M-A Ruel, 2010-07-20 20:55:57]

only nits, lgtm to me.

http://codereview.chromium.org/3051006/diff/3002/13001
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/3051006/diff/3002/13001#newcode136
chrome/service/cloud_print/print_system.h:136: class JobSpooler : public
base::RefCountedThreadSafe<JobSpooler> {
That's outside the scope of this change but the inner classes should be at the
beginning of the outer class.

http://codereview.chromium.org/3051006/diff/3002/13002
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3051006/diff/3002/13002#newcode265
chrome/service/cloud_print/print_system_cups.cc:265: };
is this class copyable?

http://codereview.chromium.org/3051006/diff/3002/13003
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/3051006/diff/3002/13003#newcode417
chrome/service/cloud_print/print_system_win.cc:417: last_page_printed_ = -1;
I think you should do that first and set delegate_ = NULL at the top of the
function in case things go wrong.

http://codereview.chromium.org/3051006/diff/3002/13005
File chrome/service/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/3051006/diff/3002/13005#newcode243
chrome/service/cloud_print/printer_job_handler.h:243: // The message loop proxy
reprensenting the thread on which this object
representing

[sanjeevr, 2010-07-21 01:03:11]

Made the changes.

http://codereview.chromium.org/3051006/diff/3002/13001
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/3051006/diff/3002/13001#newcode136
chrome/service/cloud_print/print_system.h:136: class JobSpooler : public
base::RefCountedThreadSafe<JobSpooler> {
On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> That's outside the scope of this change but the inner classes should be at the
> beginning of the outer class.

Done. Moved them.

http://codereview.chromium.org/3051006/diff/3002/13002
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/3051006/diff/3002/13002#newcode265
chrome/service/cloud_print/print_system_cups.cc:265: };
On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> is this class copyable?

Done.

http://codereview.chromium.org/3051006/diff/3002/13003
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/3051006/diff/3002/13003#newcode417
chrome/service/cloud_print/print_system_win.cc:417: last_page_printed_ = -1;
On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> I think you should do that first and set delegate_ = NULL at the top of the
> function in case things go wrong.

Moved last_page_printed_ above. If delegate_ is non-NULL we return false at the
top.

http://codereview.chromium.org/3051006/diff/3002/13005
File chrome/service/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/3051006/diff/3002/13005#newcode243
chrome/service/cloud_print/printer_job_handler.h:243: // The message loop proxy
reprensenting the thread on which this object
On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> representing

Done.

[gene, 2010-07-21 18:19:49]

LGTM

On 2010/07/21 01:03:11, sanjeevr wrote:
> Made the changes.
> 
> http://codereview.chromium.org/3051006/diff/3002/13001
> File chrome/service/cloud_print/print_system.h (right):
> 
> http://codereview.chromium.org/3051006/diff/3002/13001#newcode136
> chrome/service/cloud_print/print_system.h:136: class JobSpooler : public
> base::RefCountedThreadSafe<JobSpooler> {
> On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> > That's outside the scope of this change but the inner classes should be at
the
> > beginning of the outer class.
> 
> Done. Moved them.
> 
> http://codereview.chromium.org/3051006/diff/3002/13002
> File chrome/service/cloud_print/print_system_cups.cc (right):
> 
> http://codereview.chromium.org/3051006/diff/3002/13002#newcode265
> chrome/service/cloud_print/print_system_cups.cc:265: };
> On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> > is this class copyable?
> 
> Done.
> 
> http://codereview.chromium.org/3051006/diff/3002/13003
> File chrome/service/cloud_print/print_system_win.cc (right):
> 
> http://codereview.chromium.org/3051006/diff/3002/13003#newcode417
> chrome/service/cloud_print/print_system_win.cc:417: last_page_printed_ = -1;
> On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> > I think you should do that first and set delegate_ = NULL at the top of the
> > function in case things go wrong.
> 
> Moved last_page_printed_ above. If delegate_ is non-NULL we return false at
the
> top.
> 
> http://codereview.chromium.org/3051006/diff/3002/13005
> File chrome/service/cloud_print/printer_job_handler.h (right):
> 
> http://codereview.chromium.org/3051006/diff/3002/13005#newcode243
> chrome/service/cloud_print/printer_job_handler.h:243: // The message loop
proxy
> reprensenting the thread on which this object
> On 2010/07/20 20:55:57, Marc-Antoine Ruel wrote:
> > representing
> 
> Done.