Cloud Print Policy - service side enforcement, Mac browser tests.

Cloud Print Policy - service side enforcement, Mac browser tests.

Enhance MockLaunchd to the point where it can easily be injected into full
service process browser/multiprocess tests, letting those tests run on the
Mac. This gets us to the point where we can more browser/service process
interaction test coverage.

BUG=98049
TEST=none


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

[Scott Byer, 2012-01-20 20:08:01]

Mark for the mock_launchd portions, Albert for the service process portions.

[Albert Bodenhamer, 2012-01-20 20:34:18]

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:197:
FilePath executable_path = cl->GetSwitchValuePath(kTestExecutablePath);
I've seen crashes resulting from calling GetSwitchValuePath without first
testing HasSwitch.  Not sure if there will ever be a situation in this test
where it's not set but...

Maybe ASSERT HasSwitch?

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:207:
return -1;
What is the significance of -1 here?

http://codereview.chromium.org/9131016/diff/2005/chrome/common/service_proces...
File chrome/common/service_process_util.h (right):

http://codereview.chromium.org/9131016/diff/2005/chrome/common/service_proces...
chrome/common/service_process_util.h:20: #if defined(OS_MACOSX)
Wow.  Ugly pattern.  I see it's being used lots of other places though so OK.

[Mark Mentovai, 2012-01-20 20:41:34]

Nice job.

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:198:
EXPECT_NE(std::string(), executable_path.value());
EXPECT_FALSE(executable_path.value().empty()) is more of a “say what you mean”
way of writing this.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
File chrome/common/mac/mock_launchd.cc (right):

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:8: 
No blank line here.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:25: static const size_t kMaxPipeNameLength =
104;
Rather than hard-coding 104 again here, can you use sizeof, either here or
inline where you need it?

You could do

static sockaddr_un* throwaway_sockaddr_un;
static const size_t kMaxPipeNameLength =
sizeof(throwaway_sockaddr_un->sun_path);

and the compiler should throw away the unused struct, the only question would be
whether it would warn about it being unused or if it would stay quiet because
you’re using it, just not using its storage.

