Dynamic policy refresh support for the Mac.

Dynamic policy refresh support for the Mac.

Adapt the file watching code we already had for ConfigDirPolicyProvider to support a loader delegate, make the old code use it and change the Mac policy provider to watch for changes to the plist file in /Library/Managed Preferences/<username>.

BUG=52040
TEST=unit tests

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

[Mattias Nissler (ping if slow), 2010-10-26 15:16:18]

Avi, can you review this? It looks a bit large, but most of it is just moving
code around, so the existing watch-and-reload policy provider code can also be
used for the Mac provider. I've also put in code to look up the /Library
directory, I hope I found the Apple way of doing it :)

[Paweł Hajdan Jr., 2010-10-26 15:56:33]

Drive-by with some testing comments.

http://codereview.chromium.org/4062002/diff/8001/9005
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9005#newcode197
chrome/browser/policy/config_dir_policy_provider_unittest.cc:197:
ASSERT_TRUE(file_util::Delete(test_dir_, true));
Could you convert this to ScopedTempDir?

http://codereview.chromium.org/4062002/diff/8001/9005#newcode203
chrome/browser/policy/config_dir_policy_provider_unittest.cc:203: MessageLoop
loop_;
Do all of those need to be protected and not private?

[Avi (use Gerrit), 2010-10-26 20:29:03]

I really like this. LGTM

[danno, 2010-10-27 09:16:35]

Drive-by review.

http://codereview.chromium.org/4062002/diff/8001/9006
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9006#newcode34
chrome/browser/policy/configuration_policy_provider_mac.cc:34: return
path.Append(base::SysCFStringRefToUTF8(bundle_id) + ".plist");
is there a constant for ".plist" somewhere? what do you think about making it
one?

http://codereview.chromium.org/4062002/diff/8001/9007
File chrome/browser/policy/configuration_policy_provider_mac.h (right):

http://codereview.chromium.org/4062002/diff/8001/9007#newcode27
chrome/browser/policy/configuration_policy_provider_mac.h:27:
scoped_ptr<MacPreferences> preferences_;
All members after typedefs:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

http://codereview.chromium.org/4062002/diff/8001/9007#newcode35
chrome/browser/policy/configuration_policy_provider_mac.h:35: std::string name;
Seems very similar to the char* -> string mechanism that the policy provider
needs itself internally for similar reasons. Is it a good idea to share some
code/declarations?

http://codereview.chromium.org/4062002/diff/8001/9007#newcode49
chrome/browser/policy/configuration_policy_provider_mac.h:49: const
ConfigurationPolicyProvider::StaticPolicyValueMap& policy_map);
If you changed PolicyValueMap -> PolicyList, then doesn't it make sense to
change StaticPolicyValueMap to StaticPolicyList, too?

http://codereview.chromium.org/4062002/diff/8001/9008
File chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
(right):

http://codereview.chromium.org/4062002/diff/8001/9008#newcode145
chrome/browser/policy/configuration_policy_provider_mac_unittest.cc:145:
ConfigurationPolicyPrefStore::GetChromePolicyValueMap(), prefs_);
Why the change here? You have removed 5 lines but added 8, which are all
copy-and-paste. If there is a technical reason why you did this (e.g. in
TestNonForcedValue the construction has to be after the pref_ change), then
please include a comment.

http://codereview.chromium.org/4062002/diff/8001/9009
File chrome/browser/policy/file_based_policy_provider.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9009#newcode137
chrome/browser/policy/file_based_policy_provider.cc:137: LOG(INFO) << "Policy
reload from " << config_file_path().value()
A candidate for VLOG?

