Move user cloud policy to BrowserProcess.

Move user cloud policy to BrowserProcess.

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.

BUG=none
TEST=Unittests CloudPolicyProvider.*

[sfeuz, 2011-05-25 14:56:08]

It is still work in progress but wanted to give you a chance to take a look at
it already.

@mnissler/gfeher: Are there any big changes you do not agree with.
@joaodasilva: Solves the Merge Proxy settings from DevicePolicy into UserPolicy
differently. What do you think?

[Mattias Nissler (ping if slow), 2011-05-26 10:20:19]

did a very rough pass, but I guess it's good enough to keep you busy ;)

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process....
chrome/browser/browser_process.h:69: class ProfilePolicyConnector;
Please also give it a better name. Maybe UserPolicyConnector?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process....
chrome/browser/browser_process.h:165: managed_cloud_policy_provider() = 0;
We should wrap all the cloud-policy related stuff in one object in order to not
spam the browser_process namespace.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:46: #include
"chrome/browser/policy/profile_policy_connector.h"
alphabetize

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:558: // Combining UserPolicy and
DevicePolicy.
The spelling of "UserPolicy" and "DevicePolicy" suggests they are type names.
Let's just write "user policy" and "device policy".

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:561:
policy::ConfigurationPolicyPrefStore::
Indentation

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:571: if (device_policy)
Need curlies for multi-line conditional blocks

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:573:
AddCloudPolicyProvider(device_policy);
Indentation

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.h:240:
scoped_ptr<policy::CombiningCloudPolicyProvider>
Any reason why we need the specific CombiningCloudPolicyProvider type here?
ConfigurationPolicyProvider should be good enough, no?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:236: 
excess newline

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:249:
DCHECK(profile_policy_connector);
No need for the DCHECK, we crash anyway below.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:250:
profile_policy_connector->Initialize(username_,
Once we have the policy wrapper object, we should probably just call a
to-be-implemented g_browser_process->InitializeUserPolicy(username_, profile)
here.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:58:
install_attributes_.get());
does this even compile?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:72: void
AddObserver(ConfigurationPolicyProvider::Observer* observer);
Reusing that observer interface seems odd, since you're observing a cache, but
not a provider.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:81: }
newline before new visibility label

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:147: // Provider observers that
are registered with this provider.
CloudPolicyCacheBase is not a provider.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:158: class CloudPolicyProvider :
public ConfigurationPolicyProvider,
Seems like this should rather go into its own file.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:171: //
ConfigurationPolicyProvier::Observer implementation.
Provier -> Provider

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:179: void
set_cache(CloudPolicyCacheBase* cache);
newline before visibility label

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:193: class
CombiningCloudPolicyProvider : public ConfigurationPolicyProvider {
It seems like this layering is overkill. Why not just have a single
CloudPolicyProvider with which you can register different caches that will be
accessed directly?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:206: bool
Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;
newline before comment

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:219: void
AddCloudPolicyProvider(CloudPolicyProvider* cloud_policy_provider);
newline before visibility label

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:258: }
newline

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:261: CloudPolicyProvider*
cloud_policy_provider_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:274: 
excess newline

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.h:52: static
ConfigurationPolicyPrefStore* CreateManagedCloudPolicyPrefStore(
Same change as below also here?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:37: bool load_finished();
what do you need these for?

[sfeuz, 2011-05-31 07:32:30]

up for a new round.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process....
chrome/browser/browser_process.h:69: class ProfilePolicyConnector;
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Please also give it a better name. Maybe UserPolicyConnector?

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process....
chrome/browser/browser_process.h:165: managed_cloud_policy_provider() = 0;
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> We should wrap all the cloud-policy related stuff in one object in order to
not
> spam the browser_process namespace.

I see.
On the other hand: the cloud policy class-system is already quite complicated
and adding another player will not make it easier.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:46: #include
"chrome/browser/policy/profile_policy_connector.h"
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:558: // Combining UserPolicy and
DevicePolicy.
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> The spelling of "UserPolicy" and "DevicePolicy" suggests they are type names.
> Let's just write "user policy" and "device policy".

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:561:
policy::ConfigurationPolicyPrefStore::
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Indentation

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:571: if (device_policy)
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Need curlies for multi-line conditional blocks

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:573:
AddCloudPolicyProvider(device_policy);
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Indentation

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.h:240:
scoped_ptr<policy::CombiningCloudPolicyProvider>
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Any reason why we need the specific CombiningCloudPolicyProvider type here?
> ConfigurationPolicyProvider should be good enough, no?

The way this is accessed from the outside is through the getter
managed_cloud_policy_provider() which returns it as a 
ConfigurationPolicyProvider.
I dont see a reason to also store it as a ConfigurationPolicyProvider.
Am I missing an obvious drawback of this method?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:236: 
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> excess newline

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/chromeos/login/l...
chrome/browser/chromeos/login/login_utils.cc:249:
DCHECK(profile_policy_connector);
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> No need for the DCHECK, we crash anyway below.

Is that really a reason to remove this DCHECK?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:58:
install_attributes_.get());
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> does this even compile?

It does now.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:72: void
AddObserver(ConfigurationPolicyProvider::Observer* observer);
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Reusing that observer interface seems odd, since you're observing a cache, but
> not a provider.

Created a seperate interface for CloudPolicyCacheBase.
Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:81: }
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> newline before new visibility label

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:147: // Provider observers that
are registered with this provider.
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> CloudPolicyCacheBase is not a provider.

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:158: class CloudPolicyProvider :
public ConfigurationPolicyProvider,
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Seems like this should rather go into its own file.