(This is a language wart, and the solutions are all inelegant, but having a
second 104 hard-code here is also inelegant.)

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:68: " 
<key>CFBundleShortVersionString</key>\n"
The keys in this dict were already sorted, so you should keep ’em that way.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:107: while (socket_path.value().length() +
pipe_suffix.length() >
Shouldn’t this be >= or -2? You’ll need to add a slash to socket_path and also
leave room for the terminating '\0'.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:108: kMaxPipeNameLength - 1)
Can we use {braces} here for readability because the condition is already taking
up more than one line?

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:109: socket_path = socket_path.DirName();
I guess this approach is OK for test code. The pathname length restriction in
this API is ridiculous, but it’s what we’ve got to deal with.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:124: CFStringRef socket_path =
CFStringCreateWithCString(kCFAllocatorDefault,
Leaked? Use a ScopedCFTypeRef.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:128: const void *values[] = { socket_path };
You can DCHECK (probably statically, even, a la COMPILE_ASSERT) that the two
arraysizes match.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:153: const void *values[] = { path, pid };
Here too? 177 also?

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:192: memset(&unix_addr, 0, sizeof(unix_addr));
Or just
  struct sockaddr_un unix_addr = {0};

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:195: "%s", pipe_name_.c_str());
Don’t use snprintf to null-terminate. You can use strlcpy, either the <string.h>
version, or probably better, the base::strlcpy version from string_util.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:197: size_t unix_addr_len = offsetof(struct
sockaddr_un,
I would prefer to hold off on computing this until you’ve finished populating
unix_addr. That puts the steps in order so the code will read more easily.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:208: CFSocketCreateWithSocketSignature(NULL,
&signature, NULL, NULL, NULL);
Does this consume the reference to signature.address or do you need to release
it yourself?

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:214: *error =
CFErrorCreate(kCFAllocatorDefault, kCFErrorDomainPOSIX,
{brace me}

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:229: const void *socket_values[] = { sockets
};
arraysizes equal? 238 too?

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
File chrome/common/mac/mock_launchd.h (right):

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.h:10: #include <string>
Blank line before.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/service_proces...
File chrome/common/service_process_util.h (right):

http://codereview.chromium.org/9131016/diff/2005/chrome/common/service_proces...
chrome/common/service_process_util.h:20: #if defined(OS_MACOSX)
Albert Bodenhamer wrote:
> Wow.  Ugly pattern.  I see it's being used lots of other places though so OK.

This is the standard way of doing this. Objective-C can be a drag about this.

[Scott Byer, 2012-01-20 23:13:37]

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:197:
FilePath executable_path = cl->GetSwitchValuePath(kTestExecutablePath);
On 2012/01/20 20:34:19, Albert Bodenhamer wrote:
> I've seen crashes resulting from calling GetSwitchValuePath without first
> testing HasSwitch.  Not sure if there will ever be a situation in this test
> where it's not set but...
> 
> Maybe ASSERT HasSwitch?

Added an error return instead, though I'm surprised about any crashes after
looking at the CommandLine code.

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:198:
EXPECT_NE(std::string(), executable_path.value());
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> EXPECT_FALSE(executable_path.value().empty()) is more of a “say what you mean”
> way of writing this.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/browser/printing/clou...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:207:
return -1;
On 2012/01/20 20:34:19, Albert Bodenhamer wrote:
> What is the significance of -1 here?

Added a comment at the top of the routine; renumbered the returns to clean up.
Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
File chrome/common/mac/mock_launchd.cc (right):

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:8: 
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> No blank line here.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:25: static const size_t kMaxPipeNameLength =
104;
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Rather than hard-coding 104 again here, can you use sizeof, either here or
> inline where you need it?
> 
> You could do
> 
> static sockaddr_un* throwaway_sockaddr_un;
> static const size_t kMaxPipeNameLength =
> sizeof(throwaway_sockaddr_un->sun_path);
> 
> and the compiler should throw away the unused struct, the only question would
be
> whether it would warn about it being unused or if it would stay quiet because
> you’re using it, just not using its storage.
> 
> (This is a language wart, and the solutions are all inelegant, but having a
> second 104 hard-code here is also inelegant.)

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:68: " 
<key>CFBundleShortVersionString</key>\n"
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> The keys in this dict were already sorted, so you should keep ’em that way.

They weren't completely, all fixed up.
Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:107: while (socket_path.value().length() +
pipe_suffix.length() >
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Shouldn’t this be >= or -2? You’ll need to add a slash to socket_path and also
> leave room for the terminating '\0'.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:108: kMaxPipeNameLength - 1)
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Can we use {braces} here for readability because the condition is already
taking
> up more than one line?

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:124: CFStringRef socket_path =
CFStringCreateWithCString(kCFAllocatorDefault,
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Leaked? Use a ScopedCFTypeRef.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:128: const void *values[] = { socket_path };
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> You can DCHECK (probably statically, even, a la COMPILE_ASSERT) that the two
> arraysizes match.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:153: const void *values[] = { path, pid };
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Here too? 177 also?

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:192: memset(&unix_addr, 0, sizeof(unix_addr));
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Or just
>   struct sockaddr_un unix_addr = {0};

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:195: "%s", pipe_name_.c_str());
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Don’t use snprintf to null-terminate. You can use strlcpy, either the
<string.h>
> version, or probably better, the base::strlcpy version from string_util.

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:197: size_t unix_addr_len = offsetof(struct
sockaddr_un,
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> I would prefer to hold off on computing this until you’ve finished populating
> unix_addr. That puts the steps in order so the code will read more easily.

Moved down to just before use. It's just complicated enough that I think keeping
it as a separate variable assignment is more clear than inlining it into the
function call.
Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:208: CFSocketCreateWithSocketSignature(NULL,
&signature, NULL, NULL, NULL);
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> Does this consume the reference to signature.address or do you need to release
> it yourself?

It's not spelled out explicitly in the docs, but based on the API style, I'd say
it's not, so I've added the CFRelease. (valgrind still isn't working on Lion, so
I'm waiting on the try bot run).
Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:214: *error =
CFErrorCreate(kCFAllocatorDefault, kCFErrorDomainPOSIX,
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> {brace me}

Done.

http://codereview.chromium.org/9131016/diff/2005/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:229: const void *socket_values[] = { sockets
};
On 2012/01/20 20:41:35, Mark Mentovai wrote:
> arraysizes equal? 238 too?

Done.

[Mark Mentovai, 2012-01-20 23:22:24]

LGTM with two suggestions and one “this might not compile” concern.

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
File chrome/common/mac/mock_launchd.cc (right):

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:200: int path_len =
Since strlcpy returns a size_t, you can make this a size_t now, and get rid of
the cast in the DCHECK.

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:215: CFRelease(signature.address);
I would have preferred a ScopedCFTypeRef<CFDataRef> to ward off the possibility
of future early returns. That would be the style of this file, I don’t see
CFRelease called directly anywhere else.

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:238: COMPILE_ASSERT(arraysize(socket_keys) ==
arraysize(socket_values),
This doesn’t cause a compilation failure? I think that the internal
implementation of COMPILE_ASSERT is such that array_sizes_must_match will show
up as a variable, and this will look like you’re redeclaring a variable at the
same scope as a previous declaration. Compiler error! Line 249 too.

[Scott Byer, 2012-01-20 23:59:35]

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
File chrome/common/mac/mock_launchd.cc (right):

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:200: int path_len =
On 2012/01/20 23:22:25, Mark Mentovai wrote:
> Since strlcpy returns a size_t, you can make this a size_t now, and get rid of
> the cast in the DCHECK.

Done.

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:215: CFRelease(signature.address);
On 2012/01/20 23:22:25, Mark Mentovai wrote:
> I would have preferred a ScopedCFTypeRef<CFDataRef> to ward off the
possibility
> of future early returns. That would be the style of this file, I don’t see
> CFRelease called directly anywhere else.

Done.

http://codereview.chromium.org/9131016/diff/5004/chrome/common/mac/mock_launc...
chrome/common/mac/mock_launchd.cc:238: COMPILE_ASSERT(arraysize(socket_keys) ==
arraysize(socket_values),
On 2012/01/20 23:22:25, Mark Mentovai wrote:
> This doesn’t cause a compilation failure? I think that the internal
> implementation of COMPILE_ASSERT is such that array_sizes_must_match will show
> up as a variable, and this will look like you’re redeclaring a variable at the
> same scope as a previous declaration. Compiler error! Line 249 too.

No, it didn't, weird, huh? Looking at the macro, I'm surprised, too. Fixed up
this one just for paranoia - 249 is OK, since 183 is in a scope.

[Mark Mentovai, 2012-01-21 00:43:44]

LGTM for the mock launchd stuff.

[Albert Bodenhamer, 2012-01-23 18:05:40]

LGTM

http://codereview.chromium.org/9131016/diff/13/chrome/browser/printing/cloud_...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

http://codereview.chromium.org/9131016/diff/13/chrome/browser/printing/cloud_...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:212:
return 2;
NIT: I'd really prefer something like kNotInitialized here.

http://codereview.chromium.org/9131016/diff/13/chrome/common/service_process_...
File chrome/common/service_process_util_mac.mm (right):

http://codereview.chromium.org/9131016/diff/13/chrome/common/service_process_...
chrome/common/service_process_util_mac.mm:90: NSString*
GetServiceProcessLaunchDSocketEnvVar() {
NIT: It looks like this code just changed position.

[Scott Byer, 2012-01-23 19:03:46]

http://codereview.chromium.org/9131016/diff/13/chrome/browser/printing/cloud_...
File
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc
(right):

http://codereview.chromium.org/9131016/diff/13/chrome/browser/printing/cloud_...
chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc:212:
return 2;
On 2012/01/23 18:05:41, Albert Bodenhamer wrote:
> NIT: I'd really prefer something like kNotInitialized here.

Done.

http://codereview.chromium.org/9131016/diff/13/chrome/common/service_process_...
File chrome/common/service_process_util_mac.mm (right):

http://codereview.chromium.org/9131016/diff/13/chrome/common/service_process_...
chrome/common/service_process_util_mac.mm:90: NSString*
GetServiceProcessLaunchDSocketEnvVar() {
On 2012/01/23 18:05:41, Albert Bodenhamer wrote:
> NIT: It looks like this code just changed position.

Yup, out of the anon. namespace so the tests could link to it.

[commit-bot: I haz the power, 2012-01-23 19:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/9131016/12001

[commit-bot: I haz the power, 2012-01-23 21:39:11]

Change committed as 118744