Move user cloud policy to BrowserProcess (was 6979011)

Move user cloud policy to BrowserProcess (was 6979011)

This CL basically does the following things:
* Remove final dependencies of ProfilePolicyConnector on Profile
* Take ProfilePolicyConnector away from Profile and put it into BrowserPolicyConnector.
* Make BrowserPolicyConnector instance persistant and allow to
exchange the CloudPolicySubsystem backend instead.
* Introduce a new CloudPolicyProvider which combines two
(or more) CloudPolicyCacheBase backends and applies their policies in a well defined order.
That is the order in which the CloudPolicyCacheBases appear in
the List of CloudPolicyProvider determines the precedence,
early Providers get applied first and block the applied policies
for later Caches.
* Handles ProxyPolicies in CloudPolicyProvider making
the old infrastructure around MergingPolicyConnector obsolete.
* Consequently this means that user cloud policy can now serve local_state.

Continuation of http://codereview.chromium.org/6979011/
Patch from sfeuz@chromium.org with additional work from gfeher@chromium.org

BUG=none
TEST=Unittests CloudPolicyProvider.*

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

[gfeher, 2011-06-14 12:52:30]

The first patch set is just a copy of the last patch set in
http://codereview.chromium.org/6979011/
Development will continue here.

[gfeher, 2011-06-20 15:36:49]

This is ready for review now! (Passes tests and my manual checks on ChromeOS.)

Notes:
- The sync hack for desktop cloud policies won't be supported after this CL.
This is mainly because the idea behind this change is to remove the coupling
between the user that is logged into a profile and the user that is getting user
policies.
- I had to add an ugly hack in cloud_policy_provider_impl.cc:36. Would you still
LGTM if I am really-really sorry?

[Mattias Nissler (ping if slow), 2011-06-21 20:09:56]

