Initial CL for a simple port monitor for Windows to be used by the Cloud Print Virtual Print driver.

Initial CL for a simple port monitor for Windows to be used by the Cloud Print Virtual Print driver.
TEST=Everything should build and unit tests should pass.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80678
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80716

[sanjeevr, 2011-04-04 17:19:17]

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.cc (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:30: L"chrome.exe
-cloud-print-file=\"%s\" "
You should probably use chrome_switches.cc and .h. Also base has a CommandLine
class that you can use.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:48: typedef struct {
Please don't use C-style "typedef struct" (here and everywhere below)

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:64: MONITORUI
MonitorUI = {
g_monitor_ui ?

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:71: MONITOR2
Monitor2 = {
g_monitor2

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:93: BOOL
GetChromeExePath(std::wstring* chrome_path) {
Use a C++ bool (here and for all other helper functions. You might want to place
your helper functions in an anonymous namespace.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:116:
PathService::Get(base::DIR_PROGRAM_FILES, &path);
This may not be the right thing to do from a 64-bit binary. DIR_PROGRAM_FILES
uses CSIDL_PROGRAM_FILES but the right thing to use here is
CSIDL_PROGRAM_FILESX86.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:128: size_t
title_chars) {
Change this method to either return a string16 or take an out argument that is
string16*

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:136: buffer = new
BYTE[bytes_needed];
buffer is leaked below. Use scoped_ptr

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:151: void
HandlePortUi(HWND hwnd, const wchar_t* caption) {
Chrome style is to use const std::string& (or const string16&) for all input
strings.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:180: &token)) {
Chrome style is to have multiple return points and not use nested ifs (I
personally prefer nested ifs myself). Also use smart handle classes wherever
possible.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:200: job_title);
Use base::CommandLine

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:361: port_data->file
= file_util::OpenFile(*(port_data->file_path), "wb+");
Dealing in FILE* rather that file handles makes me nervous.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:392: *read_bytes =
0;
Is there a well-known SetLastError that we should be doing here?

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:444:
ClosePrinter(port_data->printer);
Nit: This should be inside the if block.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:445:
port_data->printer = NULL;
What about setting closing port_data->file and setting it to NULL?

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:459: delete
port_data->file_path;
We probably need to clear other fields too?

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.h (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.h:16: BOOL
GetChromeExePath(std::wstring* chrome_path);
Helpers like these do not need to be in the header file. They can just live in
an anonymous namespace in the cc file.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/spooler_win.h (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/spooler_win.h:13: #ifdef USE_WIN_DDK
Instead of this, you should probably check for the include guard used in the
actual DDK header file (and expect the actual DDK header to be included
elsewhere).

[Albert Bodenhamer, 2011-04-04 23:12:24]

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.cc (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:30: L"chrome.exe
-cloud-print-file=\"%s\" "
On 2011/04/04 17:19:17, sanjeevr wrote:
> You should probably use chrome_switches.cc and .h. Also base has a CommandLine
> class that you can use.

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:48: typedef struct {
On 2011/04/04 17:19:17, sanjeevr wrote:
> Please don't use C-style "typedef struct" (here and everywhere below)

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:64: MONITORUI
MonitorUI = {
On 2011/04/04 17:19:17, sanjeevr wrote:
> g_monitor_ui ?

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:71: MONITOR2
Monitor2 = {
On 2011/04/04 17:19:17, sanjeevr wrote:
> g_monitor2

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:93: BOOL
GetChromeExePath(std::wstring* chrome_path) {
On 2011/04/04 17:19:17, sanjeevr wrote:
> Use a C++ bool (here and for all other helper functions. You might want to
place
> your helper functions in an anonymous namespace.

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:116:
PathService::Get(base::DIR_PROGRAM_FILES, &path);
On 2011/04/04 17:19:17, sanjeevr wrote:
> This may not be the right thing to do from a 64-bit binary. DIR_PROGRAM_FILES
> uses CSIDL_PROGRAM_FILES but the right thing to use here is
> CSIDL_PROGRAM_FILESX86.

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:128: size_t
title_chars) {
On 2011/04/04 17:19:17, sanjeevr wrote:
> Change this method to either return a string16 or take an out argument that is
> string16*

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:136: buffer = new
BYTE[bytes_needed];
On 2011/04/04 17:19:17, sanjeevr wrote:
> buffer is leaked below. Use scoped_ptr

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:151: void
HandlePortUi(HWND hwnd, const wchar_t* caption) {
On 2011/04/04 17:19:17, sanjeevr wrote:
> Chrome style is to use const std::string& (or const string16&) for all input
> strings.

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:180: &token)) {
On 2011/04/04 17:19:17, sanjeevr wrote:
> Chrome style is to have multiple return points and not use nested ifs (I
> personally prefer nested ifs myself). Also use smart handle classes wherever
> possible.

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:200: job_title);
On 2011/04/04 17:19:17, sanjeevr wrote:
> Use base::CommandLine

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:361: port_data->file
= file_util::OpenFile(*(port_data->file_path), "wb+");
On 2011/04/04 17:19:17, sanjeevr wrote:
> Dealing in FILE* rather that file handles makes me nervous.

As discussed, we'll leave this as-is for now.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:392: *read_bytes =
0;
On 2011/04/04 17:19:17, sanjeevr wrote:
> Is there a well-known SetLastError that we should be doing here?
A few make sense.  I'm going with ERROR_NOT_SUPPORTED.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:444:
ClosePrinter(port_data->printer);
On 2011/04/04 17:19:17, sanjeevr wrote:
> Nit: This should be inside the if block.

I pulled the PortData cleanup code into a helper function.  It now makes more
sense outside the if block.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:445:
port_data->printer = NULL;
On 2011/04/04 17:19:17, sanjeevr wrote:
> What about setting closing port_data->file and setting it to NULL?

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:459: delete
port_data->file_path;
On 2011/04/04 17:19:17, sanjeevr wrote:
> We probably need to clear other fields too?

Done.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.h (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/port_monitor.h:16: BOOL
GetChromeExePath(std::wstring* chrome_path);
On 2011/04/04 17:19:17, sanjeevr wrote:
> Helpers like these do not need to be in the header file. They can just live in
> an anonymous namespace in the cc file.

GetChromeExePath is referenced from the unit tests.  I've removed the others.

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
File cloud_print/virtual_driver/win/port_monitor/spooler_win.h (right):

http://codereview.chromium.org/6778001/diff/8001/cloud_print/virtual_driver/w...
cloud_print/virtual_driver/win/port_monitor/spooler_win.h:13: #ifdef USE_WIN_DDK
On 2011/04/04 17:19:17, sanjeevr wrote:
> Instead of this, you should probably check for the include guard used in the
> actual DDK header file (and expect the actual DDK header to be included
> elsewhere).

Done.

[sanjeevr, 2011-04-05 21:08:05]

LGTM

http://codereview.chromium.org/6778001/diff/12011/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/win/port_monitor/port_monitor.cc (right):

http://codereview.chromium.org/6778001/diff/12011/cloud_print/virtual_driver/...
cloud_print/virtual_driver/win/port_monitor/port_monitor.cc:246:
SHGetFolderPath(NULL,
Please add a TODO to use SHGetKnownFolderPath (Vista and above)

[Albert Bodenhamer, 2011-04-06 22:31:26]

Changes to fix build break.

[sanjeevr, 2011-04-06 22:33:04]

LGTM