CL for linux proxy for cloud printing

Cloud Printing proxy implementation on Linux using CUPS.

BUG=none
TEST=Try running cloud print proxy on linux and confirm printer list get uploaded, jobs get processed and printed.

Landed in revision 47300

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

[gene, 2010-05-04 22:26:29]

CL for linux proxy for cloud printing

[sanjeevr, 2010-05-04 23:25:21]

Some initial comments.

http://codereview.chromium.org/1933006/diff/3001/4
File chrome/browser/printing/cloud_print/printer_info_linux.cc (right):

http://codereview.chromium.org/1933006/diff/3001/4#newcode22
chrome/browser/printing/cloud_print/printer_info_linux.cc:22: bool
ReadFileToString(const char* filename, std::string* content) {
Please use the ReadFileToString method in base/file_util.h.

http://codereview.chromium.org/1933006/diff/3001/4#newcode60
chrome/browser/printing/cloud_print/printer_info_linux.cc:60: if
(printer_info.printer_name == "chanticleer")
Please remove this

http://codereview.chromium.org/1933006/diff/3001/4#newcode70
chrome/browser/printing/cloud_print/printer_info_linux.cc:70: const char*
ppd_filename = cupsGetPPD(printer_name.c_str());
Who allocates this?

http://codereview.chromium.org/1933006/diff/3001/4#newcode75
chrome/browser/printing/cloud_print/printer_info_linux.cc:75:
remove(ppd_filename);
Please use a base utility for this.

http://codereview.chromium.org/1933006/diff/3001/4#newcode80
chrome/browser/printing/cloud_print/printer_info_linux.cc:80:
printer_info->printer_defaults.clear();
Will we ever support defaults, if yes, please add a TODO. If not, please mention
that here in a comment.

http://codereview.chromium.org/1933006/diff/3001/4#newcode96
chrome/browser/printing/cloud_print/printer_info_linux.cc:96: return
std::string("LinuxCUPSproxy");
Please change this.

http://codereview.chromium.org/1933006/diff/3001/4#newcode130
chrome/browser/printing/cloud_print/printer_info_linux.cc:130:
std::map<std::string, std::string> options;
You don't need a map. DictionaryValue provides a key_iterator.

http://codereview.chromium.org/1933006/diff/3001/4#newcode198
chrome/browser/printing/cloud_print/printer_info_linux.cc:198:
EnumeratePrinters(&printer_list);
This seems a very inefficient way to get info for a specific printer.

http://codereview.chromium.org/1933006/diff/3001/4#newcode240
chrome/browser/printing/cloud_print/printer_info_linux.cc:240:
delegate_->OnJobChanged();
Please comment here about this hack in detail.

http://codereview.chromium.org/1933006/diff/3001/4#newcode251
chrome/browser/printing/cloud_print/printer_info_linux.cc:251:
ScopedRunnableMethodFactory<PrinterChangeNotifier::NotificationState>
You don't need a method factory. You can just use NewRunnableMethod. See
examples in cloud_print_proxy_backend.cc.

[gene, 2010-05-07 00:16:22]

http://codereview.chromium.org/1933006/diff/3001/4
File chrome/browser/printing/cloud_print/printer_info_linux.cc (right):

http://codereview.chromium.org/1933006/diff/3001/4#newcode22
chrome/browser/printing/cloud_print/printer_info_linux.cc:22: bool
ReadFileToString(const char* filename, std::string* content) {
Done.
On 2010/05/04 23:25:21, sanjeevr wrote:
> Please use the ReadFileToString method in base/file_util.h.

http://codereview.chromium.org/1933006/diff/3001/4#newcode60
chrome/browser/printing/cloud_print/printer_info_linux.cc:60: if
(printer_info.printer_name == "chanticleer")
Done.
On 2010/05/04 23:25:21, sanjeevr wrote:
> Please remove this

http://codereview.chromium.org/1933006/diff/3001/4#newcode70
chrome/browser/printing/cloud_print/printer_info_linux.cc:70: const char*
ppd_filename = cupsGetPPD(printer_name.c_str());
From CUPS: "The returned filename is stored in a static buffer and is
overwritten with each call to cupsGetPPD or cupsGetPPD2."
I'll a comment.

On 2010/05/04 23:25:21, sanjeevr wrote:
> Who allocates this?

http://codereview.chromium.org/1933006/diff/3001/4#newcode75
chrome/browser/printing/cloud_print/printer_info_linux.cc:75:
remove(ppd_filename);
Done.
On 2010/05/04 23:25:21, sanjeevr wrote:
> Please use a base utility for this.

http://codereview.chromium.org/1933006/diff/3001/4#newcode80
chrome/browser/printing/cloud_print/printer_info_linux.cc:80:
printer_info->printer_defaults.clear();
Done.
On 2010/05/04 23:25:21, sanjeevr wrote:
> Will we ever support defaults, if yes, please add a TODO. If not, please
mention
> that here in a comment.

http://codereview.chromium.org/1933006/diff/3001/4#newcode130
chrome/browser/printing/cloud_print/printer_info_linux.cc:130:
std::map<std::string, std::string> options;
Comment added.
On 2010/05/04 23:25:21, sanjeevr wrote:
> You don't need a map. DictionaryValue provides a key_iterator.

http://codereview.chromium.org/1933006/diff/3001/4#newcode198
chrome/browser/printing/cloud_print/printer_info_linux.cc:198:
EnumeratePrinters(&printer_list);
Unfortunately, more efficient way is available from CUPS 1.4, which we probably
should not enforce. I've added a comment.

On 2010/05/04 23:25:21, sanjeevr wrote:
> This seems a very inefficient way to get info for a specific printer.

http://codereview.chromium.org/1933006/diff/3001/4#newcode240
chrome/browser/printing/cloud_print/printer_info_linux.cc:240:
delegate_->OnJobChanged();
Done.
On 2010/05/04 23:25:21, sanjeevr wrote:
> Please comment here about this hack in detail.

http://codereview.chromium.org/1933006/diff/3001/4#newcode251
chrome/browser/printing/cloud_print/printer_info_linux.cc:251:
ScopedRunnableMethodFactory<PrinterChangeNotifier::NotificationState>
I've made NotificationState class ref counted to use NewRunnableMethod
On 2010/05/04 23:25:21, sanjeevr wrote:
> You don't need a method factory. You can just use NewRunnableMethod. See
> examples in cloud_print_proxy_backend.cc.

[sanjeevr, 2010-05-10 23:33:24]

Some nits and one important comment.

http://codereview.chromium.org/1933006/diff/3001/4
File chrome/browser/printing/cloud_print/printer_info_linux.cc (right):

http://codereview.chromium.org/1933006/diff/3001/4#newcode70
chrome/browser/printing/cloud_print/printer_info_linux.cc:70: const char*
ppd_filename = cupsGetPPD(printer_name.c_str());
On 2010/05/07 00:16:22, gene wrote:
> From CUPS: "The returned filename is stored in a static buffer and is
> overwritten with each call to cupsGetPPD or cupsGetPPD2."
> I'll a comment.
> 
> On 2010/05/04 23:25:21, sanjeevr wrote:
> > Who allocates this?
> 
> 

In that case, is this thread-safe? If not, we might need a lock.

http://codereview.chromium.org/1933006/diff/6001/7001#newcode77
chrome/browser/printing/cloud_print/printer_info_linux.cc:77: if (res) {
Please add a tode here to change this. Rand is not a great way to generate a
unique value.

http://codereview.chromium.org/1933006/diff/6001/7001#newcode219
chrome/browser/printing/cloud_print/printer_info_linux.cc:219:
~NotificationState() {
Please define the 5000 as a const within the class.

http://codereview.chromium.org/1933006/diff/6001/7001#newcode228
chrome/browser/printing/cloud_print/printer_info_linux.cc:228: delegate_ =
delegate;
Nit: Orphant => Orphan and stoped => stopped.

[gene, 2010-05-10 23:57:38]

Added lock to protect CUPS static buffer.

http://codereview.chromium.org/1933006/diff/6001/7001
File chrome/browser/printing/cloud_print/printer_info_linux.cc (right):

http://codereview.chromium.org/1933006/diff/6001/7001#newcode77
chrome/browser/printing/cloud_print/printer_info_linux.cc:77: id +=
Uint64ToString(base::RandUint64());
On 2010/05/10 23:33:24, sanjeevr wrote:
> Please add a tode here to change this. Rand is not a great way to generate a
> unique value.

Done.

http://codereview.chromium.org/1933006/diff/6001/7001#newcode219
chrome/browser/printing/cloud_print/printer_info_linux.cc:219:
&PrinterChangeNotifier::NotificationState::Update), 5000);
On 2010/05/10 23:33:24, sanjeevr wrote:
> Please define the 5000 as a const within the class.

Done.

http://codereview.chromium.org/1933006/diff/6001/7001#newcode228
chrome/browser/printing/cloud_print/printer_info_linux.cc:228: return;  //
Orphant call. We have been stoped already.
On 2010/05/10 23:33:24, sanjeevr wrote:
> Nit: Orphant => Orphan and stoped => stopped.

Done.

[gene, 2010-05-11 06:45:20]

Added printer status and description. Please take a look.

On 2010/05/10 23:57:38, gene wrote:
> Added lock to protect CUPS static buffer.
> 
> http://codereview.chromium.org/1933006/diff/6001/7001
> File chrome/browser/printing/cloud_print/printer_info_linux.cc (right):
> 
> http://codereview.chromium.org/1933006/diff/6001/7001#newcode77
> chrome/browser/printing/cloud_print/printer_info_linux.cc:77: id +=
> Uint64ToString(base::RandUint64());
> On 2010/05/10 23:33:24, sanjeevr wrote:
> > Please add a tode here to change this. Rand is not a great way to generate a
> > unique value.
> 
> Done.
> 
> http://codereview.chromium.org/1933006/diff/6001/7001#newcode219
> chrome/browser/printing/cloud_print/printer_info_linux.cc:219:
> &PrinterChangeNotifier::NotificationState::Update), 5000);
> On 2010/05/10 23:33:24, sanjeevr wrote:
> > Please define the 5000 as a const within the class.
> 
> Done.
> 
> http://codereview.chromium.org/1933006/diff/6001/7001#newcode228
> chrome/browser/printing/cloud_print/printer_info_linux.cc:228: return;  //
> Orphant call. We have been stoped already.
> On 2010/05/10 23:33:24, sanjeevr wrote:
> > Nit: Orphant => Orphan and stoped => stopped.
> 
> Done.

[sanjeevr, 2010-05-21 20:57:13]

LGTM for the new changes.