Split the policy refresh rate preference into user- and device-policy refresh rate.

Split the policy refresh rate preference into user- and device-policy refresh rate.

This is an effort towards removing dependencies of the ProfilePolicyConnector on Profile.

BUG=none
TEST=Verify that user- and device-refresh rate are set correctly when specified by policy.


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

[sfeuz, 2011-05-13 08:58:10]

Will either add a unittest or add to gfehers pending cloud policy test.

[Mattias Nissler (ping if slow), 2011-05-13 09:45:03]

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:196:
cloud_policy_subsystem_->Initialize(prefs::kDevicePolicyRefreshRate,
Indentation

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:94: void
CloudPolicySubsystem::Initialize(const char* policy_refresh_name,
Indentation

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:102:
std::string(prefs::kDevicePolicyRefreshRate));
Why this DCHECK? The subsystem doesn't care what policy name to use. If you
really want this semantics I suggest passing in an enum value as selector rather
than a string.

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:172:
DCHECK(Source<PrefService>(source).ptr() ==
Why not use DCHECK_EQ?

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:83: void Initialize(const char*
policy_refresh_name,
it's not a policy name but a pref name.

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:249: {
Value::TYPE_INTEGER, kPolicyPolicyRefreshRate,
We'll run into a problem here if we use the same policy type for both prefs:
Even if we have two different sources for policy values (user and device), we'll
merge them here, which is not what we want. Adding a new policy type goes
through the autogenerated code, but I'm not sure whether we want the device
policies in the JSON template. Need to discuss this with gfeher@ I guess

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

http://codereview.chromium.org/7014036/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:706: const char kUserPolicyRefreshRate[] =
"policy.user_refresh_rate";
Same here.

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

http://codereview.chromium.org/7014036/diff/1/chrome/common/pref_names.h#newc...
chrome/common/pref_names.h:250: extern const char kUserPolicyRefreshRate[];
Please move these to the local_state prefs section below.

[pastarmovj, 2011-05-16 14:22:33]

looks good just two small nits.

http://codereview.chromium.org/7014036/diff/8001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1536: Setting this policy overrides the
default value of 3 hours. Valid values for this policy are in the range from 30
minutes to 1 day. Any values not in this range will be clamped to the respective
boundary.''',
It might be helpful to do the math here for the poor souls that might want to
set these by hand [1800000..86400000].

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache_unittest.cc:67:
settings.mutable_device_policy_refresh_rate()->
Why don't you introduce a variable corresponding to
settings.mutable_device_policy_refresh_rate() instead of the |settings| var you
don't seem to use |settings| for anything but calling
mutable_device_policy_refresh_rate on it. This should improve greatly the
readability of these lines.

[Mattias Nissler (ping if slow), 2011-05-16 14:28:18]

Looks good in general, but please run CrOS trybots and do some manual testing.

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.cc:278:
container.device_policy_refresh_rate()));
It seems you didn't change the .proto, so I guess this won't compile on CrOS.

[sfeuz, 2011-05-17 14:27:42]

Testing this now manually and reporting back when testing done for final review.

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.cc:196:
cloud_policy_subsystem_->Initialize(prefs::kDevicePolicyRefreshRate,
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Indentation

Done.

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:94: void
CloudPolicySubsystem::Initialize(const char* policy_refresh_name,
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Indentation

Done.

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:102:
std::string(prefs::kDevicePolicyRefreshRate));
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Why this DCHECK? The subsystem doesn't care what policy name to use. If you
> really want this semantics I suggest passing in an enum value as selector
rather
> than a string.

Done.

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.cc:172:
DCHECK(Source<PrefService>(source).ptr() ==
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Why not use DCHECK_EQ?

Done.

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_subsystem.h:83: void Initialize(const char*
policy_refresh_name,
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> it's not a policy name but a pref name.

Done.

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:249: {
Value::TYPE_INTEGER, kPolicyPolicyRefreshRate,
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> We'll run into a problem here if we use the same policy type for both prefs:
> Even if we have two different sources for policy values (user and device),
we'll
> merge them here, which is not what we want. Adding a new policy type goes
> through the autogenerated code, but I'm not sure whether we want the device
> policies in the JSON template. Need to discuss this with gfeher@ I guess

Added a policy for the DevicePolicyRefreshRate.

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

http://codereview.chromium.org/7014036/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:706: const char kUserPolicyRefreshRate[] =
"policy.user_refresh_rate";
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Same here.

Done.

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

http://codereview.chromium.org/7014036/diff/1/chrome/common/pref_names.h#newc...
chrome/common/pref_names.h:250: extern const char kUserPolicyRefreshRate[];
On 2011/05/13 09:45:03, Mattias Nissler wrote:
> Please move these to the local_state prefs section below.

Done.

http://codereview.chromium.org/7014036/diff/8001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:1536: Setting this policy overrides the
default value of 3 hours. Valid values for this policy are in the range from 30
minutes to 1 day. Any values not in this range will be clamped to the respective
boundary.''',
On 2011/05/16 14:22:33, pastarmovj wrote:
> It might be helpful to do the math here for the poor souls that might want to
> set these by hand [1800000..86400000].

Done.

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache.cc:278:
container.device_policy_refresh_rate()));
On 2011/05/16 14:28:19, Mattias Nissler wrote:
> It seems you didn't change the .proto, so I guess this won't compile on CrOS.

Done.

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
File chrome/browser/policy/device_policy_cache_unittest.cc (right):

http://codereview.chromium.org/7014036/diff/8001/chrome/browser/policy/device...
chrome/browser/policy/device_policy_cache_unittest.cc:67:
settings.mutable_device_policy_refresh_rate()->
On 2011/05/16 14:22:33, pastarmovj wrote:
> Why don't you introduce a variable corresponding to
> settings.mutable_device_policy_refresh_rate() instead of the |settings| var
you
> don't seem to use |settings| for anything but calling
> mutable_device_policy_refresh_rate on it. This should improve greatly the
> readability of these lines.

|settings| is actually used below.

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

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

http://codereview.chromium.org/7014036/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store.cc:249: {
Value::TYPE_INTEGER, kPolicyPolicyRefreshRate,
On 2011/05/17 14:27:42, sfeuz wrote:
> On 2011/05/13 09:45:03, Mattias Nissler wrote:
> > We'll run into a problem here if we use the same policy type for both prefs:
> > Even if we have two different sources for policy values (user and device),
> we'll
> > merge them here, which is not what we want. Adding a new policy type goes
> > through the autogenerated code, but I'm not sure whether we want the device
> > policies in the JSON template. Need to discuss this with gfeher@ I guess
> 
> Added a policy for the DevicePolicyRefreshRate.

Ah, but then there is the problem that the autogenerated policy decoder will
also accept that setting for user policy, which is not what we want. You may
want to look at the generator and talk to jkummerow/gfeher who have built the
code generation infrastructure.

[sfeuz, 2011-05-30 18:53:11]

Changed to use the device_only flag for device policy refresh rate.
Refresh rate for device policy can no longer be served via user policy.

Manually tested on Desktop and CrOS.
Both device policy refresh rate and user policy refresh rate are fetched and
applied.

[Mattias Nissler (ping if slow), 2011-05-31 12:35:53]

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.cc (left):

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:88: 
Please don't delete this whitespace.

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:56: 
excess whitespace.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:179: DCHECK(pref_service);
Don't need the DCHECK, since we'll be crashing below anyway if the pointer is
NULL.

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:81: void Initialize(const char*
refresh_pref_name, int delay_milliseconds);
why are you changing the delay parameter back from int64 to int?

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:118: // The preference name of
the refresh rate we listen to.
Make the comment more accurate, like "Name of the preference to read the refresh
rate from".

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:121: PrefChangeRegistrar
pref_change_registrar_;
Why have both the name and the registrar? It seems using the IntegerPrefMember
you could just bundle both.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_connector.cc (right):

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.cc:44: 
why the added newline?

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.cc:85:
cloud_policy_subsystem_.get());
indentation

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.h:15: 
excess whitespace

[sfeuz, 2011-05-31 12:52:18]

thanks for the comments.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_subsystem.cc (left):

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:88: 
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> Please don't delete this whitespace.

Done.

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:56: 
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> excess whitespace.

Done.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:179: DCHECK(pref_service);
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> Don't need the DCHECK, since we'll be crashing below anyway if the pointer is
> NULL.

Is that really a reason to remove this DCHECK?

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:81: void Initialize(const char*
refresh_pref_name, int delay_milliseconds);
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> why are you changing the delay parameter back from int64 to int?
Not intended.
Done.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:118: // The preference name of
the refresh rate we listen to.
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> Make the comment more accurate, like "Name of the preference to read the
refresh
> rate from".

Done.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:121: PrefChangeRegistrar
pref_change_registrar_;
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> Why have both the name and the registrar? It seems using the IntegerPrefMember
> you could just bundle both.
IntegerPrefMember seems like overkill since we only listen to but never change
the preference.
|refresh_pref_name| is only used for DCHECK.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
File chrome/browser/policy/profile_policy_connector.cc (right):

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.cc:44: 
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> why the added newline?

Done.

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.cc:85:
cloud_policy_subsystem_.get());
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> indentation

