Implement the ConfigDirPolicyProvider based on the AsyncPolicyLoader

Implement the ConfigDirPolicyProvider based on the AsyncPolicyLoader.

Also makes the ConfigDirPolicyLoader load both mandatory and recommended policies, and removes the recommended_platform_provider from the connector; there's only one platform provider now.

BUG=108996, 130918
TEST=Nothing breaks

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

[Joao da Silva, 2012-05-31 13:38:07]

Here's another one, please review :-)

[Mattias Nissler (ping if slow), 2012-05-31 13:58:09]

almost looks good.

https://chromiumcodereview.appspot.com/10443108/diff/7001/chrome/browser/poli...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

https://chromiumcodereview.appspot.com/10443108/diff/7001/chrome/browser/poli...
chrome/browser/policy/config_dir_policy_provider.cc:51: // If the path does not
exist or points to a directory, it's safe to load.
should be "doesn't point to a directory", no?

https://chromiumcodereview.appspot.com/10443108/diff/7001/chrome/browser/poli...
chrome/browser/policy/config_dir_policy_provider.cc:52: if
(!file_util::GetFileInfo(config_file_path(), &file_info) ||
You need to do that on both of the directories. Have you tested this?
Continuously writing the policy file should stop Chrome from reloading. Until
you don't touch the file for 5 seconds.

[Joao da Silva, 2012-06-03 15:02:01]

CL 3/5: refactored the config dir provider to be based on the AsyncPolicyLoader.

Also manually verified to work. Please review, thanks!

http://codereview.chromium.org/10443108/diff/7001/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/10443108/diff/7001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.cc:51: // If the path does not
exist or points to a directory, it's safe to load.
On 2012/05/31 13:58:09, Mattias Nissler wrote:
> should be "doesn't point to a directory", no?

I didn't even see this; this diff is smaller, but the change was to split Load()
into LoadFromPath(), which was moved to after this function to keep the
declaration order.

I've fixed this comment anyway :-)

http://codereview.chromium.org/10443108/diff/7001/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.cc:52: if
(!file_util::GetFileInfo(config_file_path(), &file_info) ||
On 2012/05/31 13:58:09, Mattias Nissler wrote:
> You need to do that on both of the directories. Have you tested this?
> Continuously writing the policy file should stop Chrome from reloading. Until
> you don't touch the file for 5 seconds.

Watching both paths after rebasing on AsyncPolicyLoader.

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

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.cc:22: const char
kMandatoryConfigDir[] = "managed";
why not make these FILE_PATH_LITERAL?

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.cc:60: static const char* paths[]
= {
should be kPaths, or even better: kConfigDirSuffixes

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
File chrome/browser/policy/config_dir_policy_loader.h (right):

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.h:11: #include
"chrome/browser/policy/async_policy_provider.h"
can be forward-declared AFAICS

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.h:12: #include
"chrome/browser/policy/policy_constants.h"
forward-declare struct PolicyDefinitionList?

[Joao da Silva, 2012-06-05 09:16:28]

This has been rebased on the AsyncPolicyLoader without WatchPath(), and now has
2 FilePathWatchers.

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
File chrome/browser/policy/config_dir_policy_loader.cc (right):

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.cc:22: const char
kMandatoryConfigDir[] = "managed";
On 2012/06/04 09:36:36, Mattias Nissler wrote:
> why not make these FILE_PATH_LITERAL?

Done.

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.cc:60: static const char* paths[]
= {
On 2012/06/04 09:36:36, Mattias Nissler wrote:
> should be kPaths, or even better: kConfigDirSuffixes

Done.

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
File chrome/browser/policy/config_dir_policy_loader.h (right):

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.h:11: #include
"chrome/browser/policy/async_policy_provider.h"
On 2012/06/04 09:36:36, Mattias Nissler wrote:
> can be forward-declared AFAICS

Done.

http://codereview.chromium.org/10443108/diff/15012/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.h:12: #include
"chrome/browser/policy/policy_constants.h"
On 2012/06/04 09:36:36, Mattias Nissler wrote:
> forward-declare struct PolicyDefinitionList?

This is for PolicyScope and PolicyLevel.

[Mattias Nissler (ping if slow), 2012-06-05 12:00:16]

LGTM with a nit

http://codereview.chromium.org/10443108/diff/29001/chrome/browser/policy/conf...
File chrome/browser/policy/config_dir_policy_loader.h (right):

http://codereview.chromium.org/10443108/diff/29001/chrome/browser/policy/conf...
chrome/browser/policy/config_dir_policy_loader.h:26: // last value read takes
precedence in case of preference key collisions.
nit: s/preference key/policy key/

[Joao da Silva, 2012-06-05 14:51:39]

https://chromiumcodereview.appspot.com/10443108/diff/29001/chrome/browser/pol...
File chrome/browser/policy/config_dir_policy_loader.h (right):

https://chromiumcodereview.appspot.com/10443108/diff/29001/chrome/browser/pol...
chrome/browser/policy/config_dir_policy_loader.h:26: // last value read takes
precedence in case of preference key collisions.
On 2012/06/05 12:00:16, Mattias Nissler wrote:
> nit: s/preference key/policy key/

Done.

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

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

[commit-bot: I haz the power, 2012-06-05 17:35:48]

Change committed as 140549