Virtual Cloud Print Driver for Mac.

Virtual Cloud Print Driver for Mac.
Includes code for the driver itself. Also modifies the browser process as well as service process to register Apple Event handlers. Also changes the service process to allow registration of driver.

BUG=
TEST=

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

[Nico, 2011-07-21 22:22:50]

Please review your CL for coding style yourself before sending it for review by
others. I found 10 style issues in 20 seconds of looking at this patch.

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:187: // Apple Event handler that recieves
print event from service
receives

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:190: //Pull parameter list out of Apple
Event.
space after //

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:192: [event
paramDescriptorForKeyword:cloud_print_class];
indent 4

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:194: if (! (param_list == NULL)) {
!=

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:196: NSString * mime = [[param_list
descriptorAtIndex:1] stringValue];
no space in front of *

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:197: NSString * input_path = [[param_list
descriptorAtIndex:2] stringValue];
objc code uses camelCase

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:199: //Conver the title to UTF 16 as
required.
space after //
Convert

http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:201: print_dialog_cloud
::CreatePrintDialogForFile(
no space in front of ::

http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
File content/common/cloud_print_class_mac.mm (right):

http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
content/common/cloud_print_class_mac.mm:7: const AEEventClass cloud_print_class
= 'GCPp';
Do you want different codes depending on chrome and chromium?

[abeera, 2011-07-22 16:34:55]

I've tried fixing the issues you highlighted and other potential style issues in
my code. I apologize for the quality, its the first time I've written any
Objective C.
On 2011/07/21 22:22:50, Nico wrote:
> Please review your CL for coding style yourself before sending it for review
by
> others. I found 10 style issues in 20 seconds of looking at this patch.
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> File chrome/browser/app_controller_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:187: // Apple Event handler that recieves
> print event from service
> receives
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:190: //Pull parameter list out of Apple
> Event.
> space after //
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:192: [event
> paramDescriptorForKeyword:cloud_print_class];
> indent 4
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:194: if (! (param_list == NULL)) {
> !=
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:196: NSString * mime = [[param_list
> descriptorAtIndex:1] stringValue];
> no space in front of *
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:197: NSString * input_path = [[param_list
> descriptorAtIndex:2] stringValue];
> objc code uses camelCase
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:199: //Conver the title to UTF 16 as
> required.
> space after //
> Convert
> 
>
http://codereview.chromium.org/7485011/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:201: print_dialog_cloud
> ::CreatePrintDialogForFile(
> no space in front of ::
> 
>
http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
> File content/common/cloud_print_class_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
> content/common/cloud_print_class_mac.mm:7: const AEEventClass
cloud_print_class
> = 'GCPp';
> Do you want different codes depending on chrome and chromium?

[abeera, 2011-07-22 16:35:06]

http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
File content/common/cloud_print_class_mac.mm (right):

http://codereview.chromium.org/7485011/diff/1/content/common/cloud_print_clas...
content/common/cloud_print_class_mac.mm:7: const AEEventClass cloud_print_class
= 'GCPp';
Different codes are not needed since the Apple Event is sent as per
the bundle ID. This is handled in the submitPrint function in
CrApplication by base::mac::BaseBundleID(), which will return the appropriate
bundle ID depending on the build.
On 2011/07/21 22:22:50, Nico wrote:
> Do you want different codes depending on chrome and chromium?

[Nico, 2011-07-22 16:53:55]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml

Are there any tests for this code?

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:188: // process, gets the required data and
launches Print dialog
trailing .

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:192: [event
paramDescriptorForKeyword:cloudPrintClass];
Indent 2 more

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:200: string16 title16 =
UTF8ToUTF16([printTitle UTF8String]);
Use SysNSStringToUTF16() from base/sys_string_conversions.h instead

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc
(right):

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:10: void
VirtualDriverInstallHelper::SetUpInstall() {
No indent inside of namespaces

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:39: }
}  // namespace cloud_print

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:15: :public
base::RefCountedThreadSafe<VirtualDriverInstallHelper>
: goes on previous line

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:18: static
void SetUpInstall();
indent 1 more, also for the rest of the class

http://codereview.chromium.org/7485011/diff/7001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7485011/diff/7001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:470: extern const char
kVirtualPrinterDriverEnabled[];
Put this below kUserPolicyRefreshRate to match the location in the cc file

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
File content/common/chrome_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.h:71: void SetHandler();
This needs a better name. (at least SetCloudPrintHandler.) I dont' think this
belongs in content/ either -- since this is only used by the service process, it
should live in some file there.

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
File content/common/chrome_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.mm:53: CFStringRef silentLaunchFlag =
CFStringCreateWithCString(
break after =

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.mm:55: CFStringRef flags[] =
{silentLaunchFlag};
spaces inside {}

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.mm:68: NULL, &ref, kDontWantURL);
wrong indent

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.mm:71: LOG(ERROR) <<"Failed to make path
ref";
space after <<

[abeera, 2011-07-22 20:59:03]

Thank you for your feedback. I did not write any tests since I'm mostly taking
advantage of pre-existing infrastructure (the AppleEvent mechanism in launch
services, the service IPC setup) and did not really know how to test them.

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/7001/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:15: :public
base::RefCountedThreadSafe<VirtualDriverInstallHelper>
Changed, but cpplint.py gives this presubmit warning: Labels should always be
indented at least one space.  If this is a member-initializer list in a
constructor or the base class list in a class definition, the colon should be on
the following line.  [whitespace/labels] [4]

What is the appropriate place?
On 2011/07/22 16:53:55, Nico wrote:
> : goes on previous line

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
File content/common/chrome_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.h:71: void SetHandler();
Name changed. This is essentially a wrapper for an ObjC function that can be
called from C++, and modifies CrApplication itself, which is why I put it in
here. 
On 2011/07/22 16:53:55, Nico wrote:
> This needs a better name. (at least SetCloudPrintHandler.) I dont' think this
> belongs in content/ either -- since this is only used by the service process,
it
> should live in some file there.

[Nico, 2011-07-22 21:02:53]

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
File content/common/chrome_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
content/common/chrome_application_mac.h:71: void SetHandler();
On 2011/07/22 20:59:03, abeera wrote:
> Name changed. This is essentially a wrapper for an ObjC function that can be
> called from C++, and modifies CrApplication itself, which is why I put it in
> here. 

The browser process has this file:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome...
maybe there should be a chrome_helper_application for the helper process?

http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
VirtualDriverInstallHelper:
space before :. that probably fixes pylint?

[abeera, 2011-07-22 22:58:32]

Thank you for the suggestion!  Creating a new ServiceCrApplication was a really
good way of resolving that, and the code is a lot cleaner now.
On 2011/07/22 21:02:53, Nico wrote:
>
http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
> File content/common/chrome_application_mac.h (right):
> 
>
http://codereview.chromium.org/7485011/diff/7001/content/common/chrome_applic...
> content/common/chrome_application_mac.h:71: void SetHandler();
> On 2011/07/22 20:59:03, abeera wrote:
> > Name changed. This is essentially a wrapper for an ObjC function that can be
> > called from C++, and modifies CrApplication itself, which is why I put it in
> > here. 
> 
> The browser process has this file:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome...
> maybe there should be a chrome_helper_application for the helper process?
> 
>
http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
> File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
> (right):
> 
>
http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
> chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
> VirtualDriverInstallHelper:
> space before :. that probably fixes pylint?

[abeera, 2011-07-22 22:58:47]

http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
VirtualDriverInstallHelper:
Pylint still complained.
On 2011/07/22 21:02:54, Nico wrote:
> space before :. that probably fixes pylint?

[abeera, 2011-07-29 21:09:55]

Please take a look when you have a chance. Thanks!
On 2011/07/22 22:58:47, abeera wrote:
>
http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
> File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
> (right):
> 
>
http://codereview.chromium.org/7485011/diff/12001/chrome/browser/printing/clo...
> chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
> VirtualDriverInstallHelper:
> Pylint still complained.
> On 2011/07/22 21:02:54, Nico wrote:
> > space before :. that probably fixes pylint?

[Nico, 2011-08-01 17:13:39]

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:174: -
(void)installCloudPrint:(NSAppleEventDescriptor*)event;
uninstallCloudPrint: is missing

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:189: -
(void)submitPrint:(NSAppleEventDescriptor*)event {
This is used only for cloud print, right? Should have "cloud" in the name
somewhere.

Also, no methods go above awakeFromNib – move this below to after
getUrl:withReply: to match the order in the interface block. Same for next
method.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:202: FilePath([inputPath
UTF8String]),title16, [mime UTF8String], false);
space after ,

Also, make it somehow clear what the "false" means here. Your options:

1. Change the function to not take a bool but an enum with a descriptive name,
then the calling site will look like ", CLOUD_PRINT_DONT_KILL_KITTENS)" and it's
clear what this means

2. "const bool killKittens = false; ..., killKittens);"

3. "..., /*killKittens=*/false)"

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc
(right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:7: #include
"chrome/common/service_messages.h"
all indentation in this file is wrong

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:13:
namespace cloud_print {
add newline

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
VirtualDriverInstallHelper :
Needs comments, the public methods as well

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:16: {
brace goes on previous line

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:25: };
indentation wrong. newline after this

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:27: 
  // namespace cloud_print

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:8: 
remove this newline, sort the 3 include/import statements

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:27: // Event handler for Cloud
Print Event. Forwards print job recieved to Chrome,
received

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:31: const char silent[] =
"--no-startup-window";
i think there's a global constant for this already?

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:56: NSAppleEventDescriptor*
sendEvent = [NSAppleEventDescriptor
Break before [, align colons

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:75: params.initialEvent =
(AppleEvent *)  [sendEvent aeDesc];
no space before *

[abeera, 2011-08-01 20:45:10]

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:174: -
(void)installCloudPrint:(NSAppleEventDescriptor*)event;
On 2011/08/01 17:13:39, Nico wrote:
> uninstallCloudPrint: is missing

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:189: -
(void)submitPrint:(NSAppleEventDescriptor*)event {
On 2011/08/01 17:13:39, Nico wrote:
> This is used only for cloud print, right? Should have "cloud" in the name
> somewhere.
> 
> Also, no methods go above awakeFromNib – move this below to after
> getUrl:withReply: to match the order in the interface block. Same for next
> method.

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/app_controlle...
chrome/browser/app_controller_mac.mm:202: FilePath([inputPath
UTF8String]),title16, [mime UTF8String], false);
On 2011/08/01 17:13:39, Nico wrote:
> space after ,
> 
> Also, make it somehow clear what the "false" means here. Your options:
> 
> 1. Change the function to not take a bool but an enum with a descriptive name,
> then the calling site will look like ", CLOUD_PRINT_DONT_KILL_KITTENS)" and
it's
> clear what this means
> 
> 2. "const bool killKittens = false; ..., killKittens);"
> 
> 3. "..., /*killKittens=*/false)"

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc
(right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:7: #include
"chrome/common/service_messages.h"
On 2011/08/01 17:13:39, Nico wrote:
> all indentation in this file is wrong

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:13:
namespace cloud_print {
On 2011/08/01 17:13:39, Nico wrote:
> add newline

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:14: class
VirtualDriverInstallHelper :
On 2011/08/01 17:13:39, Nico wrote:
> Needs comments, the public methods as well

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:16: {
On 2011/08/01 17:13:39, Nico wrote:
> brace goes on previous line

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:25: };
On 2011/08/01 17:13:39, Nico wrote:
> indentation wrong. newline after this

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:27: 
On 2011/08/01 17:13:39, Nico wrote:
>   // namespace cloud_print

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:8: 
On 2011/08/01 17:13:39, Nico wrote:
> remove this newline, sort the 3 include/import statements

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:27: // Event handler for Cloud
Print Event. Forwards print job recieved to Chrome,
On 2011/08/01 17:13:39, Nico wrote:
> received

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:31: const char silent[] =
"--no-startup-window";
Done. Earlier, this lived in content/common, which did not have access to
chrome/common.
On 2011/08/01 17:13:39, Nico wrote:
> i think there's a global constant for this already?

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:56: NSAppleEventDescriptor*
sendEvent = [NSAppleEventDescriptor
On 2011/08/01 17:13:39, Nico wrote:
> Break before [, align colons

Done.

http://codereview.chromium.org/7485011/diff/7010/chrome/service/chrome_servic...
chrome/service/chrome_service_application_mac.mm:75: params.initialEvent =
(AppleEvent *)  [sendEvent aeDesc];
On 2011/08/01 17:13:39, Nico wrote:
> no space before *

Done.

[Nico, 2011-08-01 21:31:12]

getting there, style-wise

Someone from cloud_print needs to have a look too. Since abodenha isn't
responding, I'm cc'ing scottbyer

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:201: andEventID:cloudPrintClass]; // Event
handler for cloud print.
Remove comment

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1093: [event
paramDescriptorForKeyword:cloudPrintClass];
indent 4

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc
(right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:5: #include
"chrome/browser/printing/cloud_print/virtual_driver_install_helper.h"
One newline here. The standard format is

#include "header_file_for_this_cc.cc"

one include block for all the other includes

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:12:
scoped_refptr<VirtualDriverInstallHelper> help =
NewRunnableMethod() retains its argument, so no need for a scoped_refptr here I
think.

Can't you do

  ServiceProcessControl* service = ServiceProcessControl::GetInstance();
  service->Launch()(NewRunnableMethod(service, ServiceProcessControl::Send, new
ServiceMsg_EnableVirtualDriver()));

? Then you don't need the temporary object and the method. (if all callers of
Launch() just end up calling methods on SPC anyway, Launch() should get a set of
flags, not a callable)

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:15: //
Helper class to register the Cloud Print Driver with the service process
trailing .

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:17: public
base::RefCountedThreadSafe<VirtualDriverInstallHelper> {
indent 2 more

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:29: 
no newline here

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:30: std::string silent = "--";
"--" + switches::kNoStartupWindow

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:32: // Set up flag so that it
can be passed along with Apple Event.
with the Apple Event

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:57: [NSAppleEventDescriptor
nit: i'd get rid of this linebreak

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:95: 
no newline

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:160: 
no newline

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_proc...
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_proc...
chrome/service/service_process.cc:201: EnableVirtualPrintDriver();
Doesn't this just write "true" back to the prefs if you read "true" from the
prefs? Why is this needed? Looks like a no-op.

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/installer_util_mac.h (right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.h:12: void sendEvent(const
AEEventClass sendClass);
Needs a namespace, a less generic name, a comment, and probably a & after
AEEventClass

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/mac_backend.gyp (right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/mac_backend.gyp:1: # Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Why a new gyp file? Why not add to the existing backend.gyp?

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:20: // Duplicated is
content/common/cloud_print_class_mac.h
why not include that header then?

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/uninstall_cloud_print_driver_mac.mm
(right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/uninstall_cloud_print_driver_mac.mm:10: const
AEEventClass cloudPrintUninstallClass = 'GCPu';
duplicated as well

http://codereview.chromium.org/7485011/diff/16002/content/common/cloud_print_...
File content/common/cloud_print_class_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/content/common/cloud_print_...
content/common/cloud_print_class_mac.mm:7: const AEEventClass cloudPrintClass =
'GCPp';
duplicated

[abeera, 2011-08-02 01:21:02]

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:201: andEventID:cloudPrintClass]; // Event
handler for cloud print.
On 2011/08/01 21:31:12, Nico wrote:
> Remove comment

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1093: [event
paramDescriptorForKeyword:cloudPrintClass];
On 2011/08/01 21:31:12, Nico wrote:
> indent 4

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc
(right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:5: #include
"chrome/browser/printing/cloud_print/virtual_driver_install_helper.h"
On 2011/08/01 21:31:12, Nico wrote:
> One newline here. The standard format is
> 
> #include "header_file_for_this_cc.cc"
> 
> one include block for all the other includes

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.cc:12:
scoped_refptr<VirtualDriverInstallHelper> help =
To post a task, a class needs to extend from RefCountedThreadSafe, which
ServiceProcessControl does not. So, I felt that using this forwarding class was
the best way to do things.
I tried to change it to not use scoped_refptr, however the browser crashed:  it
is needed.
On 2011/08/01 21:31:12, Nico wrote:
> NewRunnableMethod() retains its argument, so no need for a scoped_refptr here
I
> think.
> 
> Can't you do
> 
>   ServiceProcessControl* service = ServiceProcessControl::GetInstance();
>   service->Launch()(NewRunnableMethod(service, ServiceProcessControl::Send,
new
> ServiceMsg_EnableVirtualDriver()));
> 
> ? Then you don't need the temporary object and the method. (if all callers of
> Launch() just end up calling methods on SPC anyway, Launch() should get a set
of
> flags, not a callable)

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/virtual_driver_install_helper.h
(right):

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:15: //
Helper class to register the Cloud Print Driver with the service process
On 2011/08/01 21:31:12, Nico wrote:
> trailing .

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:17: public
base::RefCountedThreadSafe<VirtualDriverInstallHelper> {
On 2011/08/01 21:31:12, Nico wrote:
> indent 2 more

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/virtual_driver_install_helper.h:29: 
On 2011/08/01 21:31:12, Nico wrote:
> no newline here

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:30: std::string silent = "--";
On 2011/08/01 21:31:12, Nico wrote:
> "--" + switches::kNoStartupWindow

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:32: // Set up flag so that it
can be passed along with Apple Event.
On 2011/08/01 21:31:12, Nico wrote:
> with the Apple Event

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:57: [NSAppleEventDescriptor
On 2011/08/01 21:31:12, Nico wrote:
> nit: i'd get rid of this linebreak

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:95: 
On 2011/08/01 21:31:12, Nico wrote:
> no newline

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:160: 
On 2011/08/01 21:31:12, Nico wrote:
> no newline

Done.

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_proc...
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/7485011/diff/16002/chrome/service/service_proc...
chrome/service/service_process.cc:201: EnableVirtualPrintDriver();
Changed to OnServiceEnabled(), which registers that a service is running.
On 2011/08/01 21:31:12, Nico wrote:
> Doesn't this just write "true" back to the prefs if you read "true" from the
> prefs? Why is this needed? Looks like a no-op.

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/installer_util_mac.h (right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.h:12: void sendEvent(const
AEEventClass sendClass);
On 2011/08/01 21:31:12, Nico wrote:
> Needs a namespace, a less generic name, a comment, and probably a & after
> AEEventClass

Done.

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/16002/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:20: // Duplicated is
content/common/cloud_print_class_mac.h
Currently, the driver does not depend on content/common, and I do not want to
introduce any additional dependencies. There is some duplication in the Windows
virtual driver as well, and and all these need to be resolved together, in a
future CL.
On 2011/08/01 21:31:12, Nico wrote:
> why not include that header then?

[Scott Byer, 2011-08-02 23:52:52]

Please fix the lint errors as well. Also, double-check that you haven't
introduced a permissions issue for virtual_driver_posix.gyp - make sure it
doesn't have the executable bit set before the next upload.

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:25: #include
"chrome/browser/printing/cloud_print/virtual_driver_install_helper.h"
alphabetize include order

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:163: 
nit: extra blank line

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1095: if (paramList != NULL) {
Check against nil for ObjC objects

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1416: const char kVirtualPrinterDriverEnabled[] =
"virtual_driver.enabled";
This goes up with the service process prefs group. Consider using
cloud_print.enable_virtual_driver

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h#...
chrome/common/pref_names.h:519: extern const char
kVirtualPrinterDriverEnabled[];
See comment in the .cc file - when moved there, it should be moved to the
corresponding spot in here.

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:52: LOG(ERROR) <<
GetMacOSStatusCommentString(status);
Is it reasonable to continue from here or should you just return?

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:73: params.initialEvent =
(AppleEvent*)  [sendEvent aeDesc];
prefer static_cast<>()

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:91: (ServiceCrApplication*)
[ServiceCrApplication sharedApplication];
prefer static_cast<>()

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

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:74: 'installer_util_mac.mm',
put in alphabetical order

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/installer_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:35: std::cerr << "Failed
to make path ref";
Add a comment explaining the iostream use.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:41:
NSAppleEventDescriptor* sendEvent =
Be consistent with error checking - check for nil.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:47:
LSApplicationParameters params = { 0, kLSLaunchDefaults , &ref, NULL, NULL,
nit: no space before comma

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:49: params.initialEvent =
(AppleEvent*) [sendEvent aeDesc];
prefer static_cast<>()

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:55:
NSAppleEventDescriptor* parameters = [NSAppleEventDescriptor listDescriptor];
The order of construction here is funny; I would not even start constructing
parameters until after the event was created.

Also, there's a nil check on event but not on parameters. Both affect whether
you send a valid message.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:72:
LSApplicationParameters params = {0, kLSLaunchDefaults , &ref, NULL, NULL,
...I would move this down to just above where the initial event is set

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:75: [event
setParamDescriptor:parameters forKeyword:cloudPrintClass];
...and I would move this up to as soon as parameters was constructed.

The idea is to increase the locality of what's being done. Avoid card shuffling
initialization sequences.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:76:
params.initialEvent =(AppleEvent*) [event aeDesc];
nit: space after =, prefer static_cast<>()

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:80: LOG(ERROR)
<<"Unable to launch";
nit: space after operator

[abeera, 2011-08-03 20:34:42]

Thanks for all the feedback. Lint still shows errors with service_main and
service_messages, but these are not actual errors.

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:25: #include
"chrome/browser/printing/cloud_print/virtual_driver_install_helper.h"
On 2011/08/02 23:52:52, Scott Byer wrote:
> alphabetize include order

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:163: 
On 2011/08/02 23:52:52, Scott Byer wrote:
> nit: extra blank line

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
chrome/browser/app_controller_mac.mm:1095: if (paramList != NULL) {
On 2011/08/02 23:52:52, Scott Byer wrote:
> Check against nil for ObjC objects

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1416: const char kVirtualPrinterDriverEnabled[] =
"virtual_driver.enabled";
On 2011/08/02 23:52:52, Scott Byer wrote:
> This goes up with the service process prefs group. Consider using
> cloud_print.enable_virtual_driver

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h#...
chrome/common/pref_names.h:519: extern const char
kVirtualPrinterDriverEnabled[];
On 2011/08/02 23:52:52, Scott Byer wrote:
> See comment in the .cc file - when moved there, it should be moved to the
> corresponding spot in here.

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:52: LOG(ERROR) <<
GetMacOSStatusCommentString(status);
On 2011/08/02 23:52:52, Scott Byer wrote:
> Is it reasonable to continue from here or should you just return?

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:73: params.initialEvent =
(AppleEvent*)  [sendEvent aeDesc];
On 2011/08/02 23:52:52, Scott Byer wrote:
> prefer static_cast<>()

Done.

http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:91: (ServiceCrApplication*)
[ServiceCrApplication sharedApplication];
On 2011/08/02 23:52:52, Scott Byer wrote:
> prefer static_cast<>()

Done.

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

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:74: 'installer_util_mac.mm',
On 2011/08/02 23:52:52, Scott Byer wrote:
> put in alphabetical order

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/installer_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:35: std::cerr << "Failed
to make path ref";
On 2011/08/02 23:52:52, Scott Byer wrote:
> Add a comment explaining the iostream use.

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:41:
NSAppleEventDescriptor* sendEvent =
On 2011/08/02 23:52:52, Scott Byer wrote:
> Be consistent with error checking - check for nil.

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:47:
LSApplicationParameters params = { 0, kLSLaunchDefaults , &ref, NULL, NULL,
On 2011/08/02 23:52:52, Scott Byer wrote:
> nit: no space before comma

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:49: params.initialEvent =
(AppleEvent*) [sendEvent aeDesc];
On 2011/08/02 23:52:52, Scott Byer wrote:
> prefer static_cast<>()

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:55:
NSAppleEventDescriptor* parameters = [NSAppleEventDescriptor listDescriptor];
On 2011/08/02 23:52:52, Scott Byer wrote:
> The order of construction here is funny; I would not even start constructing
> parameters until after the event was created.
> 
> Also, there's a nil check on event but not on parameters. Both affect whether
> you send a valid message.

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:72:
LSApplicationParameters params = {0, kLSLaunchDefaults , &ref, NULL, NULL,
On 2011/08/02 23:52:52, Scott Byer wrote:
> ...I would move this down to just above where the initial event is set

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:75: [event
setParamDescriptor:parameters forKeyword:cloudPrintClass];
On 2011/08/02 23:52:52, Scott Byer wrote:
> ...and I would move this up to as soon as parameters was constructed.
> 
> The idea is to increase the locality of what's being done. Avoid card
shuffling
> initialization sequences.

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:76:
params.initialEvent =(AppleEvent*) [event aeDesc];
On 2011/08/02 23:52:52, Scott Byer wrote:
> nit: space after =, prefer static_cast<>()

Done.

http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:80: LOG(ERROR)
<<"Unable to launch";
On 2011/08/02 23:52:52, Scott Byer wrote:
> nit: space after operator

Done.

[Scott Byer, 2011-08-04 20:43:47]

Ok, looking much better. Need a final review from Nico, the cloud print portions
look fine.

On 2011/08/03 20:34:42, abeera wrote:
> Thanks for all the feedback. Lint still shows errors with service_main and
> service_messages, but these are not actual errors.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
> File chrome/browser/app_controller_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
> chrome/browser/app_controller_mac.mm:25: #include
> "chrome/browser/printing/cloud_print/virtual_driver_install_helper.h"
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > alphabetize include order
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
> chrome/browser/app_controller_mac.mm:163: 
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > nit: extra blank line
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/browser/app_controll...
> chrome/browser/app_controller_mac.mm:1095: if (paramList != NULL) {
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > Check against nil for ObjC objects
> 
> Done.
> 
> http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc
> File chrome/common/pref_names.cc (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.cc...
> chrome/common/pref_names.cc:1416: const char kVirtualPrinterDriverEnabled[] =
> "virtual_driver.enabled";
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > This goes up with the service process prefs group. Consider using
> > cloud_print.enable_virtual_driver
> 
> Done.
> 
> http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h
> File chrome/common/pref_names.h (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/common/pref_names.h#...
> chrome/common/pref_names.h:519: extern const char
> kVirtualPrinterDriverEnabled[];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > See comment in the .cc file - when moved there, it should be moved to the
> > corresponding spot in here.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
> File chrome/service/chrome_service_application_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
> chrome/service/chrome_service_application_mac.mm:52: LOG(ERROR) <<
> GetMacOSStatusCommentString(status);
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > Is it reasonable to continue from here or should you just return?
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
> chrome/service/chrome_service_application_mac.mm:73: params.initialEvent =
> (AppleEvent*)  [sendEvent aeDesc];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > prefer static_cast<>()
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/chrome/service/chrome_servi...
> chrome/service/chrome_service_application_mac.mm:91: (ServiceCrApplication*)
> [ServiceCrApplication sharedApplication];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > prefer static_cast<>()
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/posix/backend.gyp (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/backend.gyp:74: 'installer_util_mac.mm',
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > put in alphabetical order
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/posix/installer_util_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/installer_util_mac.mm:35: std::cerr <<
"Failed
> to make path ref";
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > Add a comment explaining the iostream use.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/installer_util_mac.mm:41:
> NSAppleEventDescriptor* sendEvent =
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > Be consistent with error checking - check for nil.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/installer_util_mac.mm:47:
> LSApplicationParameters params = { 0, kLSLaunchDefaults , &ref, NULL, NULL,
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > nit: no space before comma
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/installer_util_mac.mm:49: params.initialEvent
=
> (AppleEvent*) [sendEvent aeDesc];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > prefer static_cast<>()
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:55:
> NSAppleEventDescriptor* parameters = [NSAppleEventDescriptor listDescriptor];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > The order of construction here is funny; I would not even start constructing
> > parameters until after the event was created.
> > 
> > Also, there's a nil check on event but not on parameters. Both affect
whether
> > you send a valid message.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:72:
> LSApplicationParameters params = {0, kLSLaunchDefaults , &ref, NULL, NULL,
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > ...I would move this down to just above where the initial event is set
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:75: [event
> setParamDescriptor:parameters forKeyword:cloudPrintClass];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > ...and I would move this up to as soon as parameters was constructed.
> > 
> > The idea is to increase the locality of what's being done. Avoid card
> shuffling
> > initialization sequences.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:76:
> params.initialEvent =(AppleEvent*) [event aeDesc];
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > nit: space after =, prefer static_cast<>()
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/21003/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:80: LOG(ERROR)
> <<"Unable to launch";
> On 2011/08/02 23:52:52, Scott Byer wrote:
> > nit: space after operator
> 
> Done.

[Scott Byer, 2011-08-04 20:43:54]

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/installer_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/installer_util_mac.mm:47: if(sendEvent == nil)
{
nit: space after if

[Nico, 2011-08-04 20:53:12]

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

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:6: 'targets': [ ],
is this needed? remove?

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:8: 'variables': {
'chromium_code':1},
space in front of }

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:17: 'targets' : [
no space in front of :

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:35: 'target_name': 'GCP-driver',
Looks like this is the same target on linux and mac. Do make the target
unconditional and just the per-platform stuff (xcode_framework_dirs)
os-dependent. chromium_code lets you import build/common.gypi implicitly, which
means you can keep most of the 'sources' section unconditional.

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:40: 'msvs_guid':
'8D06D53B-289E-4f99-99FC-77C77DB478A8',
remove

[abeera, 2011-08-04 21:53:35]

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

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:8: 'variables': {
'chromium_code':1},
On 2011/08/04 20:53:12, Nico wrote:
> space in front of }

Done.

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:35: 'target_name': 'GCP-driver',
On 2011/08/04 20:53:12, Nico wrote:
> Looks like this is the same target on linux and mac. Do make the target
> unconditional and just the per-platform stuff (xcode_framework_dirs)
> os-dependent. chromium_code lets you import build/common.gypi implicitly,
which
> means you can keep most of the 'sources' section unconditional.

Done.

http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:40: 'msvs_guid':
'8D06D53B-289E-4f99-99FC-77C77DB478A8',
On 2011/08/04 20:53:12, Nico wrote:
> remove

Done.

[Nico, 2011-08-04 22:04:38]

LGTM

Please send this to the normal trybots as well as to mac_clang and linux_clang.
You will need OWNERS reviews, too.

http://codereview.chromium.org/7485011/diff/44004/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/44004/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.h:13: @interface
ServiceCrApplication : CrApplication
Needs a comment

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

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:7: {
Indent this whole block by 2

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:22: 'sources!':
['../virtual_driver_switches.cc'],
dedent everything in here by 2

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:27: ]
nit: we usually keep trailing commas on these (previous line too)

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:29: 'target_defaults': {
this goes above 'targets'

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:35: {
indent 1

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:85:
LSApplicationParameters params = { 0, kLSLaunchDefaults , &ref, NULL, NULL,
break after { instead

[Scott Byer, 2011-08-04 22:34:27]

LGTM for the cloud_print and chrome/service parts. You'll need to get a review
from one of the content/common OWNERS.


On 2011/08/04 21:53:35, abeera wrote:
>
http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
> File cloud_print/virtual_driver/posix/backend.gyp (right):
> 
>
http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/backend.gyp:8: 'variables': {
> 'chromium_code':1},
> On 2011/08/04 20:53:12, Nico wrote:
> > space in front of }
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/backend.gyp:35: 'target_name': 'GCP-driver',
> On 2011/08/04 20:53:12, Nico wrote:
> > Looks like this is the same target on linux and mac. Do make the target
> > unconditional and just the per-platform stuff (xcode_framework_dirs)
> > os-dependent. chromium_code lets you import build/common.gypi implicitly,
> which
> > means you can keep most of the 'sources' section unconditional.
> 
> Done.
> 
>
http://codereview.chromium.org/7485011/diff/34006/cloud_print/virtual_driver/...
> cloud_print/virtual_driver/posix/backend.gyp:40: 'msvs_guid':
> '8D06D53B-289E-4f99-99FC-77C77DB478A8',
> On 2011/08/04 20:53:12, Nico wrote:
> > remove
> 
> Done.

[brettw, 2011-08-05 23:15:17]

http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
File content/common/cloud_print_class_mac.h (right):

http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:12: extern const AEEventClass
cloudPrintClass;
Is this the correct style for this name? I'm not so familar with Obj-C style,
but I would expect this to use more Google-standard constant naming
(kCloudPrintClass). Something more specific like kCloudPrintAEEventClass might
be better for this global constant.

I'd feel better if this was in the content namespace. We do this for many of the
constants in common to keep them out of the global namespace.

[abeera, 2011-08-08 17:10:48]

http://codereview.chromium.org/7485011/diff/44004/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/44004/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.h:13: @interface
ServiceCrApplication : CrApplication
On 2011/08/04 22:04:38, Nico wrote:
> Needs a comment

Done.

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

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:7: {
On 2011/08/04 22:04:38, Nico wrote:
> Indent this whole block by 2

Done.

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:22: 'sources!':
['../virtual_driver_switches.cc'],
On 2011/08/04 22:04:38, Nico wrote:
> dedent everything in here by 2

Done.

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:27: ]
On 2011/08/04 22:04:38, Nico wrote:
> nit: we usually keep trailing commas on these (previous line too)

Done.

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:29: 'target_defaults': {
On 2011/08/04 22:04:38, Nico wrote:
> this goes above 'targets'

Done.

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/backend.gyp:35: {
On 2011/08/04 22:04:38, Nico wrote:
> indent 1

Done.

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
File cloud_print/virtual_driver/posix/printer_driver_util_mac.mm (right):

http://codereview.chromium.org/7485011/diff/44004/cloud_print/virtual_driver/...
cloud_print/virtual_driver/posix/printer_driver_util_mac.mm:85:
LSApplicationParameters params = { 0, kLSLaunchDefaults , &ref, NULL, NULL,
On 2011/08/04 22:04:38, Nico wrote:
> break after { instead

Done.

[abeera, 2011-08-08 17:12:14]

Thank you for your feedback.

http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
File content/common/cloud_print_class_mac.h (right):

http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:12: extern const AEEventClass
cloudPrintClass;
On 2011/08/05 23:15:18, brettw wrote:
> Is this the correct style for this name? I'm not so familar with Obj-C style,
> but I would expect this to use more Google-standard constant naming
> (kCloudPrintClass). Something more specific like kCloudPrintAEEventClass might
> be better for this global constant.
> 
> I'd feel better if this was in the content namespace. We do this for many of
the
> constants in common to keep them out of the global namespace.

Done.

[abeera, 2011-08-09 23:54:57]

I made a few changes to printer_driver_util_mac.mm to handle the first install
case, and rebased off trunk. Please take a look when free. Thanks. 
On 2011/08/08 17:12:14, abeera wrote:
> Thank you for your feedback.
> 
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> File content/common/cloud_print_class_mac.h (right):
> 
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> content/common/cloud_print_class_mac.h:12: extern const AEEventClass
> cloudPrintClass;
> On 2011/08/05 23:15:18, brettw wrote:
> > Is this the correct style for this name? I'm not so familar with Obj-C
style,
> > but I would expect this to use more Google-standard constant naming
> > (kCloudPrintClass). Something more specific like kCloudPrintAEEventClass
might
> > be better for this global constant.
> > 
> > I'd feel better if this was in the content namespace. We do this for many of
> the
> > constants in common to keep them out of the global namespace.
> 
> Done.

[abeera, 2011-08-12 17:40:52]

Please take a look when possible. I'm an intern ending next Friday, so  a quick
reply would be appreciated.
On 2011/08/09 23:54:57, abeera wrote:
> I made a few changes to printer_driver_util_mac.mm to handle the first install
> case, and rebased off trunk. Please take a look when free. Thanks. 
> On 2011/08/08 17:12:14, abeera wrote:
> > Thank you for your feedback.
> > 
> >
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> > File content/common/cloud_print_class_mac.h (right):
> > 
> >
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> > content/common/cloud_print_class_mac.h:12: extern const AEEventClass
> > cloudPrintClass;
> > On 2011/08/05 23:15:18, brettw wrote:
> > > Is this the correct style for this name? I'm not so familar with Obj-C
> style,
> > > but I would expect this to use more Google-standard constant naming
> > > (kCloudPrintClass). Something more specific like kCloudPrintAEEventClass
> might
> > > be better for this global constant.
> > > 
> > > I'd feel better if this was in the content namespace. We do this for many
of
> > the
> > > constants in common to keep them out of the global namespace.
> > 
> > Done.

[Nico, 2011-08-12 17:43:01]

On 2011/08/12 17:40:52, abeera wrote:
> Please take a look when possible. I'm an intern ending next Friday, so  a
quick
> reply would be appreciated.

^ brettw, this is waiting for you

> On 2011/08/09 23:54:57, abeera wrote:
> > I made a few changes to printer_driver_util_mac.mm to handle the first
install
> > case, and rebased off trunk. Please take a look when free. Thanks. 
> > On 2011/08/08 17:12:14, abeera wrote:
> > > Thank you for your feedback.
> > > 
> > >
> >
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> > > File content/common/cloud_print_class_mac.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/7485011/diff/44032/content/common/cloud_print_...
> > > content/common/cloud_print_class_mac.h:12: extern const AEEventClass
> > > cloudPrintClass;
> > > On 2011/08/05 23:15:18, brettw wrote:
> > > > Is this the correct style for this name? I'm not so familar with Obj-C
> > style,
> > > > but I would expect this to use more Google-standard constant naming
> > > > (kCloudPrintClass). Something more specific like kCloudPrintAEEventClass
> > might
> > > > be better for this global constant.
> > > > 
> > > > I'd feel better if this was in the content namespace. We do this for
many
> of
> > > the
> > > > constants in common to keep them out of the global namespace.
> > > 
> > > Done.

[brettw, 2011-08-12 21:06:50]

LGTM, mostly style nits, I'm assuming others have reviewed the logic since I
don't really understand the larger picture.

http://codereview.chromium.org/7485011/diff/64002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1395: 
I'd only have one blank line here.

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.h:20: namespace
chrome_service_application_mac {
Blank line after this.

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:100: } // namespace
chrome_service_application_mac
Two spaces before //

http://codereview.chromium.org/7485011/diff/64002/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:112:
IPC_MESSAGE_HANDLER(ServiceMsg_EnableVirtualDriver,
How about using IPC_MESSAGE_FORWARD so you can avoid implementing the one-line
functions below.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
File content/common/cloud_print_class_mac.h (right):

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:10: namespace content {
Blank line before & after this.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:13: }    // namespace content
Two spaces before the //, blank line before this.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
File content/common/cloud_print_class_mac.mm (right):

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.mm:9: }   // namespace content
Same comments as above re blank lines and spacing before //

[abeera, 2011-08-15 16:23:06]

http://codereview.chromium.org/7485011/diff/64002/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/common/pref_names.cc...
chrome/common/pref_names.cc:1395: 
On 2011/08/12 21:06:51, brettw wrote:
> I'd only have one blank line here.

Done.

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.h (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.h:20: namespace
chrome_service_application_mac {
On 2011/08/12 21:06:51, brettw wrote:
> Blank line after this.

Done.

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
File chrome/service/chrome_service_application_mac.mm (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/chrome_servi...
chrome/service/chrome_service_application_mac.mm:100: } // namespace
chrome_service_application_mac
On 2011/08/12 21:06:51, brettw wrote:
> Two spaces before //

Done.

http://codereview.chromium.org/7485011/diff/64002/chrome/service/service_ipc_...
File chrome/service/service_ipc_server.cc (right):

http://codereview.chromium.org/7485011/diff/64002/chrome/service/service_ipc_...
chrome/service/service_ipc_server.cc:112:
IPC_MESSAGE_HANDLER(ServiceMsg_EnableVirtualDriver,
All the IPC calls are handled by implementing small functions; I wanted to be
consistent.
On 2011/08/12 21:06:51, brettw wrote:
> How about using IPC_MESSAGE_FORWARD so you can avoid implementing the one-line
> functions below.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
File content/common/cloud_print_class_mac.h (right):

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:10: namespace content {
On 2011/08/12 21:06:51, brettw wrote:
> Blank line before & after this.

Done.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.h:13: }    // namespace content
On 2011/08/12 21:06:51, brettw wrote:
> Two spaces before the //, blank line before this.

Done.

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
File content/common/cloud_print_class_mac.mm (right):

http://codereview.chromium.org/7485011/diff/64002/content/common/cloud_print_...
content/common/cloud_print_class_mac.mm:9: }   // namespace content
On 2011/08/12 21:06:51, brettw wrote:
> Same comments as above re blank lines and spacing before //

Done.

[commit-bot: I haz the power, 2011-08-15 19:30:19]

Can't process patch for file
cloud_print/virtual_driver/virtual_driver_posix.gyp.
A +

[jam, 2011-10-27 05:43:48]

I just saw this change. the cloud_print files shouldn't have gone in to content.
see http://dev.chromium.org/developers/content-module

[Albert Bodenhamer, 2011-10-27 15:51:31]

It'll be a few days before I can get to this.  If you want that code out of
your way sooner, this stuff should be pretty easy to roll back.

On Wed, Oct 26, 2011 at 10:43 PM, <jam@chromium.org> wrote:

> I just saw this change. the cloud_print files shouldn't have gone in to
> content.
> see
http://dev.chromium.org/**developers/content-module<http://dev.chromium.org/d...
>
>
http://codereview.chromium.**org/7485011/<http://codereview.chromium.org/7485...
>



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

[jam, 2011-10-27 20:16:39]

I'll just move it myself, just wanted to point this out for future changes.

On Thu, Oct 27, 2011 at 8:51 AM, Albert Bodenhamer <abodenha@chromium.org>wrote:

> It'll be a few days before I can get to this.  If you want that code out of
> your way sooner, this stuff should be pretty easy to roll back.
>
>
> On Wed, Oct 26, 2011 at 10:43 PM, <jam@chromium.org> wrote:
>
>> I just saw this change. the cloud_print files shouldn't have gone in to
>> content.
>> see
http://dev.chromium.org/**developers/content-module<http://dev.chromium.org/d...
>>
>>
http://codereview.chromium.**org/7485011/<http://codereview.chromium.org/7485...
>>
>
>
>
> --
> Albert Bodenhamer | Software Engineer |
abodenha@chromium.<abodenha@google.com>
> org
>