Redesign cloud printing subsystem layer.

On behalf of gene@chromium.org. Redesign cloud printing subsystem layer. Make it a class, and allow to have internal state. Also, some minor improvements.

BUG=none
TEST=Try run Windows cloud print proxy and confirm documents get printed.

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

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

[gene, 2010-06-03 00:23:45]

Preliminary CL. Please check common and windows parts. In the mean time I'll
work on Linux CUPS changes.

[sanjeevr, 2010-06-03 06:36:23]

Some initial comments.

http://codereview.chromium.org/2519002/diff/26001/27003
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2519002/diff/26001/27003#newcode232
chrome/service/cloud_print/cloud_print_proxy_backend.cc:232: if (print_system_
!= NULL)
Please add a NOTREACHED if print_system_ is NULL. The cloud print proxy code
should not be invoked on OSes that do not have a print system.

Also, the PrintSystem should get created in the background thread which means
you should do it in the Initialize* methods. And if creation of the print system
fails, Initialize should also fail (similar to the case when authentication
fails).

http://codereview.chromium.org/2519002/diff/26001/27003#newcode366
chrome/service/cloud_print/cloud_print_proxy_backend.cc:366: if (print_system_
!= NULL)
Initialize should fail if print_system_ is NULL. Then you don't need to check
for non-NULL print_system_ everywhere.

http://codereview.chromium.org/2519002/diff/26001/27005
File chrome/service/cloud_print/job_status_updater.cc (right):

http://codereview.chromium.org/2519002/diff/26001/27005#newcode26
chrome/service/cloud_print/job_status_updater.cc:26: DCHECK(delegate_);
DCHECK(print_system_);

