Device policy infrastructure

Device policy infrastructure

This continues the work of http://codereview.chromium.org/6312121/. Description of that CL:

This refactors the cloud policy-related code to support device policy
that gets associated with the whole browser session. Device policy
information will show up in g_browser_process->local_state(). Also,
start supporting recommended policy from the cloud.

BUG=chromium-os:11259, chromium-os:11257, chromium-os:11256
TEST=Enable device policy by passing --device-policy-cache-dir, claim a device and verify that policy gets downloaded.

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

[Jakob Kummerow, 2011-02-14 13:53:01]

Continuation of Mattias' work. 

What's done:
- renaming of newly introduced classes
- integration with extended CloudPolicyCache

What's left to do (maybe in another CL):
- unit tests for new classes

Please review.

[gfeher, 2011-02-14 17:28:50]

My first round.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:30: // Activates the cloud
policy context. We can't pass these in the constructor
context->connector?

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache.cc:44: if (cache_->has_device_policy())
{
Shouldn't this be has_cloud_policy() ?

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:30: // of the Provide method,
which can be called on the FILE thread, all public
There is no Provide method here. Please update comment.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:67: // Indicates the current
state the provider is in.
provider->controller ?

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:40: // Shuts the context down.
This must be called before threading and network
context->subsystem?

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:749: if (!provider) {
Is this logic a temporary thing that will go away in the final design? Otherwise
please explain to me why is this good.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
File chrome/browser/policy/profile_policy_connector.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
chrome/browser/policy/profile_policy_connector.h:38:
ConfigurationPolicyProvider* GetRecommendedPolicyProvider();
Rename to GetManaged(Recommended)CloudProvider to become consistent with
BrowserPolicyProvider?

[brettw, 2011-02-14 17:49:31]

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:71:
device_management_service_->Initialize(request_context);
It looks like this function starts a URL request, and you're doing this on the
startup path. Is that correct? Is there a way to delay this request for a bit
after startup?

[Mattias Nissler (ping if slow), 2011-02-14 18:55:58]

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:71:
device_management_service_->Initialize(request_context);
On 2011/02/14 17:49:31, brettw wrote:
> It looks like this function starts a URL request, and you're doing this on the
> startup path. Is that correct? Is there a way to delay this request for a bit
> after startup?

Some background information:
- This code is currently CrOS only, since we don't have cloud policy enabled for
desktop Chrome at the moment (though we're planning to eventually do so)
- The request will only be generated if we already know that we need to recheck
for cloud policy. This is only the case if the user is set up for cloud policy
and the cache file is invalid or too old.
- There are actually two instances of CloudPolicySubsystem. One is for user
policy that is bound to a profile (affects e.g. homepage URL). We initialize the
cloud policy stuff after the profile is up, because we want to have up-to-date
policy configuration as soon as possible after firing up the browser.
- The second instance of CloudPolicySubsystem is concerned with CrOS device
policy. It's started as soon as the default request context (to be replaced by
willchan's system request context eventually) is up, since we need the
up-to-date cloud policy information in order to make a call on whether we want
to accept a user to log on to CrOS (in case the user white list is managed
through policy)

Bottom line: I think the requests happen at the right time and delaying them is
either not helpful/appropriate. Moreover, we try hard to only make the requests
when policy is actually in effect for the user/device. I'm happy to discuss
further if you still have concerns!

[brettw, 2011-02-14 19:15:35]

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:71:
device_management_service_->Initialize(request_context);
On 2011/02/14 18:55:58, Mattias Nissler wrote:
> On 2011/02/14 17:49:31, brettw wrote:
> > It looks like this function starts a URL request, and you're doing this on
the
> > startup path. Is that correct? Is there a way to delay this request for a
bit
> > after startup?
> 
> Some background information:
> - This code is currently CrOS only, since we don't have cloud policy enabled
for
> desktop Chrome at the moment (though we're planning to eventually do so)
> - The request will only be generated if we already know that we need to
recheck
> for cloud policy. This is only the case if the user is set up for cloud policy
> and the cache file is invalid or too old.
> - There are actually two instances of CloudPolicySubsystem. One is for user
> policy that is bound to a profile (affects e.g. homepage URL). We initialize
the
> cloud policy stuff after the profile is up, because we want to have up-to-date
> policy configuration as soon as possible after firing up the browser.

Everybody wants everything as soon as possible. The question here is whether you
want to slow down the main window appearing. It seems impossible for the initial
homepage display to be affected by this request since it will happen too fast.
Since you've already lost that initial display, why not allow the window to come
up a bit faster? We do this whenever possible.

This seems to me to be a lot like chrome/browser/google/google_url_tracker.cc
which is trying to find the correct country-specific URL to use for searches if
you have your search provider set to Google. In both cases there's a cache, a
network request, and the response affects "stuff you want to do right away"
after starting the browser. You can see that code has a 5 second timer. Given
that we've put so much effort into this kind of thing in the past, I'm not sure
how your case is different.


> - The second instance of CloudPolicySubsystem is concerned with CrOS device
> policy. It's started as soon as the default request context (to be replaced by
> willchan's system request context eventually) is up, since we need the
> up-to-date cloud policy information in order to make a call on whether we want
> to accept a user to log on to CrOS (in case the user white list is managed
> through policy)

Okay, and I assume you don't allow login until this response has completed?

Brett

[Mattias Nissler (ping if slow), 2011-02-14 19:38:56]

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:71:
device_management_service_->Initialize(request_context);
On 2011/02/14 19:15:35, brettw wrote:
> On 2011/02/14 18:55:58, Mattias Nissler wrote:
> > On 2011/02/14 17:49:31, brettw wrote:
> > > It looks like this function starts a URL request, and you're doing this on
> the
> > > startup path. Is that correct? Is there a way to delay this request for a
> bit
> > > after startup?
> > 
> > Some background information:
> > - This code is currently CrOS only, since we don't have cloud policy enabled
> for
> > desktop Chrome at the moment (though we're planning to eventually do so)
> > - The request will only be generated if we already know that we need to
> recheck
> > for cloud policy. This is only the case if the user is set up for cloud
policy
> > and the cache file is invalid or too old.
> > - There are actually two instances of CloudPolicySubsystem. One is for user
> > policy that is bound to a profile (affects e.g. homepage URL). We initialize
> the
> > cloud policy stuff after the profile is up, because we want to have
up-to-date
> > policy configuration as soon as possible after firing up the browser.
> 
> Everybody wants everything as soon as possible. The question here is whether
you
> want to slow down the main window appearing. It seems impossible for the
initial
> homepage display to be affected by this request since it will happen too fast.
> Since you've already lost that initial display, why not allow the window to
come
> up a bit faster? We do this whenever possible.

That sounds reasonable. However, I'm not yet buying the argument that not
generating the request will make the window come up faster. The request
processing happens asynchronously on the IO thread, so why would we see slowdown
(assuming number of cores is > 1)?

The other thing is that we want to actually notify the user when we learn policy
is in effect through an infobar. I'm a bit concerned about the UX on CrOS: With
the delay you log on, wait five seconds, and then magically your policy arrives
modifies your toolbar and you see the infobar, whereas when we fetch immediately
you log on and (assuming the server answers in reasonable time) get the toolbar
change and infobar close to instantly (Note we've already made the compromise
not to block CrOS login waiting for policy).

> 
> This seems to me to be a lot like chrome/browser/google/google_url_tracker.cc
> which is trying to find the correct country-specific URL to use for searches
if
> you have your search provider set to Google. In both cases there's a cache, a
> network request, and the response affects "stuff you want to do right away"
> after starting the browser. You can see that code has a 5 second timer. Given
> that we've put so much effort into this kind of thing in the past, I'm not
sure
> how your case is different.

It's different in that delaying it affects UX much more as explained above. But
I can see that it's similar in the other points you brought up.

> 
> 
> > - The second instance of CloudPolicySubsystem is concerned with CrOS device
> > policy. It's started as soon as the default request context (to be replaced
by
> > willchan's system request context eventually) is up, since we need the
> > up-to-date cloud policy information in order to make a call on whether we
want
> > to accept a user to log on to CrOS (in case the user white list is managed
> > through policy)
> 
> Okay, and I assume you don't allow login until this response has completed?

That's the plan, details TBD.

[Mattias Nissler (ping if slow), 2011-02-15 10:15:16]

mostly nits.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:121: #include <dbus/dbus-glib.h>
why? I guess I accidentally introduced this in my first version.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:158: #include
"chrome/browser/ui/cocoa/install_from_dmg.h"
same here.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:11: #include
"chrome/browser/policy/cloud_policy_identity_strategy.h"
alphabetize

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:20: class
CloudPolicyIdentityStrategy;
alphabetize

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:30: // of the Provide method,
which can be called on the FILE thread, all public
Also, the whole threading issues mentioned here don't apply any longer.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:68: enum ClientState {
s/Client/Controller/

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller_unittest.cc:24: #if 0
I think we agree this cannot go in in this state. We should at least rework the
existing tests that still apply. I'm OK with adding more comprehensive tests in
a follow-up CL.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:27: // policy. It glues together
the backend, the policy client and manages the
s/policy client/policy controller/

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:66:
scoped_ptr<CloudPolicyController> cloud_policy_controller_;
DISALLOW_COPY_AND_ASSIGN

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:749: if (!provider) {
On 2011/02/14 17:28:50, gfeher wrote:
> Is this logic a temporary thing that will go away in the final design?
Otherwise
> please explain to me why is this good.

The point here is that for user policy (i.e. profile != NULL, applying to the
profile's PrefService), we want to return the user policy provider that lives in
the profile's policy connector and for device policy (i.e. profile == NULL,
applies to local_state), we should return provider from the global connector.

But you do have a point, we should probably do this:

if (profile)
  ...
else
  ...

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:751:
g_browser_process->browser_policy_connector();
do we need the connector local? I guess it doesn't help readability much any
longer.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:773: if (!provider) {
same here.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:775:
g_browser_process->browser_policy_connector();
drop connector local?

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_cache_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
seems like this file should have been removed?

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:59: const std::string& device_id)
{
Should we put a call to Reset() here?

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:19: #if 0
Can we re-enable existing tests that still apply?

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
File chrome/browser/policy/profile_policy_connector.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
chrome/browser/policy/profile_policy_connector.h:38:
ConfigurationPolicyProvider* GetRecommendedPolicyProvider();
On 2011/02/14 17:28:50, gfeher wrote:
> Rename to GetManaged(Recommended)CloudProvider to become consistent with
> BrowserPolicyProvider?

BrowserPolicyProvider also holds the platform policy providers, that's why I
added the Cloud infix there. I have no strong feelings about the change though.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.h:43: // Checks whether a
new token should be fetched and if so, pokes the fetcher.
Comment is not accurate. It doesn't poke the fetcher but sends a notification.

http://codereview.chromium.org/6520008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6520008/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:1763:
'browser/policy/cloud_policy_identity_strategy.h',
alphabetize.

http://codereview.chromium.org/6520008/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:679: // by the policy provider.
Comment outdated, it's not the policy provider that clamps.

[danno, 2011-02-15 12:27:15]

some nits, and please implement test cases.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:31: // since the context must
start up before threading and net is up.
context->connector here, too.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/profiles/profile.h
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile.h:497: // Gets the policy context associated
with this profile.
connector?

http://codereview.chromium.org/6520008/diff/1/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:56: extern const char kDevicePolicyCacheDir[];
Alphabetize

[danno, 2011-02-15 12:52:58]

Mostly nits. Please implement this missing tests, then you'll get my LGTM.

[gfeher, 2011-02-15 13:17:01]

more nits

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_identity_strategy.h:10: #include <vector>
Do you actually need vector here?

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:35: // Initializes the context.
context->subsystem

[brettw, 2011-02-15 21:53:51]

On 2011/02/14 19:38:56, Mattias Nissler wrote:
> That sounds reasonable. However, I'm not yet buying the argument that not
> generating the request will make the window come up faster. The request
> processing happens asynchronously on the IO thread, so why would we see
slowdown
> (assuming number of cores is > 1)?
> 
> The other thing is that we want to actually notify the user when we learn
policy
> is in effect through an infobar. I'm a bit concerned about the UX on CrOS:
With
> the delay you log on, wait five seconds, and then magically your policy
arrives
> modifies your toolbar and you see the infobar, whereas when we fetch
immediately
> you log on and (assuming the server answers in reasonable time) get the
toolbar
> change and infobar close to instantly (Note we've already made the compromise
> not to block CrOS login waiting for policy).

Who are you working with on the UI side?

I think even a short 1 second timer would be helpful for the ChromeOS startup
case, and probably we can tolerate a longer one for the login side.

Chrome starts up fast because we defer as much as possible during startup, even
for things that aren't individually measurably slow.

Your code is likely to be the first network request for the entire system. I
just checked on my Windows box and currently the first request is for the home
page, which is as it should be.

Adding this faster request will initialize all kinds of heavyweight things
sooner during startup like the system networking libraries and the disk cache.
Even though this runs on a background thread, these will in turn cause I/O,
network, and CPU contention for the UI thread which will cause jankyness and
delay other things the user wants "right away" like their bookmarks, the home
page, their history for top sites and autocomplete, etc.

On ChromeOS, where the CPU is more limited, the CPU contention during system
startup is an even more performance sensitive area.

[gfeher, 2011-02-18 20:12:26]

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:43: const FilePath&
cache_file_;
This shouldn't be a reference.

[gfeher, 2011-02-18 20:21:29]

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:27:
UserPolicyIdentityStrategy::TokenCache> {
Nit: please create a private destructor and make
base::RefCountedThreadSafe<UserPolicyIdentityStrategy::TokenCache> a friend.

[Jakob Kummerow, 2011-02-21 12:12:15]

I addressed all of your feedback; please have another look. 

I'm currently working on the ChromeOS test failures, expect a small update to
fix those soon.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:121: #include <dbus/dbus-glib.h>
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> why? I guess I accidentally introduced this in my first version.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:158: #include
"chrome/browser/ui/cocoa/install_from_dmg.h"
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> same here.

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:11: #include
"chrome/browser/policy/cloud_policy_identity_strategy.h"
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> alphabetize

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:20: class
CloudPolicyIdentityStrategy;
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:30: // Activates the cloud
policy context. We can't pass these in the constructor
On 2011/02/14 17:28:50, gfeher wrote:
> context->connector?

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:31: // since the context must
start up before threading and net is up.
On 2011/02/15 12:27:15, danno wrote:
> context->connector here, too.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_cache.cc:44: if (cache_->has_device_policy())
{
On 2011/02/14 17:28:50, gfeher wrote:
> Shouldn't this be has_cloud_policy() ?

Done. Good catch!

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:30: // of the Provide method,
which can be called on the FILE thread, all public
On 2011/02/14 17:28:50, gfeher wrote:
> There is no Provide method here. Please update comment.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:30: // of the Provide method,
which can be called on the FILE thread, all public
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> Also, the whole threading issues mentioned here don't apply any longer.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:67: // Indicates the current
state the provider is in.
On 2011/02/14 17:28:50, gfeher wrote:
> provider->controller ?

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller.h:68: enum ClientState {
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> s/Client/Controller/

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_controller_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_controller_unittest.cc:24: #if 0
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> I think we agree this cannot go in in this state. We should at least rework
the
> existing tests that still apply. I'm OK with adding more comprehensive tests
in
> a follow-up CL.

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_identity_strategy.h:10: #include <vector>
On 2011/02/15 13:17:01, gfeher wrote:
> Do you actually need vector here?

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_subsystem.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:71:
device_management_service_->Initialize(request_context);
<snip: long discussion about delaying this method's work for the benefit of
faster browser startup>

As discussed, since fixing this existing sub-optimal behavior is not in the
scope of this CL, we will land it as is for now, but put in a comment describing
the problem and fix it soon in another CL.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:27: // policy. It glues together
the backend, the policy client and manages the
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> s/policy client/policy controller/

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:35: // Initializes the context.
On 2011/02/15 13:17:01, gfeher wrote:
> context->subsystem

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:40: // Shuts the context down.
This must be called before threading and network
On 2011/02/14 17:28:50, gfeher wrote:
> context->subsystem?

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:66:
scoped_ptr<CloudPolicyController> cloud_policy_controller_;
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:749: if (!provider) {
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> On 2011/02/14 17:28:50, gfeher wrote:
> > Is this logic a temporary thing that will go away in the final design?
> Otherwise
> > please explain to me why is this good.
> 
> The point here is that for user policy (i.e. profile != NULL, applying to the
> profile's PrefService), we want to return the user policy provider that lives
in
> the profile's policy connector and for device policy (i.e. profile == NULL,
> applies to local_state), we should return provider from the global connector.
> 
> But you do have a point, we should probably do this:
> 
> if (profile)
>   ...
> else
>   ...
> 

Done. And I've thrown in a comment for good measure :-)

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:751:
g_browser_process->browser_policy_connector();
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> do we need the connector local? I guess it doesn't help readability much any
> longer.

We don't need it, but it prevents an ugly line break (at "->").

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:773: if (!provider) {
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:775:
g_browser_process->browser_policy_connector();
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> drop connector local?

Same as above -- keeping it as it is for line breaking reasons.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_ma...
File chrome/browser/policy/device_management_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_cache_unittest.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> seems like this file should have been removed?

Done in the other CL (that this one is based on).

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:59: const std::string& device_id)
{
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> Should we put a call to Reset() here?

No need. In fact, as discussed offline I've removed Reset() completely.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher_unittest.cc:19: #if 0
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> Can we re-enable existing tests that still apply?

Done. That means, rewrote the tests that still make sense.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
File chrome/browser/policy/profile_policy_connector.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/profile_p...
chrome/browser/policy/profile_policy_connector.h:38:
ConfigurationPolicyProvider* GetRecommendedPolicyProvider();
On 2011/02/14 17:28:50, gfeher wrote:
> Rename to GetManaged(Recommended)CloudProvider to become consistent with
> BrowserPolicyProvider?

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:27:
UserPolicyIdentityStrategy::TokenCache> {
On 2011/02/18 20:21:29, gfeher wrote:
> Nit: please create a private destructor and make
> base::RefCountedThreadSafe<UserPolicyIdentityStrategy::TokenCache> a friend.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.cc:43: const FilePath&
cache_file_;
On 2011/02/18 20:12:26, gfeher wrote:
> This shouldn't be a reference.

Done.

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

http://codereview.chromium.org/6520008/diff/1/chrome/browser/policy/user_poli...
chrome/browser/policy/user_policy_identity_strategy.h:43: // Checks whether a
new token should be fetched and if so, pokes the fetcher.
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> Comment is not accurate. It doesn't poke the fetcher but sends a notification.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/browser/profiles/profile.h
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile.h:497: // Gets the policy context associated
with this profile.
On 2011/02/15 12:27:15, danno wrote:
> connector?

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6520008/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:1763:
'browser/policy/cloud_policy_identity_strategy.h',
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> alphabetize.

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6520008/diff/1/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:56: extern const char kDevicePolicyCacheDir[];
On 2011/02/15 12:27:15, danno wrote:
> Alphabetize

Done.

http://codereview.chromium.org/6520008/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/6520008/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:679: // by the policy provider.
On 2011/02/15 10:15:16, Mattias Nissler wrote:
> Comment outdated, it's not the policy provider that clamps.

Done.

[Jakob Kummerow, 2011-02-21 12:51:04]

As promised, here's an update to fix the ChromeOS tests.

[Mattias Nissler (ping if slow), 2011-02-21 14:39:13]

Almost there. The tests are good, thanks for writing them!

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:56: #include
"chrome/browser/policy/browser_policy_connector.h"
Don't need this. Actually, you can drop the whole file from the CL.

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:158:
DCHECK_EQ(NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE, type.value);
I think the usual if-else-NOTREACHED is better, since we run into the exact
problem you resolved with this, no?

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:59: // Activates the cloud
policy connector. Called when the default request
s/connector/subsystem/? Dunno what name's better.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:234: if (new_policy_differs) {
Do we need also notify if initialization_complete_ was flipped to true?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:264: if (new_policy_differs) {
Same here?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:37: class CloudPolicyCache : public
base::NonThreadSafe {
I don't know who requested to derive from NonThreadSafe (was it me?). The
documentation states "This is intended to be used with classes that appear to be
thread safe, but aren't." I think our default is to assume non-thread-safety,
except for classes that obviously do thread switching, so I don't think that
CloudPolicyCache "appears to be thread safe" here, but YMMV.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:142:
ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:104:
ConfigurationPolicyObserverRegistrar registrar;
Hm, it seems a curious choice to hide the observer tests in this helper method.
Why not have separate test that tests observer functionality and drop the
observer here?

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:36: void
RemoveObserver(CloudPolicyIdentityStrategy::Observer*) {}
Why do you have the observer declarations here?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:38: private:
I think most of our code has a newline before visibility declarations.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:57: void
RemoveObserver(DeviceTokenFetcher::Observer*) {}
Why the observer declarations?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:58: private:
Same here.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:162:
SetupIdentityStrategy("", "device_id", "DannoHelper@gmail.com", "auth_token");
Haha! I guess I hereby proved I read testing code in reviews :)

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:116: ==
*(Details<std::string>(details).ptr()) &&
We should break the line after the ==

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:54: }
Newline before comments.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:65: }
newline before visibility decl?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:67:
std::deque<DeviceManagementBackend*> backends_;
Hm. I've thought about this a bit and the problem you're trying to solve is that
the fetcher assumes ownership of the backend when it calls CreateBackend, right?

If so, can't we have a subclass of MockDeviceManagementBackend that also
implements -service and hands out proxies that can be deleted? That way, you
could also use the usual sequencing in gmock, no?

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:171: #if 0
Delete? If you want the TODO, I'd rather put it on the TokenCache declaration.

http://codereview.chromium.org/6520008/diff/16001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:12: #include "build/build_config.h"
I guess this is one of my accidental reorderings that was can revert since
build_config.h should go first. Or better, put build_config.h first, followed by
a newline.

[Jakob Kummerow, 2011-02-22 10:02:33]

Review control, this is Charlie-Lima-Six-Five-Two-Something, approaching runway
2 and requesting permission to land.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/browser_main...
chrome/browser/browser_main.cc:56: #include
"chrome/browser/policy/browser_policy_connector.h"
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Don't need this. Actually, you can drop the whole file from the CL.

Done.

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.cc:158:
DCHECK_EQ(NotificationType::DEFAULT_REQUEST_CONTEXT_AVAILABLE, type.value);
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> I think the usual if-else-NOTREACHED is better, since we run into the exact
> problem you resolved with this, no?

Done.

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:59: // Activates the cloud
policy connector. Called when the default request
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> s/connector/subsystem/? Dunno what name's better.

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:234: if (new_policy_differs) {
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Do we need also notify if initialization_complete_ was flipped to true?

Done.
I believe this case won't happen anyway with the current implementation, but if
it did occur, we'd need to send out the notification.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.cc:264: if (new_policy_differs) {
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Same here?

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache.h (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:37: class CloudPolicyCache : public
base::NonThreadSafe {
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> I don't know who requested to derive from NonThreadSafe (was it me?). The
> documentation states "This is intended to be used with classes that appear to
be
> thread safe, but aren't." I think our default is to assume non-thread-safety,
> except for classes that obviously do thread switching, so I don't think that
> CloudPolicyCache "appears to be thread safe" here, but YMMV.

As discussed offline, we'll leave it as is.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache.h:142:
ObserverList<ConfigurationPolicyProvider::Observer, true> observer_list_;
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_cache_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_cache_unittest.cc:104:
ConfigurationPolicyObserverRegistrar registrar;
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Hm, it seems a curious choice to hide the observer tests in this helper
method.
> Why not have separate test that tests observer functionality and drop the
> observer here?

This doesn't just test whether notifying the observers work. Rather, it tests
whether the cache correctly determined whether the new policy is different from
the existing policy or not. We used to have the return value of SetPolicy() for
that, but that method's semantics have changed, so we need the observer
notification as a sentry mechanism.

As discussed, I'll leave it like this.

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:36: void
RemoveObserver(CloudPolicyIdentityStrategy::Observer*) {}
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Why do you have the observer declarations here?

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:38: private:
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> I think most of our code has a newline before visibility declarations.

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:57: void
RemoveObserver(DeviceTokenFetcher::Observer*) {}
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Why the observer declarations?

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:58: private:
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Same here.

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:162:
SetupIdentityStrategy("", "device_id", "DannoHelper@gmail.com", "auth_token");
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Haha! I guess I hereby proved I read testing code in reviews :)

I was wondering whether or not I'd get that through the review process ;)

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

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:116: ==
*(Details<std::string>(details).ptr()) &&
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> We should break the line after the ==

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:54: }
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Newline before comments.

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:65: }
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> newline before visibility decl?

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:67:
std::deque<DeviceManagementBackend*> backends_;
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Hm. I've thought about this a bit and the problem you're trying to solve is
that
> the fetcher assumes ownership of the backend when it calls CreateBackend,
right?
> 
> If so, can't we have a subclass of MockDeviceManagementBackend that also
> implements -service and hands out proxies that can be deleted? That way, you
> could also use the usual sequencing in gmock, no?

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:171: #if 0
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> Delete? If you want the TODO, I'd rather put it on the TokenCache declaration.

Done.

http://codereview.chromium.org/6520008/diff/16001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6520008/diff/16001/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:12: #include "build/build_config.h"
On 2011/02/21 14:39:13, Mattias Nissler wrote:
> I guess this is one of my accidental reorderings that was can revert since
> build_config.h should go first. Or better, put build_config.h first, followed
by
> a newline.

Done.

[Mattias Nissler (ping if slow), 2011-02-22 12:01:20]

LGTM with nits and pending trybots.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:38: class
ProxyDeviceManagementBackend;
Don't need that forward declaration.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:40: class
DeviceManagementServiceBackendHybrid
How about a comment?

[gfeher, 2011-02-22 13:45:42]

LGTM with a nit.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_connector.h (right):

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.h:48:
scoped_ptr<UserPolicyIdentityStrategy> identity_strategy;
Nit: please append underscore to the end of the name.

[danno, 2011-02-22 15:02:54]

mostly nits

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

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:30:
~MockCloudPolicyIdentityStrategy() {}
nit: virtual

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:51:
~MockDeviceTokenFetcher() {}
nit: virtual

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:76:
controller_.reset();  // Implicitly unregisters observers.
Nit: Don't you mean "explicitly"? Or maybe just: // unregister observers.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:42: public
MockDeviceManagementBackend {
Can you make the DeviceManagementServiceBackendHybrid "have a"
MockDeviceManagementBackend backend rather then having the it "be one"?

[Jakob Kummerow, 2011-02-22 16:53:58]

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

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:30:
~MockCloudPolicyIdentityStrategy() {}
On 2011/02/22 15:02:54, danno wrote:
> nit: virtual

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:51:
~MockDeviceTokenFetcher() {}
On 2011/02/22 15:02:54, danno wrote:
> nit: virtual

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_controller_unittest.cc:76:
controller_.reset();  // Implicitly unregisters observers.
On 2011/02/22 15:02:54, danno wrote:
> Nit: Don't you mean "explicitly"? Or maybe just: // unregister observers.

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:38: class
ProxyDeviceManagementBackend;
On 2011/02/22 12:01:20, Mattias Nissler wrote:
> Don't need that forward declaration.

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:40: class
DeviceManagementServiceBackendHybrid
On 2011/02/22 12:01:20, Mattias Nissler wrote:
> How about a comment?

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:42: public
MockDeviceManagementBackend {
On 2011/02/22 15:02:54, danno wrote:
> Can you make the DeviceManagementServiceBackendHybrid "have a"
> MockDeviceManagementBackend backend rather then having the it "be one"?

Done.

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_connector.h (right):

http://codereview.chromium.org/6520008/diff/20001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.h:48:
scoped_ptr<UserPolicyIdentityStrategy> identity_strategy;
On 2011/02/22 13:45:42, gfeher wrote:
> Nit: please append underscore to the end of the name.

Done.

[danno, 2011-02-22 17:32:09]

LGTM with one nit!

http://codereview.chromium.org/6520008/diff/17006/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/17006/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:52:
DeviceManagementBackend* backend_;
nit: mark as weak

[Jakob Kummerow, 2011-02-23 10:04:56]

http://codereview.chromium.org/6520008/diff/17006/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/6520008/diff/17006/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:52:
DeviceManagementBackend* backend_;
On 2011/02/22 17:32:09, danno wrote:
> nit: mark as weak

Done.

...and landed.