I agree, split the CloudPolicyProvider+CombiningCloudPolicyProvider into one
file. They somehow belong together but maybe we should also split those two?

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:171: //
ConfigurationPolicyProvier::Observer implementation.
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Provier -> Provider

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:179: void
set_cache(CloudPolicyCacheBase* cache);
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> newline before visibility label

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:193: class
CombiningCloudPolicyProvider : public ConfigurationPolicyProvider {
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> It seems like this layering is overkill. Why not just have a single
> CloudPolicyProvider with which you can register different caches that will be
> accessed directly?

Note that CloudPolicyProvider also keeps track of whether to use the Managed or
Recommended map.
Removing this layer would mean that we have to track this in the current
CombiningCloudPolicyProvider. I.e. either having a list of pairs of
<PolicyLevel, CloudPolicyCacheBase*> or fixing the PolicyLevel per
CombiningCloudPolicyProvider (and not be able to combine managed- and
recommended policies.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:206: bool
Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> newline before comment

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:219: void
AddCloudPolicyProvider(CloudPolicyProvider* cloud_policy_provider);
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> newline before visibility label

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:258: }
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:261: CloudPolicyProvider*
cloud_policy_provider_;
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:274: 
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> excess newline

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.h:52: static
ConfigurationPolicyPrefStore* CreateManagedCloudPolicyPrefStore(
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> Same change as below also here?

Done.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:37: bool load_finished();
On 2011/05/26 10:20:20, Mattias Nissler wrote:
> what do you need these for?

Currently there is the following potential race condition between TokenCache and
DeviceTokenFetching:
* TokenService notifies about a successful token. CheckAndTrigger fetch issues a
fetch with some random device_id.
* While the request is out in the wild TokenCache finishes loading successfully
and sets the device_id and dm_token.
* DeviceTokenFetcher returns from the request and sets the new token from the
cloud (but not the device_id).
=> device_id/token pair is invalid.

We should wait until TokenCache has finished loading before starting to get the
token via DeviceTokenFetcher.

[Mattias Nissler (ping if slow), 2011-05-31 14:14:19]

Another round of comments. I guess we should sit down and talk about policy
providers and caches and how to merge stuff at some point, writing review
comments feels tedious ;)

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process....
chrome/browser/browser_process.h:165: managed_cloud_policy_provider() = 0;
On 2011/05/31 07:32:31, sfeuz wrote:
> On 2011/05/26 10:20:20, Mattias Nissler wrote:
> > We should wrap all the cloud-policy related stuff in one object in order to
> not
> > spam the browser_process namespace.
> 
> I see.
> On the other hand: the cloud policy class-system is already quite complicated
> and adding another player will not make it easier.

It's not another player, but just a wrapper. Move everything you have now in
BrowserProcess into some new class and keep only a single pointer here.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.h:240:
scoped_ptr<policy::CombiningCloudPolicyProvider>
On 2011/05/31 07:32:31, sfeuz wrote:
> On 2011/05/26 10:20:20, Mattias Nissler wrote:
> > Any reason why we need the specific CombiningCloudPolicyProvider type here?
> > ConfigurationPolicyProvider should be good enough, no?
> 
> The way this is accessed from the outside is through the getter
> managed_cloud_policy_provider() which returns it as a 
> ConfigurationPolicyProvider.
> I dont see a reason to also store it as a ConfigurationPolicyProvider.
> Am I missing an obvious drawback of this method?

Technically, there is not difference. My point is just that on this level we
shouldn't really need to know what exact CloudPolicyProvider implementation we
have, in particular if this all lives in BrowserProcessImpl (but that's the
other discussion point).

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache_base.h:193: class
CombiningCloudPolicyProvider : public ConfigurationPolicyProvider {
On 2011/05/31 07:32:31, sfeuz wrote:
> On 2011/05/26 10:20:20, Mattias Nissler wrote:
> > It seems like this layering is overkill. Why not just have a single
> > CloudPolicyProvider with which you can register different caches that will
be
> > accessed directly?
> 
> Note that CloudPolicyProvider also keeps track of whether to use the Managed
or
> Recommended map.
> Removing this layer would mean that we have to track this in the current
> CombiningCloudPolicyProvider. I.e. either having a list of pairs of
> <PolicyLevel, CloudPolicyCacheBase*> or fixing the PolicyLevel per
> CombiningCloudPolicyProvider (and not be able to combine managed- and
> recommended policies.

Why would we want to combine recommended and managed in the first place? That
would be wrong anyhow.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:37: bool load_finished();
On 2011/05/31 07:32:31, sfeuz wrote:
> On 2011/05/26 10:20:20, Mattias Nissler wrote:
> > what do you need these for?
> 
> Currently there is the following potential race condition between TokenCache
and
> DeviceTokenFetching:
> * TokenService notifies about a successful token. CheckAndTrigger fetch issues
a
> fetch with some random device_id.
> * While the request is out in the wild TokenCache finishes loading
successfully
> and sets the device_id and dm_token.
> * DeviceTokenFetcher returns from the request and sets the new token from the
> cloud (but not the device_id).
> => device_id/token pair is invalid.
> 
> We should wait until TokenCache has finished loading before starting to get
the
> token via DeviceTokenFetcher.

I see and agree we should handle that condition gracefully.

So if we keep the accessors, I suggest you just inline the implementation (your
call though). Also, it looks like load_initiate() is not called from outside of
the class, so  you could get rid of the accessor entirely.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/prefs/pref_servi...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/prefs/pref_servi...
chrome/browser/prefs/pref_service.cc:135:
ConfigurationPolicyPrefStore::CreateManagedCloudPolicyPrefStore(profile);
Why does this still need the profile?

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:246: 
excess whitespace

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:39: CloudPolicyProvider*
GetManagedCloudProvider() const;
You shouldn't need to now that these are CloudPolicyProviders. I know that
currently you rely on this since the user policy stuff goes through this
accessor to get to the device policy maps. But that seems wrong too... Can't we
just give the user policy providers access directly to the
BrowserPolicyConnector's cache?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:9: #include
"base/gtest_prod_util.h"
Don't need this?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:10: #include
"base/memory/scoped_ptr.h"
nor this?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:15: #include
"chrome/browser/policy/configuration_policy_provider.h"
nor this?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:22: class CloudPolicyProvider;
Do you actually need that include?

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:139: }
6 levels of nesting? Compared to the existing merging code, this is super-hard
to follow. How about implementing a Merge() function that takes two policy maps
and takes care of handling the necessary grouping?

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:22: // destroying the
CloudPolicyProvider instance.
destroying is not exactly exchanging the |cache_| pointer, is it? Also, the
set_cache() call below suggests that you can replace the backend without
destroying anything.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:43: PolicyMap* policy_map();
It feels wrong that you both expose the policy map and still implement the
ConfigurationPolicyProvider interface, which are both ways of getting at the
same information. To me, it seems like this is an indicator of the layering
becoming convoluted where there's actually no reason for that.

Here is my suggestion: Have CloudPolicyCacheBase implement whatever interface
you need to get to the actual policy data. Implement a cloud policy provider
that does the necessary merging and accesses whatever set of CloudPolicyCaches
it needs to talk to.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:73: // Returns true if we could
apply at least one policy.
That seems wrong. It's totally OK for no policy to existing, why would we want
to return false then?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:74: bool
Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;
virtual

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:78: bool
IsInitializationComplete() const OVERRIDE;
virtual

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:79: void
AddObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;
virtual

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:80: void
RemoveObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;
virtual

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:94: // OnProviderGoingAway.
It seems like a adding an appropriate parameter to the
CloudPolicyProvider::Observer interface would be not to hard and achieve the
same thing, no?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:138: //
CombiningCoudPolicyProvider. Order dependant.
"Order dependant" conveys to me: There is something important about the ordering
here. Unfortunately, you leave it to the reader to figure out what that is.
That's really inconvenient, given that it's easy for you to just spell it out :)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_connector.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:61: const FilePath& policy_dir,
indentation

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:84: new
UserPolicyCache(policy_cache_dir.Append(kPolicyCacheFile));
indentation

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:129: if (token_details->service()
== GaiaConstants::kDeviceManagementService)
You should also make sure that |source| corresponds to |token_service_|

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:133: } else if (type ==
NotificationType::LOGIN_USER_CHANGED) {
I don't understand why we require handling this notification at all. Just for
making the DCHECK?

[Joao da Silva, 2011-05-31 14:50:23]

Big CL, took a while to review! :-)
Mostly cosmetic changes here. We should discuss the bigger picture.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:544: if (!created_user_policy_connector_)
{
Why have a boolean for this test, instead of testing the scoped_ptr?
Something like:

if (!user_policy_connector_.get()) {
  user_policy_connector_.reset(policy::UserPolicyConnector::Create());
}

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:567:
managed_cloud_policy_provider_.get()->AddCloudPolicyProvider(user_policy);
No need for .get(), just -> from the scoped_ptr.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:572:
managed_cloud_policy_provider_.get()->
Same.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:594:
recommended_cloud_policy_provider_.get()->
Same.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:600:
recommended_cloud_policy_provider_.get()->
Same again :-)

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:17: #if defined(OS_WIN)
Why you no like blank line? :-)

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:91:
scoped_ptr<CloudPolicyProvider> recommended_cloud_provider_;
These are only used on ChromeOS, so how about wrapping with

