Make CUPS cloud print proxy to support multiple print servers....

Make CUPS cloud print proxy to support multiple print servers.
Add notifications for printer settings changed, and periodic update to check for new printers.

BUG=none
TEST=Make sure CP proxy works on Linux.


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

[sanjeevr, 2010-11-03 17:23:58]

Overall, this looks good. A few comments.

http://codereview.chromium.org/4233004/diff/14001/15002
File chrome/service/cloud_print/print_system.h (right):

http://codereview.chromium.org/4233004/diff/14001/15002#newcode135
chrome/service/cloud_print/print_system.h:135: virtual void Init() = 0;
Nit: What does Init() really do? Is it always needed? (for example something
like ValidatePrintTicket and GenerateProxyId may not need Init). Perhaps we
should not call this Init()?

http://codereview.chromium.org/4233004/diff/14001/15003
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/4233004/diff/14001/15003#newcode111
chrome/service/cloud_print/print_system_cups.cc:111: typedef
std::map<std::string, PrintServerInfoCUPS*> PrintersMap;
Please document what these different data structures represent and why you need
them (for example the printer_map_ is purely for optimization)

http://codereview.chromium.org/4233004/diff/14001/15003#newcode326
chrome/service/cloud_print/print_system_cups.cc:326: NOTREACHED();
You don't need a NOTREACHED here

http://codereview.chromium.org/4233004/diff/14001/15003#newcode332
chrome/service/cloud_print/print_system_cups.cc:332:
AddPrintServer(std::string());
You can make this code common so you call AddPrintServer with an empty string in
only one place.

http://codereview.chromium.org/4233004/diff/14001/15003#newcode387
chrome/service/cloud_print/print_system_cups.cc:387: void
PrintSystemCUPS::EnumeratePrinters(printing::PrinterList* printer_list) {
While we are doing this, can we make EnumeratePrinters return a bool value, so
we know not to delete the printers from the server?

[gene1, 2010-11-03 18:26:34]

Mostly done. Some comments inline.

On Wed, Nov 3, 2010 at 10:23 AM, <sanjeevr@chromium.org> wrote:

> Overall, this looks good. A few comments.
>
>
> http://codereview.chromium.org/4233004/diff/14001/15002
> File chrome/service/cloud_print/print_system.h (right):
>
> http://codereview.chromium.org/4233004/diff/14001/15002#newcode135
> chrome/service/cloud_print/print_system.h:135: virtual void Init() = 0;
> Nit: What does Init() really do? Is it always needed? (for example
> something like ValidatePrintTicket and GenerateProxyId may not need
> Init). Perhaps we should not call this Init()?
>

 GenerateProxyId does not need init anyways. Caller should use
PrintSystem::GenerateProxyId().
ValidatePrintTicket technically does not need Init() today as well, but this
is just implementation detail.
It takes printer name and can check for printer to be valid, for example:
if (!IsValidPrinter(printer_name)) return false;
In that case Init() call will be required. I would prefer to leave name as
Init() here.

>
> http://codereview.chromium.org/4233004/diff/14001/15003
> File chrome/service/cloud_print/print_system_cups.cc (right):
>
> http://codereview.chromium.org/4233004/diff/14001/15003#newcode111
> chrome/service/cloud_print/print_system_cups.cc:111: typedef
> std::map<std::string, PrintServerInfoCUPS*> PrintersMap;
> Please document what these different data structures represent and why
> you need them (for example the printer_map_ is purely for optimization)
>
done

>
> http://codereview.chromium.org/4233004/diff/14001/15003#newcode326
> chrome/service/cloud_print/print_system_cups.cc:326: NOTREACHED();
> You don't need a NOTREACHED here
>
done

>
> http://codereview.chromium.org/4233004/diff/14001/15003#newcode332
> chrome/service/cloud_print/print_system_cups.cc:332:
> AddPrintServer(std::string());
> You can make this code common so you call AddPrintServer with an empty
> string in only one place.
>
done

>
> http://codereview.chromium.org/4233004/diff/14001/15003#newcode387
> chrome/service/cloud_print/print_system_cups.cc:387: void
> PrintSystemCUPS::EnumeratePrinters(printing::PrinterList* printer_list)
> {
> While we are doing this, can we make EnumeratePrinters return a bool
> value, so we know not to delete the printers from the server?


I am looking at proxy backend code and I want to double check this with you.
I was thinking about replacing:
  print_system_->EnumeratePrinters(&printer_list_);
with:
  if (!print_system_->EnumeratePrinters(&printer_list_)) {
    MessageLoop::current()->PostDelayedTask(
        FROM_HERE,
        NewRunnableMethod(this,
&CloudPrintProxyBackend::Core::StartRegistration),
        5 * 60 * 1000);
    return;
  }

How does this sounds?


>
>
> http://codereview.chromium.org/4233004/show
>

[sanjeevr, 2010-11-03 19:15:31]

LGTM