Add ManagedUserSharedSettingsService.

Add ManagedUserSharedSettingsService.

ManagedUserSharedSettingsService syncs settings that can be modified/accessed both by a supervised user and their manager (for example, the avatar).

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

[Bernhard Bauer, 2014-01-03 16:16:31]

Please review.

Denis: FYI.

[Pam (message me for reviews), 2014-01-08 10:30:40]

It'd be nice to add one example of a shared setting type to the log description.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:56:
DCHECK(id.empty() || id == mu_id);
Suggest also checking that the mu_id is not empty.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:131: if
(!data->GetDictionaryWithoutPathExpansion(mu_id, &dict))
It would be worth documenting these three nested dictionaries in the header.
e.g., the need for a innermost level to hold "value" and "acknowledged" isn't
self-evident to someone not familiar with the setup.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:196:
std::map<std::string, std::set<std::string> > seen_keys;
How 'bout some inline comments?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:296: // For
an update action, the managed user should already exist.
This comment is backwards from the logic; i.e., it's really checking "if the MU
already exists, this should be an update action".

I am led to wonder why it matters (why sync cares), but that's outside the scope
of this review.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service.h (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:33: typedef
base::Callback<void(const std::string& /* mu_id */,
A comment saying that this is called with settings changes, and perhaps renaming
to SettingsChangeCallback or ChangeCallback or similar, would be good.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:39: // to get
an instance of this service.
It's not clear from this comment whether the second sentence depends on the
first, or is its counterpoint. (That is, whether it applies only when testing,
or only when not, or both.)

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:46: //
|mu_id|.
For complete clarity, something like

Returns the value for the given |key| and the supervised user identified by
|mu_id|. If the profile that owns this service belongs to a supervised
user, |mu_id| must be their own. If either the supervised user or the key does
not exist, or the current profile is a supervised user and the |mu_id| doesn't
match, NULL is returned.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:65: void
SetValueInternal(const std::string& mu_id,
I remember seeing some announcement about naming public-for-testing methods with
...ForTesting or similar, and a presubmit check that enforced that usage. Am I
dreaming?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h:31:
};
Do you need to override GetBrowserContextToUse, to handle incognito?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc:57:
return syncer::SyncError();
Is it worth testing the behavior when ProcessSyncChanges returns an error?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update.cc (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update.cc:39:
callback_.Run(value->Equals(value_.get()));
And also make sure acknowledged = true? Or what's the point of |acknowledged|?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update.h (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update.h:23: // flipped
the acknowledgement flag for the setting. If another client changes
flips

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc:63:
base::StringValue("Barney Gumble"),
Please add a comment pointing this out as the failure point.

https://codereview.chromium.org/123293003/diff/120001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/common/pref_name...
chrome/common/pref_names.cc:50: // XXX
XXX

[Bernhard Bauer, 2014-01-08 11:42:04]

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:56:
DCHECK(id.empty() || id == mu_id);
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> Suggest also checking that the mu_id is not empty.

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:131: if
(!data->GetDictionaryWithoutPathExpansion(mu_id, &dict))
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> It would be worth documenting these three nested dictionaries in the header.
> e.g., the need for a innermost level to hold "value" and "acknowledged" isn't
> self-evident to someone not familiar with the setup.

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:196:
std::map<std::string, std::set<std::string> > seen_keys;
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> How 'bout some inline comments?

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.cc:296: // For
an update action, the managed user should already exist.
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> This comment is backwards from the logic; i.e., it's really checking "if the
MU
> already exists, this should be an update action".
> 
> I am led to wonder why it matters (why sync cares), but that's outside the
scope
> of this review.

It's a sanity check to make sure that the copy of the data that we have in the
Sync database stays in Sync (ha!) with the copy in Preferences.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service.h (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:33: typedef
base::Callback<void(const std::string& /* mu_id */,
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> A comment saying that this is called with settings changes, and perhaps
renaming
> to SettingsChangeCallback or ChangeCallback or similar, would be good.

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:39: // to get
an instance of this service.
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> It's not clear from this comment whether the second sentence depends on the
> first, or is its counterpoint. (That is, whether it applies only when testing,
> or only when not, or both.)

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:46: //
|mu_id|.
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> For complete clarity, something like
> 
> Returns the value for the given |key| and the supervised user identified by
> |mu_id|. If the profile that owns this service belongs to a supervised
> user, |mu_id| must be their own. If either the supervised user or the key does
> not exist, or the current profile is a supervised user and the |mu_id| doesn't
> match, NULL is returned.

Well, the "must be their own" part isn't entirely true; a user can ask for
values for any ID they want, but a supervised user will only get values for
their own ID.

I updated the comment a bit; let me know if it's OK now.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:65: void
SetValueInternal(const std::string& mu_id,
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> I remember seeing some announcement about naming public-for-testing methods
with
> ...ForTesting or similar, and a presubmit check that enforced that usage. Am I
> dreaming?

There is indeed such a presubmit check, but it is for code that only exists for
testing. This method is used in production, but just inside of the class. I
figured adding a ...ForTesting method that just forwards to ...Internal would be
a bit much.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h:31:
};
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> Do you need to override GetBrowserContextToUse, to handle incognito?

I don't think so. By default, the service will be NULL for an incognito profile,
which should be fine, right?

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc:57:
return syncer::SyncError();
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> Is it worth testing the behavior when ProcessSyncChanges returns an error?

Not sure; in SetValueInternal we DCHECK that there is no error, and in
MergeDataAndStartSyncing we just pass the error returned by ProcessSyncChanges
on.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update.cc (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update.cc:39:
callback_.Run(value->Equals(value_.get()));
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> And also make sure acknowledged = true? Or what's the point of |acknowledged|?

Yes, I meant to DCHECK that in the service.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update.h (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update.h:23: // flipped
the acknowledgement flag for the setting. If another client changes
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> flips

Well, technically it happens a little bit afterwards :) I clarified the comment.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc:63:
base::StringValue("Barney Gumble"),
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> Please add a comment pointing this out as the failure point.

Done.

https://codereview.chromium.org/123293003/diff/120001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/common/pref_name...
chrome/common/pref_names.cc:50: // XXX
On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> XXX

Fixed.

[Pam (message me for reviews), 2014-01-08 12:19:28]

LGTM, but see comments

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service.h (right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service.h:65: void
SetValueInternal(const std::string& mu_id,
On 2014/01/08 11:42:04, Bernhard Bauer wrote:
> On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> > I remember seeing some announcement about naming public-for-testing methods
> with
> > ...ForTesting or similar, and a presubmit check that enforced that usage. Am
I
> > dreaming?
> 
> There is indeed such a presubmit check, but it is for code that only exists
for
> testing. This method is used in production, but just inside of the class. I
> figured adding a ...ForTesting method that just forwards to ...Internal would
be
> a bit much.

Agreed. I reviewed the .h before the .cc and didn't go back after seeing the
method was used internally too; sorry.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h:31:
};
On 2014/01/08 11:42:04, Bernhard Bauer wrote:
> On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> > Do you need to override GetBrowserContextToUse, to handle incognito?
> 
> I don't think so. By default, the service will be NULL for an incognito
profile,
> which should be fine, right?

For the currently planned settings, yes, but I could see it being confusing if
some other setting is added later, especially since the non-shared settings
forward from Incognito to normal.

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_unittest.cc:57:
return syncer::SyncError();
On 2014/01/08 11:42:04, Bernhard Bauer wrote:
> On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> > Is it worth testing the behavior when ProcessSyncChanges returns an error?
> 
> Not sure; in SetValueInternal we DCHECK that there is no error, and in
> MergeDataAndStartSyncing we just pass the error returned by ProcessSyncChanges
> on.

It might be nice to verify that MergeDayaAndStartSyncing doesn't explode or
anything, but I guess it'll do as is.

https://codereview.chromium.org/123293003/diff/370001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/370001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc:57:
// Syncing down another change will cause the update to fail.
A different change, right? Because of the different setting value.

[Bernhard Bauer, 2014-01-08 12:59:41]

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h
(right):

https://codereview.chromium.org/123293003/diff/120001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_service_factory.h:31:
};
On 2014/01/08 12:19:28, Pam (also PM for reviews) wrote:
> On 2014/01/08 11:42:04, Bernhard Bauer wrote:
> > On 2014/01/08 10:30:40, Pam (also PM for reviews) wrote:
> > > Do you need to override GetBrowserContextToUse, to handle incognito?
> > 
> > I don't think so. By default, the service will be NULL for an incognito
> profile,
> > which should be fine, right?
> 
> For the currently planned settings, yes, but I could see it being confusing if
> some other setting is added later, especially since the non-shared settings
> forward from Incognito to normal.

I'm not even sure anymore why we do that. For shared settings, if a client in
the future will want to get a ManagedUserSharedSettingsService for an incognito
profile, it can add the redirection. If we want to make it consistent with the
non-shared version, I'd be in favor of removing the redirection there.

https://codereview.chromium.org/123293003/diff/370001/chrome/browser/managed_...
File chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc
(right):

https://codereview.chromium.org/123293003/diff/370001/chrome/browser/managed_...
chrome/browser/managed_mode/managed_user_shared_settings_update_unittest.cc:57:
// Syncing down another change will cause the update to fail.
On 2014/01/08 12:19:28, Pam (also PM for reviews) wrote:
> A different change, right? Because of the different setting value.

Right, done.

[Pam (message me for reviews), 2014-01-08 14:22:52]

> > > > Do you need to override GetBrowserContextToUse, to handle incognito?
> > > 
> > > I don't think so. By default, the service will be NULL for an incognito
> > > profile, which should be fine, right?
> > 
> > For the currently planned settings, yes, but I could see it being confusing
if
> > some other setting is added later, especially since the non-shared settings
> > forward from Incognito to normal.
> 
> I'm not even sure anymore why we do that. For shared settings, if a client in
> the future will want to get a ManagedUserSharedSettingsService for an
incognito
> profile, it can add the redirection. If we want to make it consistent with the
> non-shared version, I'd be in favor of removing the redirection there.

Sounds good to me. I also can't think of why we'd need a settings sync service
in Incognito. But in a separate change.

[Bernhard Bauer, 2014-01-08 18:06:24]

Nicolas, could I get an OWNERS review?

[Nicolas Zea, 2014-01-08 19:33:01]

sync lgtm

[commit-bot: I haz the power, 2014-01-08 19:49:04]

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

[commit-bot: I haz the power, 2014-01-08 22:46:19]

Change committed as 243680