#if defined(OS_CHROMEOS)?

The implementations of Get*CloudProvider() should then also test for
OS_CHROMEOS, and just return NULL on other platforms. Similar to the
implementation of GetEnterpriseDomain().

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:78: PolicyMap*
recommended_policy();
Shouldn't these be const?

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:31: cache_->AddObserver(this);
If this can replace a previous cache, shouldn't it remove |this| as an observer
of the previous cache too? I see OnCacheGoingAway() below; if it is expected
that cache_ is always NULL here, add a DCHECK(cache_ != NULL).

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:45: return cache_ &&
cache_->initialization_complete();
It would be nicer to have consistent naming for this method :-)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:67: PolicyMap*
CloudPolicyProvider::policy_map() {
Return const here?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:102:
(*i)->cloud_policy_provider()->IsInitializationComplete()) {
How about this to save one level of nesting and some typing:

CloudPolicyProviderWithObserver* provider = (*i)->cloud_policy_provider();
if (!provider || !provider->IsInitializationComplete())
  continue;

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:104: if (policy_map) {
Same:
if (!policy_map)
  continue;

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:105: const PolicyDefinitionList*
policy_list(policy_definition_list());
This can be moved outside the loop.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:109: if
(already_applied.find(j->policy_type)!=already_applied.end())
Nit: spaces around != ?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:139: }
We should discuss this code.

The original only merged the proxy settings, but this code will merge anything
that hasn't been set by the user policy. Other policies might also require
special handling (for example, DefaultSearchProvider*).

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:148: initialization_complete =
initialization_complete ||
This means that initialization is complete once *any* of the providers is
initialized. Is this what is intended? Or did you intend to return true when
*all* the providers are initialized?

In that case, you could just return false during the loop if one provider is not
initialized. If all are initialized and none returns from the loop, just return
true after the loop.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:43: PolicyMap* policy_map();
Should this be const?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:78: bool
IsInitializationComplete() const OVERRIDE;
I'm not sure this is the correct behavior, instead of returning true when *all*
the cloud providers are initialized. Any reason for this?

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:32:
DCHECK(reinterpret_cast<const Value*>(arg)->GetAsInteger(&value));
DCHECK will be removed on non-debug builds, and thus |value| won't be
initialized. Also, in a test do you want a DCHECK or an EXPECT?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:88: 
Nit: remove extra line?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:160: DCHECK(
proxy_related_policies == 5 );
Nit: extra spaces next to the parenthesis?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:163: for (unsigned int i
= 0; i<n ; ++i) {
Nit: i < n (spaces)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:194: DCHECK(result);
Why not EXPECT_TRUE?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:225: for (unsigned int i
= 0; i<n ; ++i) {
Nit: i < n (spaces)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:262: DCHECK(n -
arraysize(proxy_policies) < 1000);
EXPECT_TRUE?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:275: for (unsigned int j
= 0;j < arraysize(proxy_policies); ++j) {
Nit: space after j = 0;

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:320: for (unsigned int i
= 0; i<n ; ++i) {
Nit: i < n (spaces)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_subsystem.cc (left):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem.cc:74: DCHECK(!prefs_);
Why has this been removed? This is set to NULL on Shutdown(), which should be
called before the dtor.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface_unittest.cc
(right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface_unittest.cc:14: }  //
namespace policy
Just remove the whole file.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.cc:17: #include
"chrome/browser/policy/user_policy_connector.h"
Alphabetic order.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_connector.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:42: }
Why a static Create() instead of making the ctor public?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:123: const NotificationSource&
source,
Nit: indentation.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:12: #include
"chrome/browser/policy/user_policy_connector.h"
Nit: alphabetic order.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:120: GetChromePolicyDefinitionList();
Nit: indent.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:132: GetChromePolicyDefinitionList();
Nit: indent.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
File chrome/test/testing_browser_process.h (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.h:175: managed_cloud_policy_provider_;
Nit: indent?

[sfeuz, 2011-06-03 08:30:35]

ok new round.

two big changes (as discussed with mnissler offline) are:
* Combine UserPolicyConnector and BrowserPolicyConnector to
BrowserPolicyConnector. This makes quite some stuff easier and also reduces the
involvement of browser_process significantly.
* Remove a layering in CloudPolicyProvider and introduce a cached PolicyMap
which is a combination of underlying caches.

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
File chrome/browser/policy/user_policy_identity_strategy.cc (right):

http://codereview.chromium.org/6979011/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:37: bool load_finished();
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> On 2011/05/31 07:32:31, sfeuz wrote:
> > On 2011/05/26 10:20:20, Mattias Nissler wrote:
> > > what do you need these for?
> > 
> > Currently there is the following potential race condition between TokenCache
> and
> > DeviceTokenFetching:
> > * TokenService notifies about a successful token. CheckAndTrigger fetch
issues
> a
> > fetch with some random device_id.
> > * While the request is out in the wild TokenCache finishes loading
> successfully
> > and sets the device_id and dm_token.
> > * DeviceTokenFetcher returns from the request and sets the new token from
the
> > cloud (but not the device_id).
> > => device_id/token pair is invalid.
> > 
> > We should wait until TokenCache has finished loading before starting to get
> the
> > token via DeviceTokenFetcher.
> 
> I see and agree we should handle that condition gracefully.
> 
> So if we keep the accessors, I suggest you just inline the implementation
(your
> call though). Also, it looks like load_initiate() is not called from outside
of
> the class, so  you could get rid of the accessor entirely.

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:544: if (!created_user_policy_connector_)
{
On 2011/05/31 14:50:23, joaodasilva wrote:
> Why have a boolean for this test, instead of testing the scoped_ptr?
> Something like:
> 
> if (!user_policy_connector_.get()) {
>   user_policy_connector_.reset(policy::UserPolicyConnector::Create());
> }

Obsolete.
But still the same is done in browser_policy_connector. I actually dont know
exactly why...

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:567:
managed_cloud_policy_provider_.get()->AddCloudPolicyProvider(user_policy);
On 2011/05/31 14:50:23, joaodasilva wrote:
> No need for .get(), just -> from the scoped_ptr.

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:572:
managed_cloud_policy_provider_.get()->
On 2011/05/31 14:50:23, joaodasilva wrote:
> Same.
Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:594:
recommended_cloud_policy_provider_.get()->
On 2011/05/31 14:50:23, joaodasilva wrote:
> Same.
Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:600:
recommended_cloud_policy_provider_.get()->
On 2011/05/31 14:50:23, joaodasilva wrote:
> Same again :-)
Obsolete :-)

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/login_utils.cc:246: 
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> excess whitespace

Done.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.cc:17: #if defined(OS_WIN)
On 2011/05/31 14:50:23, joaodasilva wrote:
> Why you no like blank line? :-)

Done.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:39: CloudPolicyProvider*
GetManagedCloudProvider() const;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> You shouldn't need to now that these are CloudPolicyProviders. I know that
> currently you rely on this since the user policy stuff goes through this
> accessor to get to the device policy maps. But that seems wrong too... Can't
we
> just give the user policy providers access directly to the
> BrowserPolicyConnector's cache?

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/browse...
chrome/browser/policy/browser_policy_connector.h:91:
scoped_ptr<CloudPolicyProvider> recommended_cloud_provider_;
On 2011/05/31 14:50:23, joaodasilva wrote:
> These are only used on ChromeOS, so how about wrapping with
> 
> #if defined(OS_CHROMEOS)?
> 
> The implementations of Get*CloudProvider() should then also test for
> OS_CHROMEOS, and just return NULL on other platforms. Similar to the
> implementation of GetEnterpriseDomain().

Obsolete.
But I tried to put everything else which is device-only behind the guards.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:9: #include
"base/gtest_prod_util.h"
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> Don't need this?

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:10: #include
"base/memory/scoped_ptr.h"
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> nor this?

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:15: #include
"chrome/browser/policy/configuration_policy_provider.h"
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> nor this?

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:22: class CloudPolicyProvider;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> Do you actually need that include?
Nope. Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_cache_base.h:78: PolicyMap*
recommended_policy();
On 2011/05/31 14:50:23, joaodasilva wrote:
> Shouldn't these be const?

I agree. Done.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:31: cache_->AddObserver(this);
On 2011/05/31 14:50:23, joaodasilva wrote:
> If this can replace a previous cache, shouldn't it remove |this| as an
observer
> of the previous cache too? I see OnCacheGoingAway() below; if it is expected
> that cache_ is always NULL here, add a DCHECK(cache_ != NULL).

Good point.
However in the new design caches can't be removed externally but only go away on
OnCacheGoingAway. I think it is not necessary to add a RemoveCache-method.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:45: return cache_ &&
cache_->initialization_complete();
On 2011/05/31 14:50:23, joaodasilva wrote:
> It would be nicer to have consistent naming for this method :-)

I followed the convention that initialization_complete() is a simple getter for
|initialization_complete|. And IsInitializationComplete is given by
ConfigurationPolicyProvider.
If you feel strongly about it then I am happy to change it :-)

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:67: PolicyMap*
CloudPolicyProvider::policy_map() {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Return const here?

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:102:
(*i)->cloud_policy_provider()->IsInitializationComplete()) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> How about this to save one level of nesting and some typing:
> 
> CloudPolicyProviderWithObserver* provider = (*i)->cloud_policy_provider();
> if (!provider || !provider->IsInitializationComplete())
>   continue;

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:104: if (policy_map) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Same:
> if (!policy_map)
>   continue;

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:105: const PolicyDefinitionList*
policy_list(policy_definition_list());
On 2011/05/31 14:50:23, joaodasilva wrote:
> This can be moved outside the loop.

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:109: if
(already_applied.find(j->policy_type)!=already_applied.end())
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: spaces around != ?

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:139: }
On 2011/05/31 14:50:23, joaodasilva wrote:
> We should discuss this code.
> 
> The original only merged the proxy settings, but this code will merge anything
> that hasn't been set by the user policy. Other policies might also require
> special handling (for example, DefaultSearchProvider*).