http://codereview.chromium.org/4062002/diff/8001/9010
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/8001/9010#newcode36
chrome/browser/policy/file_based_policy_provider.h:36: class Delegate {
The Delegate doesn't really qualify as a pure interface, since the constructor
takes an argument. Since you multiply inherit from this, seems like a coding
style violation:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

http://codereview.chromium.org/4062002/diff/8001/9010#newcode56
chrome/browser/policy/file_based_policy_provider.h:56: const FilePath
config_file_path_;
DISALLOW_COPY_AND_ASSIGN here?

[Mattias Nissler (ping if slow), 2010-10-27 17:50:06]

I adressed all of your comments. Can you take another look? Note that I reworked
the policy value map handling quite a bit to make it easier to use and get rid
of the redundant map building in the mac loader delegate.

http://codereview.chromium.org/4062002/diff/8001/9005
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9005#newcode197
chrome/browser/policy/config_dir_policy_provider_unittest.cc:197:
ASSERT_TRUE(file_util::Delete(test_dir_, true));
On 2010/10/26 15:56:33, Paweł Hajdan Jr. wrote:
> Could you convert this to ScopedTempDir?

Done.

http://codereview.chromium.org/4062002/diff/8001/9005#newcode203
chrome/browser/policy/config_dir_policy_provider_unittest.cc:203: MessageLoop
loop_;
On 2010/10/26 15:56:33, Paweł Hajdan Jr. wrote:
> Do all of those need to be protected and not private?

Done.

http://codereview.chromium.org/4062002/diff/8001/9006
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9006#newcode34
chrome/browser/policy/configuration_policy_provider_mac.cc:34: return
path.Append(base::SysCFStringRefToUTF8(bundle_id) + ".plist");
On 2010/10/27 09:16:35, danno wrote:
> is there a constant for ".plist" somewhere? what do you think about making it
> one?

I don't think it's worth it, since the local constant is cheap, and there's the
hassle of an added dependency and having to educate people to use it.

http://codereview.chromium.org/4062002/diff/8001/9007
File chrome/browser/policy/configuration_policy_provider_mac.h (right):

http://codereview.chromium.org/4062002/diff/8001/9007#newcode27
chrome/browser/policy/configuration_policy_provider_mac.h:27:
scoped_ptr<MacPreferences> preferences_;
On 2010/10/27 09:16:35, danno wrote:
> All members after typedefs:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

Done.

http://codereview.chromium.org/4062002/diff/8001/9007#newcode35
chrome/browser/policy/configuration_policy_provider_mac.h:35: std::string name;
On 2010/10/27 09:16:35, danno wrote:
> Seems very similar to the char* -> string mechanism that the policy provider
> needs itself internally for similar reasons. Is it a good idea to share some
> code/declarations? 

Done. Now using a copy created at construction time.

http://codereview.chromium.org/4062002/diff/8001/9007#newcode49
chrome/browser/policy/configuration_policy_provider_mac.h:49: const
ConfigurationPolicyProvider::StaticPolicyValueMap& policy_map);
On 2010/10/27 09:16:35, danno wrote:
> If you changed PolicyValueMap -> PolicyList, then doesn't it make sense to
> change StaticPolicyValueMap to StaticPolicyList, too?

Doesn't apply any longer.

http://codereview.chromium.org/4062002/diff/8001/9008
File chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
(right):

http://codereview.chromium.org/4062002/diff/8001/9008#newcode145
chrome/browser/policy/configuration_policy_provider_mac_unittest.cc:145:
ConfigurationPolicyPrefStore::GetChromePolicyValueMap(), prefs_);
On 2010/10/27 09:16:35, danno wrote:
> Why the change here? You have removed 5 lines but added 8, which are all
> copy-and-paste. If there is a technical reason why you did this (e.g. in
> TestNonForcedValue the construction has to be after the pref_ change), then
> please include a comment.

That's exactly the reason, added comments to all the affected tests.

http://codereview.chromium.org/4062002/diff/8001/9009
File chrome/browser/policy/file_based_policy_provider.cc (right):

http://codereview.chromium.org/4062002/diff/8001/9009#newcode137
chrome/browser/policy/file_based_policy_provider.cc:137: LOG(INFO) << "Policy
reload from " << config_file_path().value()
On 2010/10/27 09:16:35, danno wrote:
> A candidate for VLOG?

OK, I guess you can tell people to enable in order to debug things.

