Linux Virtual Printer Driver

Code for the Linux virtual printer driver. A backend for CUPS that spools the data to a file and then passes that to Chrome.


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

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

[Albert Bodenhamer, 2011-07-08 20:40:13]

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
File cloud_print/virtual_driver/linux/backend.gyp (right):

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/backend.gyp:10: 'variables': {
Remove the section if it isn't needed.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/backend.gyp:29: 'virtual_driver_posix.cc',
Fix indents here

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
File cloud_print/virtual_driver/linux/printer_driver_util_linux.cc (right):

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_linux.cc:1: #include
<errno.h>
You need to start the file with a correct header.  See other files for an
example.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_linux.cc:7: #include
<string.h>
This looks like a lot of headers.  Are they all really needed?

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_linux.cc:24: namespace
switches {
Extract these to a common location in the print driver code.  You don't need to
fix the OTHER duplication with chrome_switches.cc but I don't want to introduce
a THIRD copy of these.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_linux.cc:47: using
namespace std;
It looks like you're referring to std::string below everywhere.  You shouldn't
need this using directive.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_linux.cc:49: void
launchChrome(std::string output_path, std::string job_title,
Should be LaunchChrome or, even better, LaunchPrintDialog

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
File cloud_print/virtual_driver/linux/printer_driver_util_posix.h (right):

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_posix.h:1: #ifndef
CLOUD_PRINT_VIRTUAL_DRIVER_LINUX_PRINTER_DRIVER_UTIL_POSIX_H_
Add standard header.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_posix.h:4: #include
<string.h>
did you mean:
#include <string>

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/printer_driver_util_posix.h:6: void
launchChrome(std::string output_path, std::string job_title,
There is a similar function in Windows.  Does it make sense to create a single
virtual_driver_util.h file used by all 3 platforms?  You could also make that a
good common place for the switches in that case.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
File cloud_print/virtual_driver/linux/virtual_driver_posix.cc (right):

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.cc:1: #include <errno.h>
Add header

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.cc:27: int
writetotemp(FILE* input_pdf, FilePath output_path) {
File name should be WriteToTemp

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.cc:47: void setuser(const
char* user) {
Filename should be SetUser

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
File cloud_print/virtual_driver/linux/virtual_driver_posix.h (right):

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.h:1: #ifndef
CLOUD_PRINT_VIRTUAL_DRIVER_LINUX_VIRTUAL_DRIVER_POSIX_H_
Add standard header.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.h:1: #ifndef
CLOUD_PRINT_VIRTUAL_DRIVER_LINUX_VIRTUAL_DRIVER_POSIX_H_
This file has "posix" in the name but you put it in a linux specific location.

http://codereview.chromium.org/7222011/diff/2001/cloud_print/virtual_driver/l...
cloud_print/virtual_driver/linux/virtual_driver_posix.h:6: int writetotemp(FILE*
input_pdf, FilePath output_path);
These functions are only called locally.  Do you need to have a header for them
at all?

[Albert Bodenhamer, 2011-07-11 19:21:32]

You're almost there.

I'm not a full committer yet, so you'll also need an LGTM from someone who is. 
Sanjeev is out so you should add Scott to this.

http://codereview.chromium.org/7222011/diff/9002/cloud_print/virtual_driver/p...
File cloud_print/virtual_driver/posix/virtual_driver_posix.cc (right):

http://codereview.chromium.org/7222011/diff/9002/cloud_print/virtual_driver/p...
cloud_print/virtual_driver/posix/virtual_driver_posix.cc:69: if (argc==1) {
You're accessing argc and argv directly in some places and using CommandLine in
others.  Can you use CommandLine for everything?  If not, you should comment
why.

http://codereview.chromium.org/7222011/diff/9002/cloud_print/virtual_driver/p...
cloud_print/virtual_driver/posix/virtual_driver_posix.cc:136:
LOG(ERROR)<<"Unable to change file permissions";
Should you return an error here?

[Scott Byer, 2011-07-11 22:11:57]

LGTM w/ nit.

http://codereview.chromium.org/7222011/diff/15001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/virtual_driver_switches.h (right):

http://codereview.chromium.org/7222011/diff/15001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/virtual_driver_switches.h:12: //
TODO(abodenha@chromium.org) Reunify them in some sensible manner.
Let's file a bug on this and add the http://crbug.com/# link here, or we'll
forget about it.

[Albert Bodenhamer, 2011-07-11 22:13:34]

LGTM

On Mon, Jul 11, 2011 at 3:11 PM, <scottbyer@chromium.org> wrote:

> LGTM w/ nit.
>
>
> http://codereview.chromium.**org/7222011/diff/15001/cloud_**
>
print/virtual_driver/virtual_**driver_switches.h<http://codereview.chromium.org/7222011/diff/15001/cloud_print/virtual_driver/virtual_driver_switches.h>
> File cloud_print/virtual_driver/**virtual_driver_switches.h (right):
>
> http://codereview.chromium.**org/7222011/diff/15001/cloud_**
>
print/virtual_driver/virtual_**driver_switches.h#newcode12<http://codereview.chromium.org/7222011/diff/15001/cloud_print/virtual_driver/virtual_driver_switches.h#newcode12>
> cloud_print/virtual_driver/**virtual_driver_switches.h:12: //
> TODO(abodenha@chromium.org) Reunify them in some sensible manner.
> Let's file a bug on this and add the http://crbug.com/# link here, or
> we'll forget about it.
>
>
>
http://codereview.chromium.**org/7222011/<http://codereview.chromium.org/7222...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[abeera, 2011-07-11 23:10:09]

Done. Bug filed as www.crbug.com/88991

[commit-bot: I haz the power, 2011-07-13 17:38:56]

Change committed as 92378

[Evan Martin, 2011-07-13 18:19:05]

Reviewers: in the future, can you also help new contributors write a useful
commit message?

[Albert Bodenhamer, 2011-07-13 18:20:53]

I missed that.  Sorry.  I'll pay closer attention in the future.

On Wed, Jul 13, 2011 at 11:19 AM, <evan@chromium.org> wrote:

> Reviewers: in the future, can you also help new contributors write a useful
> commit message?
>
>
>
http://codereview.chromium.**org/7222011/<http://codereview.chromium.org/7222...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Evan Martin, 2011-07-13 18:23:23]

No prob, just a friendly reminder.  :)

On Wed, Jul 13, 2011 at 11:20 AM, Albert Bodenhamer
<abodenha@chromium.org> wrote:
> I missed that.  Sorry.  I'll pay closer attention in the future.
>
> On Wed, Jul 13, 2011 at 11:19 AM, <evan@chromium.org> wrote:
>>
>> Reviewers: in the future, can you also help new contributors write a
>> useful
>> commit message?
>>
>> http://codereview.chromium.org/7222011/
>
>
>
> --
> Albert Bodenhamer | Software Engineer | abodenha@chromium.org
>

[rvargas (doing something else), 2011-07-19 23:17:33]

http://codereview.chromium.org/7222011/diff/20001/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/backend.gyp (right):

http://codereview.chromium.org/7222011/diff/20001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:18: 'msvs_guid':
'8D06D53B-289E-4f99-99FC-77C77DB478A8',
Don't need this.

http://codereview.chromium.org/7222011/diff/20001/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:28: # Local Variables:
We don't use this anymore.

[Scott Byer, 2011-07-20 19:36:30]

LGTM. Nice, cleaner.

On 2011/07/20 18:33:54, abeera wrote:

[commit-bot: I haz the power, 2011-07-20 20:29:01]

Try job failure for 7222011-33001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[Scott Byer, 2011-07-20 21:33:04]

LGTM on the latest patch.

On 2011/07/20 20:29:01, I haz the power (commit-bot) wrote:
> Try job failure for 7222011-33001 (retry) on linux for step "compile" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[Scott Byer, 2011-07-20 21:33:05]

LGTM on the latest patch.

On 2011/07/20 20:29:01, I haz the power (commit-bot) wrote:
> Try job failure for 7222011-33001 (retry) on linux for step "compile" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-07-21 00:21:31]

Change committed as 93298