http://codereview.chromium.org/2519002/diff/26001/27005#newcode43
chrome/service/cloud_print/job_status_updater.cc:43: if (print_system_ &&
Don't check if (print_system_)

http://codereview.chromium.org/2519002/diff/26001/27007
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/2519002/diff/26001/27007#newcode76
chrome/service/cloud_print/print_system.h:76: class PrintSystem {
Many classes store a pointer to this. Please make this an RefCounted object and
make sure all classes use a scoped_refptr<PrintSystem>.

Incidentally, the way this is done in other places in Chrome (as an example, see
src/printing/printing_context.h) is that there is only one class and the methods
are not pure virtual. There are just 3 different implementations of the same
class on 3 different platforms. If you want to store state that is specific to a
platform, you can have an opaque PrintSystemState* member that each
implementation can define separately. Another way is to define certain member
variables under an #if defined(OS_XXX) block. That way there is no special
GetPrintSystem method, callers would just use new PrintSystem. I am not saying
we HAVE to follow the same mechanism but since this is commonly used everywhere
in Chrome, we should look into the pros and cons. The advantages with your
approach are if we ever have the need for multiple PrintSystems on the same OS
(but this is unlikely?) and also you can do without a dummy implementation. But
I think the right way to avoid the dummy is to not include the cloud print proxy
related files for OSes such as Chrome OS. wdyt?

http://codereview.chromium.org/2519002/diff/26001/27010
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/2519002/diff/26001/27010#newcode131
chrome/service/cloud_print/print_system_win.cc:131: class NotificationState
You don't need this class anymore. You can moved it entirely inside
WindowsPrintServerWatcher.

http://codereview.chromium.org/2519002/diff/26001/27010#newcode240
chrome/service/cloud_print/print_system_win.cc:240: class WindowsPrintSystem :
public PrintSystem {
Please name this PrintSystemWin to maintain consistency with other classes.

http://codereview.chromium.org/2519002/diff/26001/27015
File chrome/service/cloud_print/printer_job_handler.cc (right):

http://codereview.chromium.org/2519002/diff/26001/27015#newcode126
chrome/service/cloud_print/printer_job_handler.cc:126: if (!print_system_ ||
printer_watcher_ == NULL)
Just DCHECK on these, no need to return false if they are NULL. They should not
be NULL.

http://codereview.chromium.org/2519002/diff/26001/27015#newcode359
chrome/service/cloud_print/printer_job_handler.cc:359: if (!print_system_)
Same here

[gene, 2010-06-03 18:31:31]

Done and uploaded.
Please take a look.

On 2010/06/03 06:36:23, sanjeevr wrote:
> Some initial comments.
> 
> http://codereview.chromium.org/2519002/diff/26001/27003
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/26001/27003#newcode232
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:232: if (print_system_
> != NULL)
> Please add a NOTREACHED if print_system_ is NULL. The cloud print proxy code
> should not be invoked on OSes that do not have a print system.
> 
> Also, the PrintSystem should get created in the background thread which means
> you should do it in the Initialize* methods. And if creation of the print
system
> fails, Initialize should also fail (similar to the case when authentication
> fails).
> 
> http://codereview.chromium.org/2519002/diff/26001/27003#newcode366
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:366: if (print_system_
> != NULL)
> Initialize should fail if print_system_ is NULL. Then you don't need to check
> for non-NULL print_system_ everywhere.
> 
> http://codereview.chromium.org/2519002/diff/26001/27005
> File chrome/service/cloud_print/job_status_updater.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/26001/27005#newcode26
> chrome/service/cloud_print/job_status_updater.cc:26: DCHECK(delegate_);
> DCHECK(print_system_);
> 
> http://codereview.chromium.org/2519002/diff/26001/27005#newcode43
> chrome/service/cloud_print/job_status_updater.cc:43: if (print_system_ &&
> Don't check if (print_system_)
> 
> http://codereview.chromium.org/2519002/diff/26001/27007
> File chrome/service/cloud_print/print_system.h (right):
> 
> http://codereview.chromium.org/2519002/diff/26001/27007#newcode76
> chrome/service/cloud_print/print_system.h:76: class PrintSystem {
> Many classes store a pointer to this. Please make this an RefCounted object
and
> make sure all classes use a scoped_refptr<PrintSystem>.
> 
> Incidentally, the way this is done in other places in Chrome (as an example,
see
> src/printing/printing_context.h) is that there is only one class and the
methods
> are not pure virtual. There are just 3 different implementations of the same
> class on 3 different platforms. If you want to store state that is specific to
a
> platform, you can have an opaque PrintSystemState* member that each
> implementation can define separately. Another way is to define certain member
> variables under an #if defined(OS_XXX) block. That way there is no special
> GetPrintSystem method, callers would just use new PrintSystem. I am not saying
> we HAVE to follow the same mechanism but since this is commonly used
everywhere
> in Chrome, we should look into the pros and cons. The advantages with your
> approach are if we ever have the need for multiple PrintSystems on the same OS
> (but this is unlikely?) and also you can do without a dummy implementation.
But
> I think the right way to avoid the dummy is to not include the cloud print
proxy
> related files for OSes such as Chrome OS. wdyt?
> 
> http://codereview.chromium.org/2519002/diff/26001/27010
> File chrome/service/cloud_print/print_system_win.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/26001/27010#newcode131
> chrome/service/cloud_print/print_system_win.cc:131: class NotificationState
> You don't need this class anymore. You can moved it entirely inside
> WindowsPrintServerWatcher.
> 
> http://codereview.chromium.org/2519002/diff/26001/27010#newcode240
> chrome/service/cloud_print/print_system_win.cc:240: class WindowsPrintSystem :
> public PrintSystem {
> Please name this PrintSystemWin to maintain consistency with other classes.
> 
> http://codereview.chromium.org/2519002/diff/26001/27015
> File chrome/service/cloud_print/printer_job_handler.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/26001/27015#newcode126
> chrome/service/cloud_print/printer_job_handler.cc:126: if (!print_system_ ||
> printer_watcher_ == NULL)
> Just DCHECK on these, no need to return false if they are NULL. They should
not
> be NULL.
> 
> http://codereview.chromium.org/2519002/diff/26001/27015#newcode359
> chrome/service/cloud_print/printer_job_handler.cc:359: if (!print_system_)
> Same here

[sanjeevr, 2010-06-03 21:32:42]

http://codereview.chromium.org/2519002/diff/39001/40003
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2519002/diff/39001/40003#newcode279
chrome/service/cloud_print/cloud_print_proxy_backend.cc:279: if
(!print_system_.get())
NOTREACHED here. Perhaps even a CHECK.

http://codereview.chromium.org/2519002/diff/39001/40006
File chrome/service/cloud_print/job_status_updater.h (right):

http://codereview.chromium.org/2519002/diff/39001/40006#newcode35
chrome/service/cloud_print/job_status_updater.h:35:
scoped_refptr<cloud_print::PrintSystem> print_system,
The input argument can be a PrintSystem*

http://codereview.chromium.org/2519002/diff/39001/40007
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/2519002/diff/39001/40007#newcode112
chrome/service/cloud_print/print_system.h:112: class PrintServerWatcher {
Please derive this from base::NonThreadSafe

http://codereview.chromium.org/2519002/diff/39001/40007#newcode126
chrome/service/cloud_print/print_system.h:126: class PrinterWatcher {
Please derive this from base::NonThreadSafe

http://codereview.chromium.org/2519002/diff/39001/40009
File chrome/service/cloud_print/print_system_dummy.cc (right):

http://codereview.chromium.org/2519002/diff/39001/40009#newcode21
chrome/service/cloud_print/print_system_dummy.cc:21: NOTIMPLEMENTED();
Nit: This should be NOTREACHED(); NOTIMPLEMENTED() implies that we will
implement this later.

http://codereview.chromium.org/2519002/diff/39001/40010
File chrome/service/cloud_print/print_system_win.cc (right):

http://codereview.chromium.org/2519002/diff/39001/40010#newcode317
chrome/service/cloud_print/print_system_win.cc:317: virtual void
OnPrinterAdded() {}
NOTREACHED here

http://codereview.chromium.org/2519002/diff/39001/40016
File chrome/service/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/2519002/diff/39001/40016#newcode99
chrome/service/cloud_print/printer_job_handler.h:99:
scoped_refptr<cloud_print::PrintSystem> print_system,
Pass in PrintSystem*

[gene, 2010-06-03 21:47:22]

Fixed and uploaded.
Please take a look.

On 2010/06/03 21:32:42, sanjeevr wrote:
> http://codereview.chromium.org/2519002/diff/39001/40003
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40003#newcode279
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:279: if
> (!print_system_.get())
> NOTREACHED here. Perhaps even a CHECK.
> 
> http://codereview.chromium.org/2519002/diff/39001/40006
> File chrome/service/cloud_print/job_status_updater.h (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40006#newcode35
> chrome/service/cloud_print/job_status_updater.h:35:
> scoped_refptr<cloud_print::PrintSystem> print_system,
> The input argument can be a PrintSystem*
> 
> http://codereview.chromium.org/2519002/diff/39001/40007
> File chrome/service/cloud_print/print_system.h (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40007#newcode112
> chrome/service/cloud_print/print_system.h:112: class PrintServerWatcher {
> Please derive this from base::NonThreadSafe
> 
> http://codereview.chromium.org/2519002/diff/39001/40007#newcode126
> chrome/service/cloud_print/print_system.h:126: class PrinterWatcher {
> Please derive this from base::NonThreadSafe
> 
> http://codereview.chromium.org/2519002/diff/39001/40009
> File chrome/service/cloud_print/print_system_dummy.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40009#newcode21
> chrome/service/cloud_print/print_system_dummy.cc:21: NOTIMPLEMENTED();
> Nit: This should be NOTREACHED(); NOTIMPLEMENTED() implies that we will
> implement this later.
> 
> http://codereview.chromium.org/2519002/diff/39001/40010
> File chrome/service/cloud_print/print_system_win.cc (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40010#newcode317
> chrome/service/cloud_print/print_system_win.cc:317: virtual void
> OnPrinterAdded() {}
> NOTREACHED here
> 
> http://codereview.chromium.org/2519002/diff/39001/40016
> File chrome/service/cloud_print/printer_job_handler.h (right):
> 
> http://codereview.chromium.org/2519002/diff/39001/40016#newcode99
> chrome/service/cloud_print/printer_job_handler.h:99:
> scoped_refptr<cloud_print::PrintSystem> print_system,
> Pass in PrintSystem*

[gene, 2010-06-03 23:11:57]

Added CUPS print system. Please take a look.
I'll test both (mac & linux) proxies in the mean time.

On 2010/06/03 21:47:22, gene wrote:
> Fixed and uploaded.
> Please take a look.
> 
> On 2010/06/03 21:32:42, sanjeevr wrote:
> > http://codereview.chromium.org/2519002/diff/39001/40003
> > File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40003#newcode279
> > chrome/service/cloud_print/cloud_print_proxy_backend.cc:279: if
> > (!print_system_.get())
> > NOTREACHED here. Perhaps even a CHECK.
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40006
> > File chrome/service/cloud_print/job_status_updater.h (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40006#newcode35
> > chrome/service/cloud_print/job_status_updater.h:35:
> > scoped_refptr<cloud_print::PrintSystem> print_system,
> > The input argument can be a PrintSystem*
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40007
> > File chrome/service/cloud_print/print_system.h (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40007#newcode112
> > chrome/service/cloud_print/print_system.h:112: class PrintServerWatcher {
> > Please derive this from base::NonThreadSafe
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40007#newcode126
> > chrome/service/cloud_print/print_system.h:126: class PrinterWatcher {
> > Please derive this from base::NonThreadSafe
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40009
> > File chrome/service/cloud_print/print_system_dummy.cc (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40009#newcode21
> > chrome/service/cloud_print/print_system_dummy.cc:21: NOTIMPLEMENTED();
> > Nit: This should be NOTREACHED(); NOTIMPLEMENTED() implies that we will
> > implement this later.
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40010
> > File chrome/service/cloud_print/print_system_win.cc (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40010#newcode317
> > chrome/service/cloud_print/print_system_win.cc:317: virtual void
> > OnPrinterAdded() {}
> > NOTREACHED here
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40016
> > File chrome/service/cloud_print/printer_job_handler.h (right):
> > 
> > http://codereview.chromium.org/2519002/diff/39001/40016#newcode99
> > chrome/service/cloud_print/printer_job_handler.h:99:
> > scoped_refptr<cloud_print::PrintSystem> print_system,
> > Pass in PrintSystem*

[sanjeevr, 2010-06-04 18:31:24]

LGTM with one nit.

http://codereview.chromium.org/2519002/diff/68001/69003
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2519002/diff/68001/69003#newcode233
chrome/service/cloud_print/cloud_print_proxy_backend.cc:233:
CloudPrintProxyBackend::Core::~Core() {
You don't need a d'tor.

[gene1, 2010-06-04 18:46:41]

done and uploaded.
Also changed description

On Fri, Jun 4, 2010 at 11:31 AM, <sanjeevr@chromium.org> wrote:

> LGTM with one nit.
>
>
> http://codereview.chromium.org/2519002/diff/68001/69003
>
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
>
> http://codereview.chromium.org/2519002/diff/68001/69003#newcode233
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:233:
> CloudPrintProxyBackend::Core::~Core() {
> You don't need a d'tor.
>
>
> http://codereview.chromium.org/2519002/show
>

[gene, 2010-06-04 22:06:15]

Added error handler for GetPrinterCapsAndDefaults.
Please check

On 2010/06/04 18:46:41, gene1 wrote:
> done and uploaded.
> Also changed description
> 
> On Fri, Jun 4, 2010 at 11:31 AM, <mailto:sanjeevr@chromium.org> wrote:
> 
> > LGTM with one nit.
> >
> >
> > http://codereview.chromium.org/2519002/diff/68001/69003
> >
> > File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
> >
> > http://codereview.chromium.org/2519002/diff/68001/69003#newcode233
> > chrome/service/cloud_print/cloud_print_proxy_backend.cc:233:
> > CloudPrintProxyBackend::Core::~Core() {
> > You don't need a d'tor.
> >
> >
> > http://codereview.chromium.org/2519002/show
> >
>

[sanjeevr, 2010-06-04 22:59:51]

http://codereview.chromium.org/2519002/diff/80004/75004
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/2519002/diff/80004/75004#newcode443
chrome/service/cloud_print/cloud_print_proxy_backend.cc:443:
&CloudPrintProxyBackend::Core::RegisterNextPrinter);
Nit: Don't use a next_task variable. Just use NewRunnableMethod as an argument
to PostTask.

http://codereview.chromium.org/2519002/diff/80004/75009
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/2519002/diff/80004/75009#newcode182
chrome/service/cloud_print/print_system_cups.cc:182: if (ppd_file_path == NULL)
{
Don't check ppd_file_path. It is a pointer owned by CUPs and we have already
released the lock here. Check for ppd_path.empty() instead.

[gene1, 2010-06-04 23:04:39]

Done. Please take a look.

On Fri, Jun 4, 2010 at 3:59 PM, <sanjeevr@chromium.org> wrote:

>
> http://codereview.chromium.org/2519002/diff/80004/75004
>
> File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):
>
> http://codereview.chromium.org/2519002/diff/80004/75004#newcode443
> chrome/service/cloud_print/cloud_print_proxy_backend.cc:443:
> &CloudPrintProxyBackend::Core::RegisterNextPrinter);
> Nit: Don't use a next_task variable. Just use NewRunnableMethod as an
> argument to PostTask.
>
> http://codereview.chromium.org/2519002/diff/80004/75009
> File chrome/service/cloud_print/print_system_cups.cc (right):
>
> http://codereview.chromium.org/2519002/diff/80004/75009#newcode182
> chrome/service/cloud_print/print_system_cups.cc:182: if (ppd_file_path
> == NULL) {
> Don't check ppd_file_path. It is a pointer owned by CUPs and we have
> already released the lock here. Check for ppd_path.empty() instead.
>
>
> http://codereview.chromium.org/2519002/show
>

[sanjeevr, 2010-06-04 23:13:27]

LGTM