I think we can not do much better than adding a special case for
DefaultSearchProvider.
Or maybe a generic Grouping-Mechanism to only apply them in groups but probably
overkill for 2 groups?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.cc:148: initialization_complete =
initialization_complete ||
On 2011/05/31 14:50:23, joaodasilva wrote:
> This means that initialization is complete once *any* of the providers is
> initialized. Is this what is intended? Or did you intend to return true when
> *all* the providers are initialized?
> 
> In that case, you could just return false during the loop if one provider is
not
> initialized. If all are initialized and none returns from the loop, just
return
> true after the loop.

See comments above about what to return here.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:22: // destroying the
CloudPolicyProvider instance.
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> destroying is not exactly exchanging the |cache_| pointer, is it? Also, the
> set_cache() call below suggests that you can replace the backend without
> destroying anything.

I meant to write "[...] at any point WITHOUT destroying the CloudPolicyProvider
instance."

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:43: PolicyMap* policy_map();
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> It feels wrong that you both expose the policy map and still implement the
> ConfigurationPolicyProvider interface, which are both ways of getting at the
> same information. To me, it seems like this is an indicator of the layering
> becoming convoluted where there's actually no reason for that.
> 
> Here is my suggestion: Have CloudPolicyCacheBase implement whatever interface
> you need to get to the actual policy data. Implement a cloud policy provider
> that does the necessary merging and accesses whatever set of CloudPolicyCaches
> it needs to talk to.

