Allow policy refresh rate to be configured through policy.

Allow policy refresh rate to be configured through policy.

BUG=64877
TEST=Existing unittests; manual testing.

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

[Mattias Nissler (ping if slow), 2011-01-05 19:54:24]

Please review.

Note that I changed the internals of DeviceManagementPolicyProvider to work more
like a state machine. I personally find the new approach much more
understandable, but I'm also open to discussion.

[danno, 2011-01-06 19:16:26]

The state machine seems generally OK, although seem my comments for one spot
where it didn't get clearer IMO.

You need to add the .json and .grd changes, specifically comments about the
clamping of min/max times in the doc.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:27: // The maximum
ratio in percent of the policy refresh rate we use for fuzzing
I like random deviation here, too.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:283: if
(!IsInitializationComplete() &&
I am confused here. If !IsInitializationComplete you notify the observers that
the initial policy fetch is complete? Perhaps an explanatory comment.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:287:
NotifyCloudPolicyUpdate();
I found the old code much more readable, it was clear that the notification
happened when there was an error, when the device isn't managed, etc. Perhaps it
would be clearer if this call was included in the individual state transitions.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:86: int
policy_refresh_fuzz_factor_percent,
how about "deviation" or "random_deviation" instead of "fuzz"?

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/profil...
File chrome/browser/policy/profile_policy_context.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/profil...
chrome/browser/policy/profile_policy_context.cc:84: if (type ==
NotificationType::PREF_CHANGED &&
How do you know that this notification will get processed after the PrefMember?
If this observer gets called first, you will get the wrong behavior.

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

http://codereview.chromium.org/6079012/diff/3001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:627: // Integer that gives the policy refresh rate
in milliseconds. Not all values
s/gives/specifies

[danno, 2011-01-06 19:19:49]

a few more...

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:68: RemoveAll();
It's not obvious to me why this has been added but wasn't needed before. If
there is a new behavior introduced by the policy state machine that requires
this, is there any way you can add a test?

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:200: state_ == kStateNotManaged);
Similar to above: if you are fixing a particular bug with this non-obvious
change, then a test cases might be a good idea.

[Jakob Kummerow, 2011-01-07 09:48:51]

For the record, I'm fine with the state machine changes. I don't think it has a
lot of immediate benefit, but it is a somewhat cleaner approach.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:146: int64
refresh_rate_milliseconds) {
missing line:
policy_refresh_rate_ms_ = refresh_rate_milliseconds;

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:68: RemoveAll();
On 2011/01/06 19:19:49, danno wrote:
> It's not obvious to me why this has been added but wasn't needed before. 

It's been added so that Init() can be called multiple times (when the token
fetcher is replaced by a call to DMPProvider::SetDeviceTokenFetcher()). Calling
RemoveAll() on subsequent calls to Init() makes sure the old token fetcher can
be destructed cleanly.

[Mattias Nissler (ping if slow), 2011-01-07 11:15:33]

Here's an updated version that addresses your comments. Note that the template
generator is unable to handle int-typed policies. That means we first have to
implement support for that in the template generator. Thus, I'll have to wait
with landing this or strip out the template bits and land them later. What do
you prefer?

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:27: // The maximum
ratio in percent of the policy refresh rate we use for fuzzing
On 2011/01/06 19:16:26, danno wrote:
> I like random deviation here, too.

Done.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:283: if
(!IsInitializationComplete() &&
On 2011/01/06 19:16:26, danno wrote:
> I am confused here. If !IsInitializationComplete you notify the observers that
> the initial policy fetch is complete? Perhaps an explanatory comment.

The idea is to notify observers once we switch to a state where policy is
available. The code here was indeed wrong. I changed the tests to validate that
this notification actually gets sent.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.cc:287:
NotifyCloudPolicyUpdate();
On 2011/01/06 19:16:26, danno wrote:
> I found the old code much more readable, it was clear that the notification
> happened when there was an error, when the device isn't managed, etc. Perhaps
it
> would be clearer if this call was included in the individual state
transitions.

I disagree. We now have a central point that detects whether we switch from an
initializing state to a steady state. That gives much more confidence that we
didn't miss some code path.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_management_policy_provider.h (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_management_policy_provider.h:86: int
policy_refresh_fuzz_factor_percent,
On 2011/01/06 19:16:26, danno wrote:
> how about "deviation" or "random_deviation" instead of "fuzz"? 

Done.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:68: RemoveAll();
On 2011/01/06 19:19:49, danno wrote:
> It's not obvious to me why this has been added but wasn't needed before. If
> there is a new behavior introduced by the policy state machine that requires
> this, is there any way you can add a test? 

This is actually a left-over from an experiment and not required. Removed.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/device...
chrome/browser/policy/device_token_fetcher.cc:200: state_ == kStateNotManaged);
On 2011/01/06 19:19:49, danno wrote:
> Similar to above: if you are fixing a particular bug with this non-obvious
> change, then a test cases might be a good idea.

This allows us to Restart() the fetcher even before we ran StartFetching(). I
added a test for that.

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/profil...
File chrome/browser/policy/profile_policy_context.cc (right):

http://codereview.chromium.org/6079012/diff/3001/chrome/browser/policy/profil...
chrome/browser/policy/profile_policy_context.cc:84: if (type ==
NotificationType::PREF_CHANGED &&
On 2011/01/06 19:16:26, danno wrote:
> How do you know that this notification will get processed after the
PrefMember?
> If this observer gets called first, you will get the wrong behavior.

No, PrefMember doesn't register the observer with the PrefService directly, but
rather forwards the notification to use when it detects a change.

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

http://codereview.chromium.org/6079012/diff/3001/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:627: // Integer that gives the policy refresh rate
in milliseconds. Not all values
On 2011/01/06 19:16:26, danno wrote:
> s/gives/specifies

Done.

[Mattias Nissler (ping if slow), 2011-01-17 14:05:27]

Can you do another review pass? Thanks.

[danno, 2011-01-18 10:07:11]

LGTM, possible changing the InSequence usage in tests as discussed if possible.