http://codereview.chromium.org/4062002/diff/8001/9010
File chrome/browser/policy/file_based_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/8001/9010#newcode36
chrome/browser/policy/file_based_policy_provider.h:36: class Delegate {
On 2010/10/27 09:16:35, danno wrote:
> The Delegate doesn't really qualify as a pure interface, since the constructor
> takes an argument. Since you multiply inherit from this, seems like a coding
> style violation:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

I don't have multiple inheritance here. In fact, the provider takes ownership of
the delegate, so you have to provider an independent object anyway.

http://codereview.chromium.org/4062002/diff/8001/9010#newcode56
chrome/browser/policy/file_based_policy_provider.h:56: const FilePath
config_file_path_;
On 2010/10/27 09:16:35, danno wrote:
> DISALLOW_COPY_AND_ASSIGN here?

Done.

[Paweł Hajdan Jr., 2010-10-28 07:39:24]

Code I commented in the drive-by LGTM with comments. Thanks!

http://codereview.chromium.org/4062002/diff/20001/21005
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/20001/21005#newcode27
chrome/browser/policy/config_dir_policy_provider_unittest.cc:27:
ASSERT_TRUE(file_util::DirectoryExists(test_dir()));
nit: This is redundant, could you remove it?

If you want some checks, then I'd suggest adding
EXPECT_TRUE(test_dir_.IsValid()) to test_dir().

http://codereview.chromium.org/4062002/diff/20001/21018
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/20001/21018#newcode71
chrome/browser/policy/file_based_policy_provider_unittest.cc:71: BrowserThread
ui_thread_;
Could you make the BrowserThread members private?

[Mattias Nissler (ping if slow), 2010-10-28 08:21:29]

http://codereview.chromium.org/4062002/diff/20001/21005
File chrome/browser/policy/config_dir_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/20001/21005#newcode27
chrome/browser/policy/config_dir_policy_provider_unittest.cc:27:
ASSERT_TRUE(file_util::DirectoryExists(test_dir()));
On 2010/10/28 07:39:24, Paweł Hajdan Jr. wrote:
> nit: This is redundant, could you remove it?
> 
> If you want some checks, then I'd suggest adding
> EXPECT_TRUE(test_dir_.IsValid()) to test_dir().

Done.

http://codereview.chromium.org/4062002/diff/20001/21018
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/4062002/diff/20001/21018#newcode71
chrome/browser/policy/file_based_policy_provider_unittest.cc:71: BrowserThread
ui_thread_;
On 2010/10/28 07:39:24, Paweł Hajdan Jr. wrote:
> Could you make the BrowserThread members private?

Done.

[Mattias Nissler (ping if slow), 2010-10-28 11:57:09]

New version that uses statically declared policy maps exclusively. Danno, how do
you like this variant?

[danno, 2010-10-28 16:01:56]

I like this much better. Almost there, see my comments below.

http://codereview.chromium.org/4062002/diff/27001/28004
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/27001/28004#newcode33
chrome/browser/policy/config_dir_policy_provider.h:33:
ConfigDirPolicyProvider(const PolicyDefinitionMap* policy_map,
In the last round you started to move towards "list" terminology instead of
"map". I think I like that better (or perhaps even PolicyDefinitionArray).
What's your reasoning for switching back?

http://codereview.chromium.org/4062002/diff/27001/28009
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/27001/28009#newcode38
chrome/browser/policy/configuration_policy_provider.h:38: // existing object.
Comment is wrong: no sharing.

http://codereview.chromium.org/4062002/diff/34001/35010
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/4062002/diff/34001/35010#newcode50
chrome/browser/policy/configuration_policy_provider_mac.cc:50:
policy_map_->begin);
Just for readability, can you declare current outside the loop and just
initialize it in the loop. Gets rid of weird wrapping.

[Mattias Nissler (ping if slow), 2010-10-28 21:36:41]

Danno, how about this version?

http://codereview.chromium.org/4062002/diff/27001/28004
File chrome/browser/policy/config_dir_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/27001/28004#newcode33
chrome/browser/policy/config_dir_policy_provider.h:33:
ConfigDirPolicyProvider(const PolicyDefinitionMap* policy_map,
On 2010/10/28 16:01:56, danno wrote:
> In the last round you started to move towards "list" terminology instead of
> "map". I think I like that better (or perhaps even PolicyDefinitionArray).
> What's your reasoning for switching back?

I really don't have a preference, changed it to PolicyDefinitionList now :)

http://codereview.chromium.org/4062002/diff/27001/28009
File chrome/browser/policy/configuration_policy_provider.h (right):

http://codereview.chromium.org/4062002/diff/27001/28009#newcode38
chrome/browser/policy/configuration_policy_provider.h:38: // existing object.
On 2010/10/28 16:01:56, danno wrote:
> Comment is wrong: no sharing.

Doesn't apply any longer.

http://codereview.chromium.org/4062002/diff/34001/35010
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/4062002/diff/34001/35010#newcode50
chrome/browser/policy/configuration_policy_provider_mac.cc:50:
policy_map_->begin);
On 2010/10/28 16:01:56, danno wrote:
> Just for readability, can you declare current outside the loop and just
> initialize it in the loop. Gets rid of weird wrapping. 

Done.

[danno, 2010-10-29 11:53:58]

LGTM.