Done. As we discussed offline.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:73: // Returns true if we could
apply at least one policy.
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> That seems wrong. It's totally OK for no policy to existing, why would we want
> to return false then?

I agree. Just returning true is probably the right way to go. Does the same
apply for the initialization-query? I.e. should we always return true in
IsInitializationComplete()?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:74: bool
Provide(ConfigurationPolicyStoreInterface* store) OVERRIDE;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> virtual

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:78: bool
IsInitializationComplete() const OVERRIDE;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> virtual

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:78: bool
IsInitializationComplete() const OVERRIDE;
On 2011/05/31 14:50:23, joaodasilva wrote:
> I'm not sure this is the correct behavior, instead of returning true when
*all*
> the cloud providers are initialized. Any reason for this?

I now believe the correct behaviour is to always return true, with the reasoning
that the Provider as is is, indeed, initialized and ready to serve what is
there. Why should we wait for all caches? They will issue an update once they
are ready anyways.

@mnissler: WDYT?

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:79: void
AddObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> virtual

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:80: void
RemoveObserver(ConfigurationPolicyProvider::Observer* observer) OVERRIDE;
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> virtual

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:94: // OnProviderGoingAway.
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> It seems like a adding an appropriate parameter to the
> CloudPolicyProvider::Observer interface would be not to hard and achieve the
> same thing, no?

Added the parameter to the cache observer interface.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider.h:138: //
CombiningCoudPolicyProvider. Order dependant.
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> "Order dependant" conveys to me: There is something important about the
ordering
> here. Unfortunately, you leave it to the reader to figure out what that is.
> That's really inconvenient, given that it's easy for you to just spell it out
:)

