Added a diagnostic user message when enumerating printers fails. Also tweaked...

Added a diagnostic user message when enumerating printers fails. Also tweaked some of the strings.
BUG=None
TEST=Test Cloud Print Proxy.


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

[gene1, 2011-01-24 19:46:47]

a couple of lint errors too :)

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:410: if
(!result.succeeded() && !result.message().empty()) {
I think we should not use message here to decide whether to proceed with
registration or not. It should be possible to fail without a valid message.

Since we need a 3-state indicator here, let's return explicit 'partial_results'
flag, or at least check the returning list for the values in case of the error.

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:920: bool
CloudPrintProxyBackend::Core::OnPrinterNotFound(
I am confused with this function. Usually OnSomething is a notification and does
not return bool. Also, 'not found' looks opposite to 'complete list available'.

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
chrome/service/cloud_print/print_system_cups.cc:159: std::string
enum_failed_server_list_;
Can we keep it as a bool flag?
I understand that in the future we might want to report a list of servers, but
in that case I would prefer to keep the list of failed servers and convert them
to the comma-separated list right before sending to the upper level.

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
File chrome/service/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
chrome/service/cloud_print/printer_job_handler.h:97: virtual bool
OnPrinterNotFound(const std::string& printer_name) = 0;
OK, after reading the comment, I understand what OnPrinterNotFound() do :). Can
we rename function to be less confusing? Something like
HasCompletePrinterInfo(), or similar?

[sanjeevr, 2011-01-24 19:59:05]

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
File chrome/service/cloud_print/cloud_print_proxy_backend.cc (right):

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:410: if
(!result.succeeded() && !result.message().empty()) {
On 2011/01/24 19:46:47, gene1 wrote:
> I think we should not use message here to decide whether to proceed with
> registration or not. It should be possible to fail without a valid message.
> 
> Since we need a 3-state indicator here, let's return explicit
'partial_results'
> flag, or at least check the returning list for the values in case of the
error.

We can have a fallback message. I don't see why we need a 3-state indicator.

http://codereview.chromium.org/6356007/diff/1/chrome/service/cloud_print/clou...
chrome/service/cloud_print/cloud_print_proxy_backend.cc:920: bool
CloudPrintProxyBackend::Core::OnPrinterNotFound(
On 2011/01/24 19:46:47, gene1 wrote:
> I am confused with this function. Usually OnSomething is a notification and
does
> not return bool. Also, 'not found' looks opposite to 'complete list
available'.

I can pass an out param instead of returning a bool.

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
File chrome/service/cloud_print/printer_job_handler.h (right):

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
chrome/service/cloud_print/printer_job_handler.h:97: virtual bool
OnPrinterNotFound(const std::string& printer_name) = 0;
On 2011/01/24 19:46:47, gene1 wrote:
> OK, after reading the comment, I understand what OnPrinterNotFound() do :).
Can
> we rename function to be less confusing? Something like
> HasCompletePrinterInfo(), or similar?

But the check is not HasCompletePrinterInfo. The notification is as it stands.
The delegate may choose not to delete the printer from the server for other
reasons.

[Scott Byer, 2011-01-24 20:02:07]

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
File chrome/service/cloud_print/print_system_cups.cc (right):

http://codereview.chromium.org/6356007/diff/13001/chrome/service/cloud_print/...
chrome/service/cloud_print/print_system_cups.cc:159: std::string
enum_failed_server_list_;
Since it's not being used, how about making it a bool for now?

http://codereview.chromium.org/6356007/diff/13001/printing/backend/print_back...
File printing/backend/print_backend_win.cc (right):

http://codereview.chromium.org/6356007/diff/13001/printing/backend/print_back...
printing/backend/print_backend_win.cc:60: if (!bytes_needed)
Check the return value as well?

[sanjeevr, 2011-01-24 22:37:55]

Made the following changes:

1. Made the failure value in the CUPS print system a bool (instead of
maintaining a server list).
2. Used a fallback message if the message is empty.
3. Made |delete_from_server| an out argument.

Please take a look.

[gene1, 2011-01-24 23:05:52]

lgtm

On 2011/01/24 22:37:55, sanjeevr wrote:
> Made the following changes:
> 
> 1. Made the failure value in the CUPS print system a bool (instead of
> maintaining a server list).
> 2. Used a fallback message if the message is empty.
> 3. Made |delete_from_server| an out argument.
> 
> Please take a look.

[Scott Byer, 2011-01-24 23:20:41]

LGTM

On 2011/01/24 22:37:55, sanjeevr wrote:
> Made the following changes:
> 
> 1. Made the failure value in the CUPS print system a bool (instead of
> maintaining a server list).
> 2. Used a fallback message if the message is empty.
> 3. Made |delete_from_server| an out argument.
> 
> Please take a look.