Add ManagedUserSettingsService and a SupervisedUserPrefStore using it.

Add ManagedUserSettingsService and a SupervisedUserPrefStore using it.

BUG=280674
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221410

[Bernhard Bauer, 2013-08-29 14:14:51]

Pam: Please review.

Mattias, Bartosz: FYI.

[Pam (message me for reviews), 2013-08-30 13:35:53]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings.cc:16: 
extra

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings.h:6: #define
CHROME_BROWSER_MANAGED_MODE_MANAGED_USER_SETTINGS_H_
Hm, maybe chnage this to managed_user_constants.h, allowing it to be used for
other things in the future if needed.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:23: // A callback
with all managed user settings. If the dictionary is NULL, it
What does "with" mean here?

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: void
Subscribe(const SettingsCallback& callback);
Do we really need Yet Another Settings Notifier? And if we do, should it use Add
rather than Subscribe, to match PrefChangeRegistrar? And the notification model,
rather than a callback?

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:38: // Sends the
settings to all subscribers.
What triggers this? Sync?

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:39: void
NotifySettingsChanged();
If it does keep using callbacks, this should be renamed; it's misleading now.
(As is the comment.)

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:42: // Whether
managed user settings are available.
What makes them available?

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:49:
scoped_ptr<base::DictionaryValue> GetSettingsIfActive();
Comment please. (Mainly that it returns an empty dict if not active.)

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:20: : weak_ptr_factory_(this)
{
I'd suggest additional indentation here.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:45: return prefs_;
This works, but it makes me itch. It could also lead someone to overlook the
return type. I'd prefer
return (prefs_ != NULL);

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:76: // Copy managed user
settings to prefs.
Try a map, like ExtensionPrefStore and others use, to make this more extensible.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.h:22: //
ManagedUserSettingsService passed in at construction.
And that gets -- will get -- its settings from the ManagedModePolicyProvider?
This seems rather more complicated, more layers, than necessary. Although maybe
I just need a clear picture of how it interrelates to see why it's all needed.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.h:39: void
CopySettingsValue(const base::DictionaryValue* settings,
Comment please

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store_unittest.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store_unittest.cc:174: //
kManagedModeManualHosts can be configured by the custodian, overriding the
kForceSafeSearch

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store_unittest.cc:209: 
extra blank lines

[Bernhard Bauer, 2013-08-30 14:54:27]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings.cc:16: 
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> extra

Removed.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings.h:6: #define
CHROME_BROWSER_MANAGED_MODE_MANAGED_USER_SETTINGS_H_
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> Hm, maybe chnage this to managed_user_constants.h, allowing it to be used for
> other things in the future if needed.

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:23: // A callback
with all managed user settings. If the dictionary is NULL, it
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> What does "with" mean here?

It's the first parameter. "A callback whose first parameter is..." seemed
awkward to me.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: void
Subscribe(const SettingsCallback& callback);
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> Do we really need Yet Another Settings Notifier? 

You mean in general, do we need a mechanism for the pref store to be notified
about changes to the managed user settings? I think so; separating the actual
syncing part from the part that makes preferences out of the settings was one of
the goals of this refactoring.

> And if we do, should it use Add
> rather than Subscribe, to match PrefChangeRegistrar?

I used Subscribe because it also fires the first time settings are available,
not only when they change. [...]

> And the notification model,
> rather than a callback?

Well, notifications in the NotificationService sense are deprecated, and using a
callback seemed easier to me than defining a single-method interface. It also
means we can use weak pointers instead of having to be notified when the
ManagedUserSettingsService goes away.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:38: // Sends the
settings to all subscribers.
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> What triggers this? Sync?

Yes. From the point of view of this class, the (not yet landed) subclass will. I
expanded the comment.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:39: void
NotifySettingsChanged();
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> If it does keep using callbacks, this should be renamed; it's misleading now.
> (As is the comment.)

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:42: // Whether
managed user settings are available.
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> What makes them available?

The subclass has to load them from a Json file on disk.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:49:
scoped_ptr<base::DictionaryValue> GetSettingsIfActive();
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> Comment please. (Mainly that it returns an empty dict if not active.)

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:20: : weak_ptr_factory_(this)
{
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> I'd suggest additional indentation here.

Like what? Note that this is also how clang-format does it.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:45: return prefs_;
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> This works, but it makes me itch. It could also lead someone to overlook the
> return type. I'd prefer
> return (prefs_ != NULL);

Well, it'd need to be `prefs_.get() != NULL`. If it's just about making the
conversion to bool implicit, how about `!!prefs_`?

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:76: // Copy managed user
settings to prefs.
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> Try a map, like ExtensionPrefStore and others use, to make this more
extensible.

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.h:22: //
ManagedUserSettingsService passed in at construction.
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> And that gets -- will get -- its settings from the ManagedModePolicyProvider?

No, ManagedModePolicyProvider will go away in favor of
ManagedUserSettingsSyncService (I know, awesome name).

> This seems rather more complicated, more layers, than necessary. Although
maybe
> I just need a clear picture of how it interrelates to see why it's all needed.

The final CL is at https://codereview.chromium.org/23376002/. We add one
additional layer on the way from Sync to preferences, because we separate the
SyncableService from the class that maps settings to preferences.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.h:39: void
CopySettingsValue(const base::DictionaryValue* settings,
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> Comment please

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store_unittest.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store_unittest.cc:174: //
kManagedModeManualHosts can be configured by the custodian, overriding the
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> kForceSafeSearch

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store_unittest.cc:209: 
On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> extra blank lines

Removed.

[Mattias Nissler (ping if slow), 2013-09-02 08:12:54]

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:12: #include
"chrome/browser/managed_mode/managed_user_settings_service.h"
chrome/browser/prefs/ should ideally avoid dependencies to other browser code.
Note that configuration_policy_pref_store.{h,cc} is in chrome/browser/policy, I
suggest you do the same for SupervisedUserPrefStore.

[Bernhard Bauer, 2013-09-02 09:27:31]

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:12: #include
"chrome/browser/managed_mode/managed_user_settings_service.h"
On 2013/09/02 08:12:54, Mattias Nissler wrote:
> chrome/browser/prefs/ should ideally avoid dependencies to other browser code.
> Note that configuration_policy_pref_store.{h,cc} is in chrome/browser/policy,
I
> suggest you do the same for SupervisedUserPrefStore.

Done.

Are there plans to extract chrome/browser/prefs into a component and/or add a
DEPS rule to enforce this?

[Mattias Nissler (ping if slow), 2013-09-02 09:29:23]

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/40001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:12: #include
"chrome/browser/managed_mode/managed_user_settings_service.h"
On 2013/09/02 09:27:31, Bernhard Bauer wrote:
> On 2013/09/02 08:12:54, Mattias Nissler wrote:
> > chrome/browser/prefs/ should ideally avoid dependencies to other browser
code.
> > Note that configuration_policy_pref_store.{h,cc} is in
chrome/browser/policy,
> I
> > suggest you do the same for SupervisedUserPrefStore.
> 
> Done.
> 
> Are there plans to extract chrome/browser/prefs into a component and/or add a
> DEPS rule to enforce this?

Adding a DEPS rule would make sense. Note that components/user_prefs already
exists.

[Pam (message me for reviews), 2013-09-02 09:38:41]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:23: // A callback
with all managed user settings. If the dictionary is NULL, it
On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > What does "with" mean here?
> 
> It's the first parameter. "A callback whose first parameter is..." seemed
> awkward to me.

A little bulky, but it would be clearer.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: void
Subscribe(const SettingsCallback& callback);
> > Do we really need Yet Another Settings Notifier? 
> 
> You mean in general, do we need a mechanism for the pref store to be notified
> about changes to the managed user settings? I think so; separating the actual
> syncing part from the part that makes preferences out of the settings was one
of
> the goals of this refactoring.

Will anything besides the PrefStore be subscribing to changes? Will there be
more than one subclass of this? If not, I don't see it as worthwhile to create
an abstract interface for only one implementer and one subscriber.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:39: void
NotifySettingsChanged();
On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > If it does keep using callbacks, this should be renamed; it's misleading
now.
> > (As is the comment.)
> 
> Done.

I meant to avoid the word "notify", with all its NotificationService
implications. Maybe the painfully literal "CallSubscriberCallbacks"? Or
InformSubscribers, if that's too awkward or too specific.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:20: : weak_ptr_factory_(this)
{
On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > I'd suggest additional indentation here.
> 
> Like what? Note that this is also how clang-format does it.

Well, OK. IIRC this case is specifically called out as open to different styles
in the guide, but I'm OK with sticking to clang-format's way.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:45: return prefs_;
On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > This works, but it makes me itch. It could also lead someone to overlook the
> > return type. I'd prefer
> > return (prefs_ != NULL);
> 
> Well, it'd need to be `prefs_.get() != NULL`. If it's just about making the
> conversion to bool implicit, how about `!!prefs_`?

Works for me.

[Bernhard Bauer, 2013-09-02 11:01:07]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:23: // A callback
with all managed user settings. If the dictionary is NULL, it
On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> > On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > > What does "with" mean here?
> > 
> > It's the first parameter. "A callback whose first parameter is..." seemed
> > awkward to me.
> 
> A little bulky, but it would be clearer.

Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: void
Subscribe(const SettingsCallback& callback);
On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> > > Do we really need Yet Another Settings Notifier? 
> > 
> > You mean in general, do we need a mechanism for the pref store to be
notified
> > about changes to the managed user settings? I think so; separating the
actual
> > syncing part from the part that makes preferences out of the settings was
one
> of
> > the goals of this refactoring.
> 
> Will anything besides the PrefStore be subscribing to changes? Will there be
> more than one subclass of this? If not, I don't see it as worthwhile to create
> an abstract interface for only one implementer and one subscriber.

The only other implementation is in tests. Testing is in fact the reason I added
the abstract interface here.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:39: void
NotifySettingsChanged();
On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> > On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > > If it does keep using callbacks, this should be renamed; it's misleading
> now.
> > > (As is the comment.)
> > 
> > Done.
> 
> I meant to avoid the word "notify", with all its NotificationService
> implications. Maybe the painfully literal "CallSubscriberCallbacks"? Or
> InformSubscribers, if that's too awkward or too specific.

I like InformSubscribers! Done.

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
File chrome/browser/prefs/supervised_user_pref_store.cc (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/prefs/supe...
chrome/browser/prefs/supervised_user_pref_store.cc:45: return prefs_;
On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> > On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > > This works, but it makes me itch. It could also lead someone to overlook
the
> > > return type. I'd prefer
> > > return (prefs_ != NULL);
> > 
> > Well, it'd need to be `prefs_.get() != NULL`. If it's just about making the
> > conversion to bool implicit, how about `!!prefs_`?
> 
> Works for me.

Done.

[Pam (message me for reviews), 2013-09-03 10:45:31]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:39: void
NotifySettingsChanged();
On 2013/09/02 11:01:07, Bernhard Bauer wrote:
> On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> > On 2013/08/30 14:54:27, Bernhard Bauer wrote:
> > > On 2013/08/30 13:35:53, Pam (also PM for reviews) wrote:
> > > > If it does keep using callbacks, this should be renamed; it's misleading
> > now.
> > > > (As is the comment.)
> > > 
> > > Done.
> > 
> > I meant to avoid the word "notify", with all its NotificationService
> > implications. Maybe the painfully literal "CallSubscriberCallbacks"? Or
> > InformSubscribers, if that's too awkward or too specific.
> 
> I like InformSubscribers! Done.

CallSubscribersBack :)
Sorry, that's the cold meds talking.

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:17: // Defines an
abstract base class for a service that stores managed user
It doesn't just store the settings -- that would be more the job of the
PrefStore. Its main purpose is to keep track of the meta-state of the settings
and inform the PrefStore. Or something like that.

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/supervised_user_pref_store.h (right):

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
chrome/browser/managed_mode/supervised_user_pref_store.h:5: #ifndef
CHROME_BROWSER_MANAGED_MODE_SUPERVISED_USER_PREF_STORE_H_
Didn't we decide to keep all the code named with Managed User instead of
Supervised User?

[Bernhard Bauer, 2013-09-03 10:53:43]

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:17: // Defines an
abstract base class for a service that stores managed user
On 2013/09/03 10:45:32, Pam (also PM for reviews) wrote:
> It doesn't just store the settings -- that would be more the job of the
> PrefStore.

Well, it does store the settings in the sense that it is the authoritative
source of managed user settings. The PrefStore really just translates them from
internal keys to preferences.

> Its main purpose is to keep track of the meta-state of the settings
> and inform the PrefStore. Or something like that.

Mmm, meta-state... :)

But yeah, this is going away anyway.

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/supervised_user_pref_store.h (right):

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
chrome/browser/managed_mode/supervised_user_pref_store.h:5: #ifndef
CHROME_BROWSER_MANAGED_MODE_SUPERVISED_USER_PREF_STORE_H_
On 2013/09/03 10:45:32, Pam (also PM for reviews) wrote:
> Didn't we decide to keep all the code named with Managed User instead of
> Supervised User?

That created confusion in PrefValueStore, where we have managed preferences,
user preferences, and then managed user preferences, which are not related to
the former two.

I'm happy about other suggestions; One possibility would be to keep the term
"supervised user" in pure prefs code (i.e. base/prefs), and only use "managed
user" in managed_mode?

[bartfab (slow), 2013-09-03 11:16:07]

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/supervised_user_pref_store.h (right):

https://codereview.chromium.org/23466004/diff/61001/chrome/browser/managed_mo...
chrome/browser/managed_mode/supervised_user_pref_store.h:5: #ifndef
CHROME_BROWSER_MANAGED_MODE_SUPERVISED_USER_PREF_STORE_H_
On 2013/09/03 10:53:43, Bernhard Bauer wrote:
> On 2013/09/03 10:45:32, Pam (also PM for reviews) wrote:
> > Didn't we decide to keep all the code named with Managed User instead of
> > Supervised User?
> 
> That created confusion in PrefValueStore, where we have managed preferences,
> user preferences, and then managed user preferences, which are not related to
> the former two.
> 
> I'm happy about other suggestions; One possibility would be to keep the term
> "supervised user" in pure prefs code (i.e. base/prefs), and only use "managed
> user" in managed_mode?

My vote goes to using "supervised user" in all new code. That way, there is a
single term going forward. And that term is consistent with the UI. Existing
code can slowly switch over to the new naming as it gets obsoleted and
refactored.

This is what we did for public sessions (formerly known as public accounts) and
it worked well for us.

[Pam (message me for reviews), 2013-09-03 12:57:17]

On 2013/09/03 10:53:43, Bernhard Bauer wrote:
> > Didn't we decide to keep all the code named with Managed User instead of
> > Supervised User?
> 
> That created confusion in PrefValueStore, where we have managed preferences,
> user preferences, and then managed user preferences, which are not related to
> the former two.

Oh, sure. In that case I withdraw my suggestion. In the long run I'd like to
move everything over to "supervised user" anyway, so here where it's needed to
avoid confusion is as good a place as any to start. Maybe a brief comment at the
top explaining for now that it's related to "managed user" stuff, though?

[Bernhard Bauer, 2013-09-04 09:29:04]

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/23001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: void
Subscribe(const SettingsCallback& callback);
On 2013/09/02 09:38:41, Pam (also PM for reviews) wrote:
> > > Do we really need Yet Another Settings Notifier? 
> > 
> > You mean in general, do we need a mechanism for the pref store to be
notified
> > about changes to the managed user settings? I think so; separating the
actual
> > syncing part from the part that makes preferences out of the settings was
one
> of
> > the goals of this refactoring.
> 
> Will anything besides the PrefStore be subscribing to changes? Will there be
> more than one subclass of this? If not, I don't see it as worthwhile to create
> an abstract interface for only one implementer and one subscriber.

Removed the abstract interface. The syncing code is not in this CL yet, to keep
it smaller.

[Pam (message me for reviews), 2013-09-04 10:21:46]

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_constants.h (right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_constants.h:10: extern const char
kContentPackDefaultFilteringBehavior[];
Comments describing these constants, please, especially since the filename
doesn't say they're all settings anymore. And even if it did, "setting" isn't
very specific; mention what subsystem uses these as keys.

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: // preferences.
The downloaded settings are persisted in a PrefStore.
It would be helpful to mention here too that this "storage" PrefStore isn't
connected to the PrefService.

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service_unittest.cc
(right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service_unittest.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
2013

[Bernhard Bauer, 2013-09-04 11:56:19]

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_constants.h (right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_constants.h:10: extern const char
kContentPackDefaultFilteringBehavior[];
On 2013/09/04 10:21:47, Pam (also PM for reviews) wrote:
> Comments describing these constants, please, especially since the filename
> doesn't say they're all settings anymore. And even if it did, "setting" isn't
> very specific; mention what subsystem uses these as keys.

Done.

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service.h (right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service.h:29: // preferences.
The downloaded settings are persisted in a PrefStore.
On 2013/09/04 10:21:47, Pam (also PM for reviews) wrote:
> It would be helpful to mention here too that this "storage" PrefStore isn't
> connected to the PrefService.

Done.

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_settings_service_unittest.cc
(right):

https://codereview.chromium.org/23466004/diff/54001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_settings_service_unittest.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2013/09/04 10:21:47, Pam (also PM for reviews) wrote:
> 2013

Done (although I think the commit queue fixes that up automatically).

[Pam (message me for reviews), 2013-09-05 10:15:28]

lgtm

[commit-bot: I haz the power, 2013-09-05 11:50:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23466004/127001

[commit-bot: I haz the power, 2013-09-05 12:31:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 13:43:24]

Change committed as 221410