Done.

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/profi...
chrome/browser/policy/profile_policy_connector.h:15: 
On 2011/05/31 12:35:53, Mattias Nissler wrote:
> excess whitespace

Done.

[Mattias Nissler (ping if slow), 2011-05-31 13:01:05]

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:179: DCHECK(pref_service);
On 2011/05/31 12:52:18, sfeuz wrote:
> On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > Don't need the DCHECK, since we'll be crashing below anyway if the pointer
is
> > NULL.
> 
> Is that really a reason to remove this DCHECK?

Yes. The reasoning is that the DCHECK doesn't give you anything an actual crash
wouldn't, so the DCHECK is just redundant.

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

http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.h:121: PrefChangeRegistrar
pref_change_registrar_;
On 2011/05/31 12:52:18, sfeuz wrote:
> On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > Why have both the name and the registrar? It seems using the
IntegerPrefMember
> > you could just bundle both.
> IntegerPrefMember seems like overkill since we only listen to but never change
> the preference.
> |refresh_pref_name| is only used for DCHECK.

I don't see why IntegerPrefMember would be overkill. I think it encapsulates
nicely the aspects of observation and access to a pref, and actually saves you a
few lines of code, so why not use it?

[Mattias Nissler (ping if slow), 2011-06-06 13:47:58]