Was explained in Provide.

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

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:32:
DCHECK(reinterpret_cast<const Value*>(arg)->GetAsInteger(&value));
On 2011/05/31 14:50:23, joaodasilva wrote:
> DCHECK will be removed on non-debug builds, and thus |value| won't be
> initialized. Also, in a test do you want a DCHECK or an EXPECT?

Done. Nice catch.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:88: 
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: remove extra line?

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:160: DCHECK(
proxy_related_policies == 5 );
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: extra spaces next to the parenthesis?

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:163: for (unsigned int i
= 0; i<n ; ++i) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: i < n (spaces)

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:194: DCHECK(result);
On 2011/05/31 14:50:23, joaodasilva wrote:
> Why not EXPECT_TRUE?

Done. I tried to make a distinction between test-related (EXPECT..) and just the
test-code (DCHECK). But I agree that EXPECT is better here.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:225: for (unsigned int i
= 0; i<n ; ++i) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: i < n (spaces)

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:262: DCHECK(n -
arraysize(proxy_policies) < 1000);
On 2011/05/31 14:50:23, joaodasilva wrote:
> EXPECT_TRUE?

It is a DCHECK because this actually checks if somebody messed with the test
parameters.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:275: for (unsigned int j
= 0;j < arraysize(proxy_policies); ++j) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: space after j = 0;

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_provider_unittest.cc:320: for (unsigned int i
= 0; i<n ; ++i) {
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: i < n (spaces)

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
File chrome/browser/policy/cloud_policy_subsystem.cc (left):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/cloud_...
chrome/browser/policy/cloud_policy_subsystem.cc:74: DCHECK(!prefs_);
On 2011/05/31 14:50:23, joaodasilva wrote:
> Why has this been removed? This is set to NULL on Shutdown(), which should be
> called before the dtor.

|prefs_| is only still here because of the refresh rate which will be taken care
of in another CL.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_store_interface_unittest.cc
(right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_store_interface_unittest.cc:14: }  //
namespace policy
On 2011/05/31 14:50:23, joaodasilva wrote:
> Just remove the whole file.

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_cache.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_cache.cc:17: #include
"chrome/browser/policy/user_policy_connector.h"
On 2011/05/31 14:50:23, joaodasilva wrote:
> Alphabetic order.

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
File chrome/browser/policy/user_policy_connector.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:42: }
On 2011/05/31 14:50:23, joaodasilva wrote:
> Why a static Create() instead of making the ctor public?

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:61: const FilePath& policy_dir,
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:84: new
UserPolicyCache(policy_cache_dir.Append(kPolicyCacheFile));
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:123: const NotificationSource&
source,
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:129: if (token_details->service()
== GaiaConstants::kDeviceManagementService)
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> You should also make sure that |source| corresponds to |token_service_|

Done.

http://codereview.chromium.org/6979011/diff/8001/chrome/browser/policy/user_p...
chrome/browser/policy/user_policy_connector.cc:133: } else if (type ==
NotificationType::LOGIN_USER_CHANGED) {
On 2011/05/31 14:14:19, Mattias Nissler wrote:
> I don't understand why we require handling this notification at all. Just for
> making the DCHECK?

Basically yes.
Also still debugging what happens if Chrome instance is killed and then
relaunched with the user directly logged in...

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
File chrome/test/testing_browser_process.cc (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:12: #include
"chrome/browser/policy/user_policy_connector.h"
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: alphabetic order.

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:120: GetChromePolicyDefinitionList();
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: indent.

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.cc:132: GetChromePolicyDefinitionList();
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: indent.

Obsolete.

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
File chrome/test/testing_browser_process.h (right):

http://codereview.chromium.org/6979011/diff/8001/chrome/test/testing_browser_...
chrome/test/testing_browser_process.h:175: managed_cloud_policy_provider_;
On 2011/05/31 14:50:23, joaodasilva wrote:
> Nit: indent?

Obsolete.

[Joao da Silva, 2011-06-03 12:23:51]

Looks good, nice cleanup.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:42: };
This is already forward-declared in browser_process.h, is it really needed here?

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:29: bool
is_proxy_policy(ConfigurationPolicyType policy) {
Place this in an unnamed namespace, so that the symbol isn't global.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:96:
caches_.insert(caches_.begin(), cache);
std::list has push_front(), so maybe caches_ should be a std::list. Its expected
size doesn't justify such a small change, so this is just a suggestion :-)

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:117: caches_.erase(i);
erase() is also cheaper on a std::list :-)

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

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:28: is_proxy = is_proxy
|| proxy_policies[i]==policy;
Why not just "return true" here, and "return false" if the loop exits?

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:87:
cloud_policy_provider_.get()->Provide(store);
The .get() isn't required, just cloud_policy_provider->Provide(..)

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:92:
cloud_policy_provider_.get()->AppendCache(cache);
Same.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:97:
cloud_policy_provider_.get()->PrependCache(cache);
Same.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:104:
cloud_policy_provider_.get()->CombineTwoPolicyMaps(base, overlay, out_map);
Same.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:134:
Value::CreateBooleanValue(true));
Indent (also on the lines below)

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:253: }
Indent.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_connector.h (left):

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.h:102: #endif  //
CHROME_BROWSER_POLICY_PROFILE_POLICY_CONNECTOR_H_
Yay, it's gone! :-)

[sfeuz, 2011-06-03 16:19:28]

thanks for the comments.
I did some more manual testing of proxy-policies on CrOS. Up to now everything
works fine (except for some UI problem with proxy-policies which seems somewhat
unrelated).

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:42: };
On 2011/06/03 12:23:51, joaodasilva wrote:
> This is already forward-declared in browser_process.h, is it really needed
here?

