Handle policy refresh internally in ConfigurationPolicyPrefStore.

Handle policy refresh internally in ConfigurationPolicyPrefStore.

This removes the final bits of thread-switching madness from PrefValueStore and also makes sure only the PrefStores and PrefService instances that are actually affected by a policy change get reconfigured.

BUG=67715
TEST=unit tests in configuration_policy_provider_unittest.cc

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

[Mattias Nissler (ping if slow), 2010-12-21 17:31:50]

Please review!

[danno, 2010-12-22 14:00:19]

look pretty close

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/config_di...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider.cc:72: last_modification =
std::max(last_modification, file_info.last_modified);
unlrelated to this CL?

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

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:114: bool
waiting_for_initial_policies() {
const

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_membe...
File chrome/browser/prefs/pref_member_unittest.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_membe...
chrome/browser/prefs/pref_member_unittest.cc:9: #include
"chrome/common/notification_type.h"
This doesn't seem to be used, did the includes in headers change to make this
necessary?

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:10: #include
"chrome/common/notification_registrar.h"
This either? did the includes change in the headers that makes this necessary?

[Mattias Nissler (ping if slow), 2010-12-22 14:11:03]

Comments answered/addressed.

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/config_di...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/config_di...
chrome/browser/policy/config_dir_policy_provider.cc:72: last_modification =
std::max(last_modification, file_info.last_modified);
On 2010/12/22 14:00:19, danno wrote:
> unlrelated to this CL?

Yes, but fixes the problem that we reload to often. I could create a separate CL
but was too lazy, and it's related to policy refresh.

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

http://codereview.chromium.org/6074003/diff/1/chrome/browser/policy/device_ma...
chrome/browser/policy/device_management_policy_provider_unittest.cc:114: bool
waiting_for_initial_policies() {
On 2010/12/22 14:00:19, danno wrote:
> const

Done.

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_membe...
File chrome/browser/prefs/pref_member_unittest.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_membe...
chrome/browser/prefs/pref_member_unittest.cc:9: #include
"chrome/common/notification_type.h"
On 2010/12/22 14:00:19, danno wrote:
> This doesn't seem to be used, did the includes in headers change to make this
> necessary?

We pulled this in via pref_value_store.h before. It's used below for a mock
notification observer.

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_notif...
File chrome/browser/prefs/pref_notifier_impl_unittest.cc (right):

http://codereview.chromium.org/6074003/diff/1/chrome/browser/prefs/pref_notif...
chrome/browser/prefs/pref_notifier_impl_unittest.cc:10: #include
"chrome/common/notification_registrar.h"
On 2010/12/22 14:00:19, danno wrote:
> This either? did the includes change in the headers that makes this necessary?

Same story, pref_value_store.h doesn't pull it in anymore.

[danno, 2010-12-23 09:39:04]

lgtm

[Mattias Nissler (ping if slow), 2010-12-23 11:33:54]

I rebased after battre committed his latest change, can you please recheck
whether this is still lgtm?

[danno, 2010-12-23 12:16:33]

slgtm with a nit

http://codereview.chromium.org/6074003/diff/19001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/6074003/diff/19001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:81: // Remove the
preferences found in the map from |prefs_|.  Returns true if
nit: comment indentation

[Dan Beam, 2016-09-29 01:50:00]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-09-29 01:50:01]

https://codereview.chromium.org/6074003/diff/4030/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

https://codereview.chromium.org/6074003/diff/4030/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:391: if
(value->GetAsBoolean(&disable_sync) && disable_sync)
mnissler@: why is this pref not being set regardless of truthiness?  i.e. why
shouldn't this be set when !disable_sync as well?

[Mattias Nissler (ping if slow), 2016-09-29 08:00:34]

https://codereview.chromium.org/6074003/diff/4030/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

https://codereview.chromium.org/6074003/diff/4030/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:391: if
(value->GetAsBoolean(&disable_sync) && disable_sync)
On 2016/09/29 01:50:01, Dan Beam wrote:
> mnissler@: why is this pref not being set regardless of truthiness?  i.e. why
> shouldn't this be set when !disable_sync as well?

When I wrote this code, it conceptually didn't make sense for an admin to
force-enable sync. Which would be implied by an admin configuring
kPolicySyncDisabled = false. Has that changed now? Why are you asking?

For background, the general idea about policy-controlled prefs is that they
provide overrides expression the constraints imposed by the admin. If the admin
doesn't care, it's left up to the user to make a decision. That said,
prefs::kSyncManaged is a degenerate case, since this pref was introduced for the
sole purpose of communicating the policy setting to the sync code, there's
actually no way for the user to manipulate this pref.