woohoo, I made it through!

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/background_view.cc:362:
device_cloud_policy_subsystem();
not sure, but I think we usually indent 4 more spaces in this case.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:65:
g_browser_process->browser_policy_connector()->DeviceStopAutoRetry();
It seems like all these DeviceXXX accessors should rather live in something
device-policy specific that you can access through BrowserPolicyConnector.
However, I'm also fine with keeping and start the refactoring we discussed.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:250: profile->GetTokenService());
Ah, so this is the reason why the profile creation cannot wait for the
initialization of the cloud policy provider?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:47: // The directory suffixes
for user cloud policy.
what is a directory suffix?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:241: if (token_service_) {
Why do we need to null-check this? The whole thing won't work at all when we
don't have TokenService?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.cc:41: NOTREACHED();
I suggest putting the NOTREACHED() below the switch and removing the default
branch in this situation. That way, adding a new enum constant will trigger a
compiler warning instead of only failing at runtime.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller_unittest.cc:98:
recommended_cloud_provider_->AppendCache(cache_.get());
As mentioned earlier, I think we should just remove the providers here and check
the PolicyMap directly, since the subject of this test is CloudPolicController
and not CloudPolicyProvider.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:11: :
ConfigurationPolicyProvider(policy_list) {
4 spaces less indentation, please.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:8: 
#include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:39: // initialized.
I must say I don't particularly like this. I think we should break the cycle at
a different point, i.e. triggering the policy fetch earlier. I'm OK though to
fix this in a follow-up CL.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:87: kPolicyProxyBypassList
};
closing brace goes on next line.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:92: for (unsigned int i = 0;
i < n; ++i)
need curlies

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:27: //   AppendCache(cache): 
adds |cache| to the back (i.e. least important cache).
It doesn't make sense to duplicate this comment.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:49: // automatically remove
it from |caches_|.
don't need duplicate comments, but please put an override comment.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:54: // automatically remove
it from |caches_|.
same here.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:60: // Detecting
proxy-policies since they need a special handling when combining
need a special -> need special

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:69: void
CombineTwoPolicyMaps(const PolicyMap& base,
can this be static?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:24: //
CloudPolicyCacheBase Implementation.
lower-case: implementation

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:49: 
private:

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:158: // Verify the test
scenario of MultipleCaches
period.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:159: void
VerifyMultipleCaches(MockCloudPolicyCache caches[],
I don't like this verification code. It's hard to understand, so it's hard to
make sure it actually tests the right thing. I think we should rather have
simple-to-understand specific tests. In particular, can't we just fill a few
caches with test data and check the specific result?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:203:
scoped_ptr<ConfigurationPolicyProvider::PolicyDefinitionList> simple_list_;
Why is this so complicated? Can't we just declare a suitable static list similar
to how it's done in configuration_policy_pref_store.cc?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:226:
scoped_array<MockCloudPolicyCache> caches(new MockCloudPolicyCache[n]);
Just put 5 instead of n and delete all the cruft above :)

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:270: for (current =
list->begin; current != list->end; ++current) {
let's just write down what exact expectation we have instead of looping and
conditionals, which is not exactly simple to understand.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:306: // Prepare
|cache[i]| to serve the first i policies from |list|.
Again, I find this too complicated. Why not just create 3 caches, add set a few
policies explicitly, merge and examine the resulting policy map?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:350:
TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsPartial) {
What additional coverage does this test give over MultipleCaches above?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:382:
A.Set(get_proxy_policy(0), Value::CreateIntegerValue(a_value));
Why not just use a static constant here?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:384:
B.Set(get_proxy_policy(i), Value::CreateIntegerValue(b_value));
and here, and unroll the loop?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:161:
provider.Provide(&store);
can't you just get the policy map from the cache and check that?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_identity_strategy.cc:47: bool load_initiated_;
I still don't particularly like these. The cache can perfectly handle multiple
load calls. I think we should rather have a flag in the IdentityStrategy that
tells us whether we're still waiting for the cache load or not.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_identity_strategy.h:71: // changed afterwards
period.

[Joao da Silva, 2011-06-22 12:51:30]

Just my 2 nits.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:95: void
InitializeUserPolicy(std::string& user_name,
Should |user_name| be const?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:11: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
Nit: header order

[gfeher, 2011-06-22 19:18:33]

Thanks for the great reviews. :-)
Up for another round.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/background_view.cc:362:
device_cloud_policy_subsystem();
Makes sense, the style guide doesn't say anything about it.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:65:
g_browser_process->browser_policy_connector()->DeviceStopAutoRetry();
I agree, I've add a TODO for now. See head-comment of BrowserPolicyConnector.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:250: profile->GetTokenService());
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> Ah, so this is the reason why the profile creation cannot wait for the
> initialization of the cloud policy provider?

Only one of the reasons, but a seemingly strong one. Other (not so strong)
reasons include that fetching user policy for the first time is delayed until
the take-your-photo screen which also relies on the profile.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:47: // The directory suffixes
for user cloud policy.
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> what is a directory suffix?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:241: if (token_service_) {
Okay. But I add a DCHECK because it's hard to debug the case when we are not
receiving any token notifications because we specified a NULL source for the
registrar.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:95: void
InitializeUserPolicy(std::string& user_name,
On 2011/06/22 12:51:30, joaodasilva wrote:
> Should |user_name| be const?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.cc:41: NOTREACHED();
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> I suggest putting the NOTREACHED() below the switch and removing the default
> branch in this situation. That way, adding a new enum constant will trigger a
> compiler warning instead of only failing at runtime.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_controller_unittest.cc:98:
recommended_cloud_provider_->AppendCache(cache_.get());
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> As mentioned earlier, I think we should just remove the providers here and
check
> the PolicyMap directly, since the subject of this test is CloudPolicController
> and not CloudPolicyProvider.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:11: :
ConfigurationPolicyProvider(policy_list) {
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> 4 spaces less indentation, please.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:8: 
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> #include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:39: // initialized.
Me neither.
We could trigger the policy fetch for the first time earlier for example. But
anyways, fixing this seems to be a bigger task so I just add a TODO for now.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:87: kPolicyProxyBypassList
};
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> closing brace goes on next line.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.cc:92: for (unsigned int i = 0;
i < n; ++i)
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> need curlies

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:27: //   AppendCache(cache): 
adds |cache| to the back (i.e. least important cache).
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> It doesn't make sense to duplicate this comment.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:49: // automatically remove
it from |caches_|.
It already has an OVERRIDE macro, isn't that enough?

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:54: // automatically remove
it from |caches_|.
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:60: // Detecting
proxy-policies since they need a special handling when combining
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> need a special -> need special

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_impl.h:69: void
CombineTwoPolicyMaps(const PolicyMap& base,
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> can this be static?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:24: //
CloudPolicyCacheBase Implementation.
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> lower-case: implementation

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:49: 
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> private:

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:158: // Verify the test
scenario of MultipleCaches
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> period.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:159: void
VerifyMultipleCaches(MockCloudPolicyCache caches[],
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> I don't like this verification code. It's hard to understand, so it's hard to
> make sure it actually tests the right thing. I think we should rather have
> simple-to-understand specific tests. In particular, can't we just fill a few
> caches with test data and check the specific result?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:203:
scoped_ptr<ConfigurationPolicyProvider::PolicyDefinitionList> simple_list_;
We can go even deeper...

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:226:
scoped_array<MockCloudPolicyCache> caches(new MockCloudPolicyCache[n]);
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> Just put 5 instead of n and delete all the cruft above :)

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:270: for (current =
list->begin; current != list->end; ++current) {
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> let's just write down what exact expectation we have instead of looping and
> conditionals, which is not exactly simple to understand.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:306: // Prepare
|cache[i]| to serve the first i policies from |list|.
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> Again, I find this too complicated. Why not just create 3 caches, add set a
few
> policies explicitly, merge and examine the resulting policy map?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:350:
TEST_F(CloudPolicyProviderTest, CombineTwoPolicyMapsPartial) {
If this doesn't fail but MultipleCaches does, then we can know that
CloudPolicyProviderImpl::CombineTwoPolicyMaps works correctly but there is some
problem elsewhere in CloudPolicyProviderImpl/CloudPolicyCacheBase.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:382:
A.Set(get_proxy_policy(0), Value::CreateIntegerValue(a_value));
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> Why not just use a static constant here?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:384:
B.Set(get_proxy_policy(i), Value::CreateIntegerValue(b_value));
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> and here, and unroll the loop?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:11: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
On 2011/06/22 12:51:30, joaodasilva wrote:
> Nit: header order

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:161:
provider.Provide(&store);
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> can't you just get the policy map from the cache and check that?

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_identity_strategy.cc:47: bool load_initiated_;
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> I still don't particularly like these. The cache can perfectly handle multiple
> load calls. I think we should rather have a flag in the IdentityStrategy that
> tells us whether we're still waiting for the cache load or not.

Done.

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_identity_strategy.h (right):

http://codereview.chromium.org/7147015/diff/4050/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_identity_strategy.h:71: // changed afterwards
On 2011/06/21 20:09:56, Mattias Nissler wrote:
> period.

Done.

[Mattias Nissler (ping if slow), 2011-06-24 09:16:46]

Getting closer. Rip the loops out of the unit test and I'll be mostly happy :)

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:269: if (token_service_) {
Why the null check? If you remove this, you can also remove the DCHECK you
added, because that'd be redundant.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
Are these includes still needed?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.cc:41: // a proper
initialization completion check here.
As discussed offline: Let's do the local initialization, before returning true.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.h:46: static unsigned int
proxy_policy_count();
do you still need this?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:78: void
ExpectStringPolicy(const std::string& expected,
Again, can't we just check the policy map instead of requiring a store?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:162: "Please update
CloudPolicyProviderTest with the new proxy policies.";
Meh! What is this test good for? Just remove.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:217: for (int i = 0; i <
simple_list_length(); ++i) {
Still using loops? Can't we just get rid of the loop and set two or three
policies explicitly? This would also allow us to get rid of the simple_list
stuff.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:236: for (int i = 0; i <
simple_list_length(); ++i) {
Still using loops?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:13: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
are these includes needed?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/dummy...
File chrome/browser/policy/dummy_cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:12: 
Please include a class comment, saying what this is good for.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:96: cache_loaded_ = true;
You should also put a check bails out if cache_loaded_ is already true, in which
case we don't want to clobber device_token_ and device_id_

[gfeher, 2011-06-24 15:32:43]

No more loops as requested. :-)
Up for another round. Note, that I haven't run this in a Chrome OS image yet.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:269: if (token_service_) {
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Why the null check? If you remove this, you can also remove the DCHECK you
> added, because that'd be redundant.

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:10: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Are these includes still needed?

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.cc:41: // a proper
initialization completion check here.
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> As discussed offline: Let's do the local initialization, before returning
true.

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.h:46: static unsigned int
proxy_policy_count();
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> do you still need this?

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:78: void
ExpectStringPolicy(const std::string& expected,
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Again, can't we just check the policy map instead of requiring a store?

We can, then I would have to extract the private member combined_ from the
provider. I guess it gives slightly better coverage this way. WDYT?

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:162: "Please update
CloudPolicyProviderTest with the new proxy policies.";
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Meh! What is this test good for? Just remove.

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:217: for (int i = 0; i <
simple_list_length(); ++i) {
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Still using loops? Can't we just get rid of the loop and set two or three
> policies explicitly? This would also allow us to get rid of the simple_list
> stuff.

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:236: for (int i = 0; i <
simple_list_length(); ++i) {
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Still using loops?

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem_unittest.cc:13: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> are these includes needed?

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/dummy...
File chrome/browser/policy/dummy_cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:12: 
On 2011/06/24 09:16:46, Mattias Nissler wrote:
> Please include a class comment, saying what this is good for.

Done.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:96: cache_loaded_ = true;
I don't understand this. Would you like a line like:
if (cache_loaded_) return;

But why, is it possible that we get a second OnTokenCacheLoaded event with empty
or bogus token and deviceid?

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:73: for (int i = 0; i <
n; i++) {
Can I keep this loop? :-)

[Mattias Nissler (ping if slow), 2011-06-24 17:27:17]

I like the way you have simplified the rocket science unit test to something
easily understandable :)

The only remaining concern I have is the IsReady() vs.
initialization_completed() part, please reconsider whether you really need both
or whether it's possible to just repurpose initialization_completed() to what
you need.

Other than that, I'm happy if you commit this once one of the other reviewers
makes a final pass and gives their thumbs up. Thanks for your patience!

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:78: void
ExpectStringPolicy(const std::string& expected,
On 2011/06/24 15:32:44, gfeher wrote:
> On 2011/06/24 09:16:46, Mattias Nissler wrote:
> > Again, can't we just check the policy map instead of requiring a store?
> 
> We can, then I would have to extract the private member combined_ from the
> provider. I guess it gives slightly better coverage this way. WDYT?

Actually you are right in not doing it, I had mixed this up with a different
test.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:96: cache_loaded_ = true;
On 2011/06/24 15:32:44, gfeher wrote:
> I don't understand this. Would you like a line like:
> if (cache_loaded_) return;

Yep.

> 
> But why, is it possible that we get a second OnTokenCacheLoaded event with
empty
> or bogus token and deviceid?

Just in case, I always like to make code robust in case stuff goes wrong
elsewhere (i.e. somebody calling Load() twice).

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:235: DCHECK(token_service);
newline before comment.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:97: // Takes ownership
of |backend|.
This comment is outdated.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:73: for (int i = 0; i <
n; i++) {
On 2011/06/24 15:32:44, gfeher wrote:
> Can I keep this loop? :-)

I'd even inline this in the test below, since n is a constant 5. Your call
though.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:184: bool
DevicePolicyCache::IsReady() {
Hm, now we have IsReady() and initialization_complete(), of which we effectively
use only one, correct? If so, I'd prefer to change the semantics of
initialization_complete() as required.

[Mattias Nissler (ping if slow), 2011-06-24 17:27:31]

I like the way you have simplified the rocket science unit test to something
easily understandable :)

The only remaining concern I have is the IsReady() vs.
initialization_completed() part, please reconsider whether you really need both
or whether it's possible to just repurpose initialization_completed() to what
you need.

Other than that, I'm happy if you commit this once one of the other reviewers
makes a final pass and gives their thumbs up. Thanks for your patience!

[gfeher, 2011-06-27 16:50:24]

Addressed/answered comments.

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/25001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:96: cache_loaded_ = true;
On 2011/06/24 17:27:17, Mattias Nissler wrote:
> On 2011/06/24 15:32:44, gfeher wrote:
> > I don't understand this. Would you like a line like:
> > if (cache_loaded_) return;
> 
> Yep.
> 
> > 
> > But why, is it possible that we get a second OnTokenCacheLoaded event with
> empty
> > or bogus token and deviceid?
> 
> Just in case, I always like to make code robust in case stuff goes wrong
> elsewhere (i.e. somebody calling Load() twice).

Done.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:235: DCHECK(token_service);
On 2011/06/24 17:27:17, Mattias Nissler wrote:
> newline before comment.

Done.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:97: // Takes ownership
of |backend|.
On 2011/06/24 17:27:17, Mattias Nissler wrote:
> This comment is outdated.

Done.

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/34005/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:184: bool
DevicePolicyCache::IsReady() {
They actually do mean two different things:
initialization_complete(): we've got a usable policy response either from the
cache/disk storage or from the network
IsReady(): initialization_complete() OR the local disk storage was checked (but
it may have been empty or otherwise corrupted)

initialization_complete() is used for several things:
- in user_policy_cache.cc: avoiding overriding the fresh policy from network
with an old policy from the cache
- in cloud_policy_cache_base.h: determining if the cache has policy or not

I'll make the method protected and improve comments.

[Joao da Silva, 2011-06-29 11:36:16]

LGTM! The comments are mostly cleanups.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:9: #include
"chrome/browser/browser_process.h"
Nit: browser_process.h not needed

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:39: #include
"chrome/browser/policy/enterprise_install_attributes.h"
Nit: enterprise_install_attributes.h included in browser_policy_connector.h

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:304:
device_identity_strategy_.reset();
These resets were already done 5 lines above.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:12: #include "base/file_util.h"
Nit: not needed here, FilePath can be forward declared instead.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:19: class PrefService;
Nit: not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:121: const NotificationDetails&
details);
Nit: OVERRIDE

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.cc:11: #include
"chrome/browser/policy/policy_notifier.h"
Nit: policy_notifier.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.h:10: #include
"base/memory/scoped_vector.h"
Nit: scoped_ptr.h and scoped_vector.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:67: void
RunCachesThroughProvider(MockCloudPolicyCache caches[], int n,
Nit: maybe use a vector for |caches|, but this is also fine.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:85: EXPECT_TRUE(value !=
NULL);
Nit: the next line will segfault when this test fails, so maybe make it
ASSERT_TRUE?

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:94: EXPECT_TRUE(value !=
NULL);
Nit: same as above.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:208: EXPECT_TRUE(NULL !=
value);
Nit: same as above, also 2 more times below.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:15: #include
"chrome/browser/policy/configuration_policy_provider.h"
Nit: configuration_policy_provider.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:10: #include
"chrome/browser/prefs/pref_member.h"
Nit: pref_member.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:22: class
ConfigurationPolicyProvider;
Nit: ConfigurationPolicyProvider not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:10: #include
"base/compiler_specific.h"
Nit: compiler_specific.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:14: #include
"chrome/browser/chromeos/login/signed_settings_helper.h"
Nit: signed_settings_helper.h already included from device_policy_cache.h.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
File chrome/browser/policy/dummy_cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:24: virtual bool
Provide(ConfigurationPolicyStoreInterface* store);
Nit: OVERRIDE

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:29: virtual void
RemoveObserver(ConfigurationPolicyProvider::Observer* observer);
Nit: OVERRIDE

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache.cc:16: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
Nit: configuration_policy_pref_store.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:11: #include
"content/browser/browser_thread.h"
Nit: gaia_constants.h and browser_thread.h not needed.

http://codereview.chromium.org/7147015/diff/37001/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:14: #include
"chrome/browser/policy/dummy_configuration_policy_provider.h"
What are these includes for? They don't seem necessary.

[gfeher, 2011-06-29 12:53:07]

Thanks for the comments! Let's see if the trybots still like it.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:9: #include
"chrome/browser/browser_process.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: browser_process.h not needed

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:39: #include
"chrome/browser/policy/enterprise_install_attributes.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: enterprise_install_attributes.h included in browser_policy_connector.h

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:304:
device_identity_strategy_.reset();
On 2011/06/29 11:36:17, joaodasilva wrote:
> These resets were already done 5 lines above.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:12: #include "base/file_util.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: not needed here, FilePath can be forward declared instead.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:19: class PrefService;
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:121: const NotificationDetails&
details);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: OVERRIDE

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.cc:11: #include
"chrome/browser/policy/policy_notifier.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: policy_notifier.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_impl.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_impl.h:10: #include
"base/memory/scoped_vector.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: scoped_ptr.h and scoped_vector.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:67: void
RunCachesThroughProvider(MockCloudPolicyCache caches[], int n,
I'll leave it like this, mostly because I am lazy, but also because the only win
would be range-checking in the below for-loop.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:85: EXPECT_TRUE(value !=
NULL);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: the next line will segfault when this test fails, so maybe make it
> ASSERT_TRUE?

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:94: EXPECT_TRUE(value !=
NULL);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: same as above.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:208: EXPECT_TRUE(NULL !=
value);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: same as above, also 2 more times below.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:15: #include
"chrome/browser/policy/configuration_policy_provider.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: configuration_policy_provider.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:10: #include
"chrome/browser/prefs/pref_member.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: pref_member.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:22: class
ConfigurationPolicyProvider;
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: ConfigurationPolicyProvider not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_store_interface.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_store_interface.h:10: #include
"base/compiler_specific.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: compiler_specific.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:14: #include
"chrome/browser/chromeos/login/signed_settings_helper.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: signed_settings_helper.h already included from device_policy_cache.h.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
File chrome/browser/policy/dummy_cloud_policy_provider.h (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:24: virtual bool
Provide(ConfigurationPolicyStoreInterface* store);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: OVERRIDE

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/dummy...
chrome/browser/policy/dummy_cloud_policy_provider.h:29: virtual void
RemoveObserver(ConfigurationPolicyProvider::Observer* observer);
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: OVERRIDE

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache.cc:16: #include
"chrome/browser/policy/configuration_policy_pref_store.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: configuration_policy_pref_store.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_identity_strategy.cc:11: #include
"content/browser/browser_thread.h"
On 2011/06/29 11:36:17, joaodasilva wrote:
> Nit: gaia_constants.h and browser_thread.h not needed.

Done.

http://codereview.chromium.org/7147015/diff/37001/chrome/test/testing_browser...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/7147015/diff/37001/chrome/test/testing_browser...
chrome/test/testing_browser_process.cc:14: #include
"chrome/browser/policy/dummy_configuration_policy_provider.h"
Good question. Let's see if I can remove them.

[gfeher, 2011-06-29 13:00:24]

Hi Miranda,
Please owner-approve the change to profile_impl.cc in this CL. It's the result
of moving the cloud policy subsystem out of profile and into the browser
process.
Thanks,
Gábor

[Miranda Callahan, 2011-06-29 13:08:07]

On 2011/06/29 13:00:24, gfeher wrote:
> Hi Miranda,
> Please owner-approve the change to profile_impl.cc in this CL. It's the result
> of moving the cloud policy subsystem out of profile and into the browser
> process.
> Thanks,
> Gábor

I'll gladly approve removing code from profiles -- LGTM! :-)

[sfeuz, 2011-07-02 12:26:42]

very happy to see this being committed :-)