I think we should forward declare again here according to style guide saying.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:29: bool
is_proxy_policy(ConfigurationPolicyType policy) {
On 2011/06/03 12:23:51, joaodasilva wrote:
> Place this in an unnamed namespace, so that the symbol isn't global.

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:96:
caches_.insert(caches_.begin(), cache);
On 2011/06/03 12:23:51, joaodasilva wrote:
> std::list has push_front(), so maybe caches_ should be a std::list. Its
expected
> size doesn't justify such a small change, so this is just a suggestion :-)

I went with vector for consistency with other list def.
I dont have any preference so if you feel strongly about it I am happy to change
it.

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

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:28: is_proxy = is_proxy
|| proxy_policies[i]==policy;
On 2011/06/03 12:23:51, joaodasilva wrote:
> Why not just "return true" here, and "return false" if the loop exits?

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:87:
cloud_policy_provider_.get()->Provide(store);
On 2011/06/03 12:23:51, joaodasilva wrote:
> The .get() isn't required, just cloud_policy_provider->Provide(..)

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:92:
cloud_policy_provider_.get()->AppendCache(cache);
On 2011/06/03 12:23:51, joaodasilva wrote:
> Same.

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:97:
cloud_policy_provider_.get()->PrependCache(cache);
On 2011/06/03 12:23:51, joaodasilva wrote:
> Same.

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:104:
cloud_policy_provider_.get()->CombineTwoPolicyMaps(base, overlay, out_map);
On 2011/06/03 12:23:51, joaodasilva wrote:
> Same.

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:134:
Value::CreateBooleanValue(true));
On 2011/06/03 12:23:51, joaodasilva wrote:
> Indent (also on the lines below)

Done.

http://codereview.chromium.org/6979011/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:253: }
On 2011/06/03 12:23:51, joaodasilva wrote:
> Indent.

Done.

[sfeuz, 2011-06-05 19:03:42]

Newest changes:
* Use UserDataDir instead of ProfileDir.
* There was a problem that user cloud policy is not initialized when session
manager restarts chrome after a crash. in such a case the --login-user switch is
used and LoginUtils::OnProfileCreated is not in the startup path. That should
work now. However there will be no TokenService reference in such a case. But
the DMToken should be cached anyways.

[sfeuz, 2011-06-08 07:23:03]

Changed the way the alternative startup path is handled:
* Using ProfileDir again instead of UserDataDir
* Initializing from browser_main just after profile was loaded.

Also rebased with the refresh rate changes and removed the overhead code there
needed before local_state was servable by user policy.

[Mattias Nissler (ping if slow), 2011-06-09 14:36:25]