Seems like the code hasn't been updated since the last round of discussions?

On 2011/05/31 13:01:05, Mattias Nissler wrote:
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> File chrome/browser/policy/cloud_policy_subsystem.cc (right):
> 
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> chrome/browser/policy/cloud_policy_subsystem.cc:179: DCHECK(pref_service);
> On 2011/05/31 12:52:18, sfeuz wrote:
> > On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > > Don't need the DCHECK, since we'll be crashing below anyway if the pointer
> is
> > > NULL.
> > 
> > Is that really a reason to remove this DCHECK?
> 
> Yes. The reasoning is that the DCHECK doesn't give you anything an actual
crash
> wouldn't, so the DCHECK is just redundant.
> 
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> File chrome/browser/policy/cloud_policy_subsystem.h (right):
> 
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> chrome/browser/policy/cloud_policy_subsystem.h:121: PrefChangeRegistrar
> pref_change_registrar_;
> On 2011/05/31 12:52:18, sfeuz wrote:
> > On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > > Why have both the name and the registrar? It seems using the
> IntegerPrefMember
> > > you could just bundle both.
> > IntegerPrefMember seems like overkill since we only listen to but never
change
> > the preference.
> > |refresh_pref_name| is only used for DCHECK.
> 
> I don't see why IntegerPrefMember would be overkill. I think it encapsulates
> nicely the aspects of observation and access to a pref, and actually saves you
a
> few lines of code, so why not use it?

[sfeuz, 2011-06-06 14:20:15]

Sorry I got confused about the state of this CL.

Uploaded another round and removed unintended changes in testserver and
addressed your DCHECK-comment.



On 2011/05/31 13:01:05, Mattias Nissler wrote:
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> File chrome/browser/policy/cloud_policy_subsystem.cc (right):
> 
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> chrome/browser/policy/cloud_policy_subsystem.cc:179: DCHECK(pref_service);
> On 2011/05/31 12:52:18, sfeuz wrote:
> > On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > > Don't need the DCHECK, since we'll be crashing below anyway if the pointer
> is
> > > NULL.
> > 
> > Is that really a reason to remove this DCHECK?
> 
> Yes. The reasoning is that the DCHECK doesn't give you anything an actual
crash
> wouldn't, so the DCHECK is just redundant.

Done.


http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> File chrome/browser/policy/cloud_policy_subsystem.h (right):
> 
>
http://codereview.chromium.org/7014036/diff/23001/chrome/browser/policy/cloud...
> chrome/browser/policy/cloud_policy_subsystem.h:121: PrefChangeRegistrar
> pref_change_registrar_;
> On 2011/05/31 12:52:18, sfeuz wrote:
> > On 2011/05/31 12:35:53, Mattias Nissler wrote:
> > > Why have both the name and the registrar? It seems using the
> IntegerPrefMember
> > > you could just bundle both.
> > IntegerPrefMember seems like overkill since we only listen to but never
change
> > the preference.
> > |refresh_pref_name| is only used for DCHECK.
> 
> I don't see why IntegerPrefMember would be overkill. I think it encapsulates
> nicely the aspects of observation and access to a pref, and actually saves you
a
> few lines of code, so why not use it?

Please note that we have to currently listen to two prefs, since user policy
cannot yet serve policy to local_state.

[Mattias Nissler (ping if slow), 2011-06-06 15:19:30]

LGTM with nits

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

http://codereview.chromium.org/7014036/diff/34001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:54: CloudPolicyCacheBase*
policy_cache) :
colon goes on next line.

http://codereview.chromium.org/7014036/diff/34001/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_subsystem.cc:90: int delay_milliseconds) {
nit: parameters should fit after opening ( on line  88.

[commit-bot: I haz the power, 2011-06-06 17:54:28]

Change committed as 88003