Implement the mac policy provider based on the AsyncPolicyLoader.

Implement the mac policy provider based on the AsyncPolicyLoader.

BUG=130918
TEST=policies still work on mac, unit_tests green

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

[Joao da Silva, 2012-06-03 14:19:59]

CL 2/5: refactored the mac provider to be based on the AsyncPolicyLoader.

Manually verified to work :-) Please review, thanks!

[Mattias Nissler (ping if slow), 2012-06-04 09:19:02]

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.cc (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.cc:91: return new
AsyncPolicyProvider(policy_list, loader);
It seems this function will look pretty much the same over the different
provider implementations. Should we rather have a CreateLoader() function?

Also, is the policy_list parameter on the provider still appropriate? IIRC,
having it on the provider is just a convenience for the implementations. With
the rewrite, the provider doesn't need it any longer.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.h (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:27: class MacPolicyLoader : public
AsyncPolicyLoader {
Shouldn't the class be named PolicyLoaderMac according to the file name?

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:43: // The returned value is owned by
the caller.
return a scoped_ptr then?

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:53: FilePath managed_policy_path_;
what's this?

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac_unittest.cc (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac_unittest.cc:222: class
MacPolicyLoaderTest : public PolicyTestBase {
Both the test case instance and this test fixture are now called
MacPolicyLoaderTest. gmock can probably handle that, but it'd rather use
different names to avoid confusion in human consumers.

[Joao da Silva, 2012-06-04 17:55:14]

This has been rebased on the recent AsyncPolicyLoader changes, and I've manually
verified that it works. Please have another look.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.cc (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.cc:91: return new
AsyncPolicyProvider(policy_list, loader);
On 2012/06/04 09:19:02, Mattias Nissler wrote:
> It seems this function will look pretty much the same over the different
> provider implementations. Should we rather have a CreateLoader() function?

Right, I think even CreateLoader() would be a very thin wrapper around the ctor.
I've just removed this.

> 
> Also, is the policy_list parameter on the provider still appropriate? IIRC,
> having it on the provider is just a convenience for the implementations. With
> the rewrite, the provider doesn't need it any longer.

Nice catch. I'll remove it in another CL after landing the changes to all the
providers.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.h (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:27: class MacPolicyLoader : public
AsyncPolicyLoader {
On 2012/06/04 09:19:02, Mattias Nissler wrote:
> Shouldn't the class be named PolicyLoaderMac according to the file name?

I prefer the name MacPolicyLoader, but the file has to end in _mac.h for the
conditional gyp rules to trigger. wdyt?

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:43: // The returned value is owned by
the caller.
On 2012/06/04 09:19:02, Mattias Nissler wrote:
> return a scoped_ptr then?

This is here just so that unit_tests can exercise this function. All the call
sites transfer the pointer to another Dictionary/List immediately, and become
clunkier if they have to .release(). I'd leave as is unless you feel strongly
about it.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:53: FilePath managed_policy_path_;
On 2012/06/04 09:19:02, Mattias Nissler wrote:
> what's this?

Added a comment.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac_unittest.cc (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac_unittest.cc:222: class
MacPolicyLoaderTest : public PolicyTestBase {
On 2012/06/04 09:19:02, Mattias Nissler wrote:
> Both the test case instance and this test fixture are now called
> MacPolicyLoaderTest. gmock can probably handle that, but it'd rather use
> different names to avoid confusion in human consumers.

Done.

[Mattias Nissler (ping if slow), 2012-06-05 11:55:59]

I think we should actually name the loader PolicyLoaderMac (see my comment).
Other than that lgtm.

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.h (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:27: class MacPolicyLoader : public
AsyncPolicyLoader {
On 2012/06/04 17:55:14, Joao da Silva wrote:
> On 2012/06/04 09:19:02, Mattias Nissler wrote:
> > Shouldn't the class be named PolicyLoaderMac according to the file name?
> 
> I prefer the name MacPolicyLoader, but the file has to end in _mac.h for the
> conditional gyp rules to trigger. wdyt?

The rest of the code base seems to favor the Mac suffix:

git ls-files | grep '_mac.h' | while read file; do grep -r 'class Mac\w\+ '
$file; done | wc -l
-> 4

git ls-files | grep '_mac.h' | while read file; do grep -r 'class \w\+Mac '
$file; done | wc -l
-> 39

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:43: // The returned value is owned by
the caller.
On 2012/06/04 17:55:14, Joao da Silva wrote:
> On 2012/06/04 09:19:02, Mattias Nissler wrote:
> > return a scoped_ptr then?
> 
> This is here just so that unit_tests can exercise this function. All the call
> sites transfer the pointer to another Dictionary/List immediately, and become
> clunkier if they have to .release(). I'd leave as is unless you feel strongly
> about it.

OK.

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:46: class
PolicyTestBase : public testing::Test {
AsynchronousPolicyTestBase is going away I guess? (didn't check the other CL,
just want to confirm).

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_mac_unittest.cc (right):

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_mac_unittest.cc:230:
scoped_ptr<AsyncPolicyLoader>(loader_)) {}
Essentially, we could rewrite these tests now without going through a provider,
but I think that's not something we need to change right now.

[Joao da Silva, 2012-06-05 13:52:10]

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_loader_mac.h (right):

http://codereview.chromium.org/10496013/diff/9001/chrome/browser/policy/polic...
chrome/browser/policy/policy_loader_mac.h:27: class MacPolicyLoader : public
AsyncPolicyLoader {
On 2012/06/05 11:55:59, Mattias Nissler wrote:
> On 2012/06/04 17:55:14, Joao da Silva wrote:
> > On 2012/06/04 09:19:02, Mattias Nissler wrote:
> > > Shouldn't the class be named PolicyLoaderMac according to the file name?
> > 
> > I prefer the name MacPolicyLoader, but the file has to end in _mac.h for the
> > conditional gyp rules to trigger. wdyt?
> 
> The rest of the code base seems to favor the Mac suffix:
> 
> git ls-files | grep '_mac.h' | while read file; do grep -r 'class Mac\w\+ '
> $file; done | wc -l
> -> 4
> 
> git ls-files | grep '_mac.h' | while read file; do grep -r 'class \w\+Mac '
> $file; done | wc -l
> -> 39

Nice! Renaming done.

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10496013/diff/16001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:46: class
PolicyTestBase : public testing::Test {
On 2012/06/05 11:55:59, Mattias Nissler wrote:
> AsynchronousPolicyTestBase is going away I guess? (didn't check the other CL,
> just want to confirm).

Yes, the cleanup CL deletes it.

[commit-bot: I haz the power, 2012-06-05 13:56:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10496013/27001

[commit-bot: I haz the power, 2012-06-05 15:43:40]

Change committed as 140530