a bag of assorted comments and style nits :)

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:358: if
(token_details->service() == GaiaConstants::kDeviceManagementService)
need parens for multi-line conditionals

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:12: #include "base/file_util.h"
no need for this include.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:94: void
ScheduleUserServiceInitialization(int64 delay_milliseconds);
Maybe we should merge these into one call? After all, the intent is to trigger
any initialization operations that are still pending. Can't the connector just
decide what still needs to be initialized?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:128: // Activates the device
cloud policy subsystem. Will be delayed.
"Will be delayed" isn't very helpful to a reader. Why and how?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:35: if
(level==POLICY_LEVEL_MANDATORY)
make it a switch?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:78: // recommended_policy()
depending on |level|.
Do we need the accessors above then? Seems we should have either one or the
other, but not both.

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:129:
mandatory_cloud_provider_.get()->Provide(&store);
Why can't we just check the policy map directly now that we can just access it?
It seems like we should remove the CloudPolicyProvider instances from this test,
since it is not subject of this test.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:29: namespace {
please move this to the top.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:41: void
CloudPolicyProvider::CombineTwoPolicyMaps(const PolicyMap& base,
You should order the function definitions according to their declaration order
in the header.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:49: added_proxy_policy =
added_proxy_policy || (is_proxy_policy(i->first));
parentheses around is_proxy_policy are not required.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:89: return true;
This seems wrong. I think the old code only returned true after policy has been
successfully loaded at least once.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.h (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:27: //   ApendCache(cache):   adds
|cache| to the back (i.e. least important cache).
Do we actually need both of them? I would expect one is enough?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:27: //   ApendCache(cache):   adds
|cache| to the back (i.e. least important cache).
Apend -> Append

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:62: ListType caches_;
Move the declaration of this data member down to the others.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:88: PolicyMap combined;
Missing a trailing underscore.

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:25: bool
is_proxy_policy(ConfigurationPolicyType policy) {
This is duplicated code. Can we somehow share?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:66: void
set_uninitialized() {
why not merge these two into set_initialized(bool)?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:68: }
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:165: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(testing::AnyNumber());
whitespace after comma

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:165: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(testing::AnyNumber());
It is common practice to import the symbols in testing you want to use with
using declarations at the top of the file.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:186: DCHECK(result);
Why DCHECK instead of EXPECT_TRUE()?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:217: 
no newline here.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:220: if
(is_proxy_policy((ConfigurationPolicyType)counter))
casting integers to enum members seems like a bad idea. Can't you just define a
PolicyDefinitionList as static data member using actual PolicyType constants?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:232: entries+n};
whitespace around +, after { and before }

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:248: if (i&1)
whitespace around &

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:258:
CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED:
whitespcae before :

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:260:
CreateCloudPolicyProvider(&simple_list,level);
whitespace after comma

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:266: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(
see my comment on importing symbols from testing:: above

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:294: int expected =
(i+1)*max_counter + (int)current->policy_type;
whitespace around + and * and after cast

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:310: for (int i=0; i <
n; ++i)
whitespace around =

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:311: if
(!is_proxy_policy((ConfigurationPolicyType)i))
whitespace after cast (also below)

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:311: if
(!is_proxy_policy((ConfigurationPolicyType)i))
Again, can we just use a static PolicyDefinitionMap instead of casting counters
to enum values?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:318:
CombineTwoPolicyMaps(A,B,&C);
whitespace after , (I'll stop commenting on missing whitespace at this point if
it's just another instance of the situations already mentioned.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache_unittest.cc:109: EXPECT_CALL(observer,
OnCacheUpdate(testing::_)).Times(1);
using testing::_ at the top of the file.

http://codereview.chromium.org/6979011/diff/27001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2021: 'browser/sync/api/syncable_service.cc',  #
TODO(zea): make own target.
Seems these lines shouldn't be in your CL.

[sfeuz, 2011-06-13 06:53:53]

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:358: if
(token_details->service() == GaiaConstants::kDeviceManagementService)
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> need parens for multi-line conditionals

Done.

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:12: #include "base/file_util.h"
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> no need for this include.

Need it for FilePath.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:94: void
ScheduleUserServiceInitialization(int64 delay_milliseconds);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Maybe we should merge these into one call? After all, the intent is to trigger
> any initialization operations that are still pending. Can't the connector just
> decide what still needs to be initialized?

I agree. I think we can just call the ScheduleInit methods of both user- and
device- subsystem everytime. The backend-service keeps track if it already got
initialized and will discard the next requests.
I will check with gfeher, who implemented this part.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:128: // Activates the device
cloud policy subsystem. Will be delayed.
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> "Will be delayed" isn't very helpful to a reader. Why and how?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.cc:35: if
(level==POLICY_LEVEL_MANDATORY)
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> make it a switch?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_base.h (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_base.h:78: // recommended_policy()
depending on |level|.
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Do we need the accessors above then? Seems we should have either one or the
> other, but not both.

I agree and think we should go with the policy() one.
Done.

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:129:
mandatory_cloud_provider_.get()->Provide(&store);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Why can't we just check the policy map directly now that we can just access
it?
> It seems like we should remove the CloudPolicyProvider instances from this
test,
> since it is not subject of this test.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:29: namespace {
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> please move this to the top.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:41: void
CloudPolicyProvider::CombineTwoPolicyMaps(const PolicyMap& base,
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> You should order the function definitions according to their declaration order
> in the header.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:49: added_proxy_policy =
added_proxy_policy || (is_proxy_policy(i->first));
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> parentheses around is_proxy_policy are not required.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.cc:89: return true;
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> This seems wrong. I think the old code only returned true after policy has
been
> successfully loaded at least once.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_provider.h (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:27: //   ApendCache(cache):   adds
|cache| to the back (i.e. least important cache).
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Do we actually need both of them? I would expect one is enough?

I'd like to keep both.
It is true that in the current setup we could live with prepend only, since
device policy should always be initialized earlier than user policy. However
with the alternative starting paths of CrOS that is not so clear and we would
have to DCHECK for no caches added yet.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:27: //   ApendCache(cache):   adds
|cache| to the back (i.e. least important cache).
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Apend -> Append

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:62: ListType caches_;
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Move the declaration of this data member down to the others.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider.h:88: PolicyMap combined;
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Missing a trailing underscore.

Done.

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

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:25: bool
is_proxy_policy(ConfigurationPolicyType policy) {
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> This is duplicated code. Can we somehow share?

Made it private static of CloudPolicyProvider.
wdyt?

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:66: void
set_uninitialized() {
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> why not merge these two into set_initialized(bool)?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:68: }
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:165: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(testing::AnyNumber());
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace after comma

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:165: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(testing::AnyNumber());
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> It is common practice to import the symbols in testing you want to use with
> using declarations at the top of the file.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:186: DCHECK(result);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Why DCHECK instead of EXPECT_TRUE()?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:217: 
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> no newline here.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:220: if
(is_proxy_policy((ConfigurationPolicyType)counter))
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> casting integers to enum members seems like a bad idea. Can't you just define
a
> PolicyDefinitionList as static data member using actual PolicyType constants?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:232: entries+n};
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace around +, after { and before }

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:248: if (i&1)
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace around &

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:258:
CloudPolicyCacheBase::POLICY_LEVEL_RECOMMENDED:
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespcae before :

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:260:
CreateCloudPolicyProvider(&simple_list,level);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace after comma

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:266: EXPECT_CALL(store,
Apply(testing::_,testing::_)).Times(
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> see my comment on importing symbols from testing:: above

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:294: int expected =
(i+1)*max_counter + (int)current->policy_type;
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace around + and * and after cast

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:310: for (int i=0; i <
n; ++i)
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace around =

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:311: if
(!is_proxy_policy((ConfigurationPolicyType)i))
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace after cast (also below)

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:311: if
(!is_proxy_policy((ConfigurationPolicyType)i))
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Again, can we just use a static PolicyDefinitionMap instead of casting
counters
> to enum values?

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_provider_unittest.cc:318:
CombineTwoPolicyMaps(A,B,&C);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> whitespace after , (I'll stop commenting on missing whitespace at this point
if
> it's just another instance of the situations already mentioned.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache_unittest.cc:109: EXPECT_CALL(observer,
OnCacheUpdate(testing::_)).Times(1);
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> using testing::_ at the top of the file.

Done.

http://codereview.chromium.org/6979011/diff/27001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6979011/diff/27001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:2021: 'browser/sync/api/syncable_service.cc',  #
TODO(zea): make own target.
On 2011/06/09 14:36:26, Mattias Nissler wrote:
> Seems these lines shouldn't be in your CL.

Done.

[gfeher, 2011-06-14 12:50:25]

Nice work Sandro!
I am continuing it in http://codereview.chromium.org/7147015/