Add a HostContentSettingsMap layer for Supervised Users.

Add a HostContentSettingsMap layer for Supervised Users.

Added a layer for Supervised Users in the HostContentSettingsMap.
As this only needs functionality to block the entire content setting
(for now), it is similar to the existing OverrideProvider.
Reorganized the common code between the two providers.
Updated the helper methods in PrefServiceBridge to check the
HostContentSettingsMap instead of Preferences when checking whether it
is managed by the custodian.

BUG=455646
BUG=455640
Committed: https://crrev.com/95221ffe02b9d0782202d80cf86766686afab13a
Cr-Commit-Position: refs/heads/master@{#319121}

[knn, 2015-02-05 11:41:27]

knn@chromium.org changed reviewers:
+ bauerb@chromium.org

[knn, 2015-02-05 11:41:28]

PTAL.

[Bernhard Bauer, 2015-02-05 12:07:52]

https://codereview.chromium.org/902833003/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_constants.h (left):

https://codereview.chromium.org/902833003/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_constants.h:12: extern const char
kCameraMicAllowed[];
So the old value was never used in the SupervisedUserPrefStore, i.e. this never
actually worked on trunk?

https://codereview.chromium.org/902833003/diff/1/components/content_settings....
File components/content_settings.gypi (right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings....
components/content_settings.gypi:30:
'content_settings/core/browser/content_settings_binary_value_map.cc',
Keep this alphabetically ordered; "binary" comes before "client".

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File
components/content_settings/core/browser/content_settings_binary_value_map.h
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_binary_value_map.h:13:
class AutoLock;
Nit: The empty lines around this line aren't necessary.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_binary_value_map.h:28:
RuleIterator* GetRuleIterator(ContentSettingsType content_type,
Make this return a scoped_ptr?

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File
components/content_settings/core/browser/content_settings_supervised_provider.cc
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:40:
content_settings::BinaryValueMap& value_map,
Don't use non-const references. If you want to change the map, pass a pointer to
it so it's obvious.

Also, any reason why this isn't a method?

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:104:
// methodes of the ProviderInterface that set or delete any settings do nothing.
Nit: "methods"

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:124:
prefs_ = NULL;
Use nullptr instead of NULL.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:129:
scoped_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
You can just put this into a nested scope that ends right before you call
NotifyObservers().

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/host_content_settings_map.cc:41:
"supervised",
"supervised_user" maybe?

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File components/content_settings/core/common/pref_names.cc (right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/common/pref_names.cc:81: const char
kSupervisedGeolocationDisabled[] =
As previously discussed offline, I'm not sure if going through prefs here makes
anything easier. You could just as well directly observe the
SupervisedUserSettingsService instead.

[knn, 2015-02-26 12:07:30]

PTAL.

https://codereview.chromium.org/902833003/diff/1/chrome/browser/supervised_us...
File chrome/browser/supervised_user/supervised_user_constants.h (left):

https://codereview.chromium.org/902833003/diff/1/chrome/browser/supervised_us...
chrome/browser/supervised_user/supervised_user_constants.h:12: extern const char
kCameraMicAllowed[];
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> So the old value was never used in the SupervisedUserPrefStore, i.e. this
never
> actually worked on trunk?

I accidentally reverted the file before submitting the original change (after a
bad 'git cl format').
Did not realize that until I worked on this change. I'll make sure I check for
all files in the future.

https://codereview.chromium.org/902833003/diff/1/components/content_settings....
File components/content_settings.gypi (right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings....
components/content_settings.gypi:30:
'content_settings/core/browser/content_settings_binary_value_map.cc',
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> Keep this alphabetically ordered; "binary" comes before "client".

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File
components/content_settings/core/browser/content_settings_binary_value_map.h
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_binary_value_map.h:13:
class AutoLock;
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> Nit: The empty lines around this line aren't necessary.

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_binary_value_map.h:28:
RuleIterator* GetRuleIterator(ContentSettingsType content_type,
On 2015/02/05 12:07:51, Bernhard (back on Feb 26) wrote:
> Make this return a scoped_ptr?

This is a ProviderInterface implementation. Needs to refactored in other places.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File
components/content_settings/core/browser/content_settings_supervised_provider.cc
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:40:
content_settings::BinaryValueMap& value_map,
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> Don't use non-const references. If you want to change the map, pass a pointer
to
> it so it's obvious.
> 
> Also, any reason why this isn't a method?

Done. This isn't a method as I don't want to expose the ugly
PrefsForSupervisedContentSettingsMapEntry and the actual mapping itself.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:104:
// methodes of the ProviderInterface that set or delete any settings do nothing.
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> Nit: "methods"

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:124:
prefs_ = NULL;
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> Use nullptr instead of NULL.

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_supervised_provider.cc:129:
scoped_ptr<base::AutoLock> auto_lock(new base::AutoLock(lock_));
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> You can just put this into a nested scope that ends right before you call
> NotifyObservers().

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/host_content_settings_map.cc:41:
"supervised",
On 2015/02/05 12:07:51, Bernhard Bauer wrote:
> "supervised_user" maybe?

Done.

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File components/content_settings/core/common/pref_names.cc (right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/common/pref_names.cc:81: const char
kSupervisedGeolocationDisabled[] =
On 2015/02/05 12:07:52, Bernhard Bauer wrote:
> As previously discussed offline, I'm not sure if going through prefs here
makes
> anything easier. You could just as well directly observe the
> SupervisedUserSettingsService instead.

Done.

[Bernhard Bauer, 2015-02-26 12:44:58]

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
File
components/content_settings/core/browser/content_settings_binary_value_map.h
(right):

https://codereview.chromium.org/902833003/diff/1/components/content_settings/...
components/content_settings/core/browser/content_settings_binary_value_map.h:28:
RuleIterator* GetRuleIterator(ContentSettingsType content_type,
On 2015/02/26 12:07:29, knn wrote:
> On 2015/02/05 12:07:51, Bernhard (back on Feb 26) wrote:
> > Make this return a scoped_ptr?
> 
> This is a ProviderInterface implementation. Needs to refactored in other
places.

Wait, this class doesn't implement an interface. If this is used by something
else that implements the interface, we can still use scoped_ptr here, and just
release it when the interface demands a raw pointer.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/android/p...
File chrome/browser/android/preferences/pref_service_bridge.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:440:
DCHECK(javascript_pref);
If |javascript_pref| is indeed null, you will crash on the next line anyway.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/android/p...
chrome/browser/android/preferences/pref_service_bridge.cc:448:
SetJavaScriptEnabled(env, obj, javascript_enabled);
Is this going to work for user-defined preferences? A default content setting
can be overridden by exceptions. Is there even a place where this pref is set on
the user level?

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.cc
(right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.cc:47: if
(!settings) return;
Return statement on new line.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_constants.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_constants.cc:15: const char
kGeolocationDisabled[] = "GeolocationDisabled";
Is the only reason we are changing these constants so that the content settings
provider is easier to implement? I would rather have a consistent style for the
settings (which is that boolean flags are positive, i.e. "allowed" and
"enabled", rather than negative), even if that means slightly more work to
implement. Policy settings historically used a mixture of positive and negative
settings, and it's super confusing.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_settings_service.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.cc:56: :
active_(false), local_settings_(new base::DictionaryValue) {
Personally, I'm okay with either keeping opening and closing brace on the same
line, or moving the closing brace to the next line, but it should be consistent
(see three lines below). If there is no strong reason to change the style, I
would keep it as it is.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.cc:116: if
(!store_) return false;
Return statement on new line.

Also, it's an error to call this method on an uninitialized object, so this
check shouldn't even exist.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_settings_service.h (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.h:83: void
RegisterContentSettings(
Why does the SupervisedUserSettingsService now have to know about the
HostContentSettingsMap? The dependencies should be the other way around:

HostContentSettingsMap (or its embedder, i.e. Profile) -> SupervisedProvider ->
SupervisedUserSettingsService

[knn, 2015-02-26 16:18:57]

You also reviewed code that I did not write in
chrome/browser/android/preferences/pref_service_bridge.cc
I should probably do that separately.

PTAL.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.cc
(right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.cc:47: if
(!settings) return;
On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> Return statement on new line.

Done.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_constants.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_constants.cc:15: const char
kGeolocationDisabled[] = "GeolocationDisabled";
On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> Is the only reason we are changing these constants so that the content
settings
> provider is easier to implement? I would rather have a consistent style for
the
> settings (which is that boolean flags are positive, i.e. "allowed" and
> "enabled", rather than negative), even if that means slightly more work to
> implement. Policy settings historically used a mixture of positive and
negative
> settings, and it's super confusing.

To the contrary, Implementation is actually easier with 'Enabled'.
However, I think Disabled reflects the intent of the preference better.
Also this conveys in a subtle way (I think) that you can only Force disable the
setting and not the other way (force enable).

What do you think about the trade offs? I am happy to change it if you think it
is not worth the in-consistency it introduces.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_settings_service.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.cc:56: :
active_(false), local_settings_(new base::DictionaryValue) {
On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> Personally, I'm okay with either keeping opening and closing brace on the same
> line, or moving the closing brace to the next line, but it should be
consistent
> (see three lines below). If there is no strong reason to change the style, I
> would keep it as it is.

Sorry about that.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.cc:116: if
(!store_) return false;
On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> Return statement on new line.
> 
> Also, it's an error to call this method on an uninitialized object, so this
> check shouldn't even exist.

Done.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_settings_service.h (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.h:83: void
RegisterContentSettings(
On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> Why does the SupervisedUserSettingsService now have to know about the
> HostContentSettingsMap? The dependencies should be the other way around:
> 
> HostContentSettingsMap (or its embedder, i.e. Profile) -> SupervisedProvider
->
> SupervisedUserSettingsService

HostContentSettingsMap is a component so it cannot depend on the
SupervisedProvider. The SupervisedUserSettingsService seemed like a nice
facilitator for this dependency instead of doing it on a profile.
I just uploaded the cl to check, I'll revert back to using the profile then.

[Bernhard Bauer, 2015-02-26 18:03:54]

On 2015/02/26 16:18:57, knn wrote:
> You also reviewed code that I did not write in
> chrome/browser/android/preferences/pref_service_bridge.cc
> I should probably do that separately.

Oh, sorry! It looked like it was related, but that was actually
https://codereview.chromium.org/920693003. I'll comment on that CL.

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_constants.cc (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_constants.cc:15: const char
kGeolocationDisabled[] = "GeolocationDisabled";
On 2015/02/26 16:18:56, knn wrote:
> On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> > Is the only reason we are changing these constants so that the content
> settings
> > provider is easier to implement? I would rather have a consistent style for
> the
> > settings (which is that boolean flags are positive, i.e. "allowed" and
> > "enabled", rather than negative), even if that means slightly more work to
> > implement. Policy settings historically used a mixture of positive and
> negative
> > settings, and it's super confusing.
> 
> To the contrary, Implementation is actually easier with 'Enabled'.
> However, I think Disabled reflects the intent of the preference better.
> Also this conveys in a subtle way (I think) that you can only Force disable
the
> setting and not the other way (force enable).

That's a good point; I'll have to think about it.

(Sending the rest of the comments to unblock you.)

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_settings_service.h (right):

https://codereview.chromium.org/902833003/diff/20001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_settings_service.h:83: void
RegisterContentSettings(
On 2015/02/26 16:18:56, knn wrote:
> On 2015/02/26 12:44:58, Bernhard (back on Feb 26) wrote:
> > Why does the SupervisedUserSettingsService now have to know about the
> > HostContentSettingsMap? The dependencies should be the other way around:
> > 
> > HostContentSettingsMap (or its embedder, i.e. Profile) -> SupervisedProvider
> ->
> > SupervisedUserSettingsService
> 
> HostContentSettingsMap is a component so it cannot depend on the
> SupervisedProvider. The SupervisedUserSettingsService seemed like a nice
> facilitator for this dependency instead of doing it on a profile.
> I just uploaded the cl to check, I'll revert back to using the profile then.

Hm, I'm wondering whether we could have an embedder method to insert the
individual providers into the HCSM. I'll talk to vabr@ (who I think owns HCSM
componentization about it).

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:16: class
Lock;
This class has a base::Lock member, so the fact that this compiles tells me that
we are already including the header transitively, which makes this forward
declaration unnecessary.

The include-what-you-use thing to do is to include base/synchronization/lock.h
instead.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:405: new
content_settings::SupervisedProvider());
The supervised_provider can subscribe itself to the settings service, if you
want to reduce the amount of code.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1050:
profile->GetHostContentSettingsMap()->RegisterProvider(
Can we do this when the HCSM is created instead?

https://codereview.chromium.org/902833003/diff/40001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:924: SupervisedUserSettingsService*
supervised_service =
Hm... I'm not sure if a testing profile actually needs this layer in the HCSM.
If no tests rely on it, I would be tempted to remove it.

[knn, 2015-02-27 10:43:53]

PTAL.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:16: class
Lock;
On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> This class has a base::Lock member, so the fact that this compiles tells me
that
> we are already including the header transitively, which makes this forward
> declaration unnecessary.
> 
> The include-what-you-use thing to do is to include base/synchronization/lock.h
> instead.

Done.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/off_the_record_profile_impl.cc:405: new
content_settings::SupervisedProvider());
On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> The supervised_provider can subscribe itself to the settings service, if you
> want to reduce the amount of code.

Done.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1050:
profile->GetHostContentSettingsMap()->RegisterProvider(
On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> Can we do this when the HCSM is created instead?
This is called when the profile is created. Isn't it good to have all
Service/Provider initialization in one place?

https://codereview.chromium.org/902833003/diff/40001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:924: SupervisedUserSettingsService*
supervised_service =
On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> Hm... I'm not sure if a testing profile actually needs this layer in the HCSM.
> If no tests rely on it, I would be tempted to remove it.

Removed as there are no tests testing supervised users and HCSM.

[Bernhard Bauer, 2015-02-27 11:20:25]

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:16: class
Lock;
On 2015/02/26 18:03:54, Bernhard Bauer wrote:
> This class has a base::Lock member, so the fact that this compiles tells me
that
> we are already including the header transitively, which makes this forward
> declaration unnecessary.
> 
> The include-what-you-use thing to do is to include base/synchronization/lock.h
> instead.

Include the header file here pls? (You can then remove it from the .cc file, I
guess.)

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1050:
profile->GetHostContentSettingsMap()->RegisterProvider(
On 2015/02/27 10:43:53, knn wrote:
> On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> > Can we do this when the HCSM is created instead?
> This is called when the profile is created. Isn't it good to have all
> Service/Provider initialization in one place?

It is, but the right place for it is in the ProfileImpl constructor :)

The only things that happen here are things that need to happen at a very
specific point during profile initialization (see the comment above). I think
creating the superviseduserprovider can happen at any time, as it will do
nothing for a non-supervised profile.

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:22:
SupervisedProvider(
Single-argument constructors should be marked explicit.

(I thought there was a presubmit check for that?)

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:50:
base::WeakPtrFactory<SupervisedProvider> weak_ptr_factory_;
Add DISALLOW_COPY_AND_ASSIGN? I'm not sure I want to think about what would
happen if this class gets copied by accident :)

[knn, 2015-02-27 12:05:15]

PTAL

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:16: class
Lock;
On 2015/02/27 11:20:25, Bernhard Bauer wrote:
> On 2015/02/26 18:03:54, Bernhard Bauer wrote:
> > This class has a base::Lock member, so the fact that this compiles tells me
> that
> > we are already including the header transitively, which makes this forward
> > declaration unnecessary.
> > 
> > The include-what-you-use thing to do is to include
base/synchronization/lock.h
> > instead.
> 
> Include the header file here pls? (You can then remove it from the .cc file, I
> guess.)

Done.

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/902833003/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:1050:
profile->GetHostContentSettingsMap()->RegisterProvider(
On 2015/02/27 11:20:25, Bernhard Bauer wrote:
> On 2015/02/27 10:43:53, knn wrote:
> > On 2015/02/26 18:03:54, Bernhard (back on Feb 26) wrote:
> > > Can we do this when the HCSM is created instead?
> > This is called when the profile is created. Isn't it good to have all
> > Service/Provider initialization in one place?
> 
> It is, but the right place for it is in the ProfileImpl constructor :)
> 
> The only things that happen here are things that need to happen at a very
> specific point during profile initialization (see the comment above). I think
> creating the superviseduserprovider can happen at any time, as it will do
> nothing for a non-supervised profile.

Acknowledged.

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_supervised_provider.h
(right):

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:22:
SupervisedProvider(
On 2015/02/27 11:20:25, Bernhard Bauer wrote:
> Single-argument constructors should be marked explicit.
> 
> (I thought there was a presubmit check for that?)

Done.

https://codereview.chromium.org/902833003/diff/60001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_supervised_provider.h:50:
base::WeakPtrFactory<SupervisedProvider> weak_ptr_factory_;
On 2015/02/27 11:20:25, Bernhard Bauer wrote:
> Add DISALLOW_COPY_AND_ASSIGN? I'm not sure I want to think about what would
> happen if this class gets copied by accident :)

Done.

[Bernhard Bauer, 2015-02-27 12:18:42]

LGTM w/ a nit:

https://codereview.chromium.org/902833003/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/902833003/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:1046:
scoped_ptr<content_settings::SupervisedProvider> supervised_provider(
This should be indented two spaces more.

[knn, 2015-02-27 12:40:44]

knn@chromium.org changed reviewers:
+ erg@chromium.org

[knn, 2015-02-27 12:40:44]

+erg for OWNERS review.
Elliot: Please review changes in 
off_the_record_profile_impl.cc
profile_impl.cc

Registering the supervised user content settings provider with the
HostContentSettingsMap.

https://codereview.chromium.org/902833003/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/902833003/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:1046:
scoped_ptr<content_settings::SupervisedProvider> supervised_provider(
On 2015/02/27 12:18:42, Bernhard Bauer wrote:
> This should be indented two spaces more.

Done.

[Elliot Glaysher, 2015-03-02 19:09:12]

lgtm

[knn, 2015-03-03 21:28:19]

Patchset #8 (id:140001) has been deleted

[knn, 2015-03-04 00:23:17]

The CQ bit was checked by knn@chromium.org

[knn, 2015-03-04 00:23:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
erg@chromium.org
Link to the patchset: https://codereview.chromium.org/902833003/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-03-04 00:24:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/902833003/180001

[commit-bot: I haz the power, 2015-03-04 01:57:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-04 01:57:09]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[knn, 2015-03-04 19:49:01]

The CQ bit was checked by knn@chromium.org

[knn, 2015-03-04 19:49:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
erg@chromium.org
Link to the patchset: https://codereview.chromium.org/902833003/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-03-04 19:49:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/902833003/220001

[commit-bot: I haz the power, 2015-03-04 20:42:55]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2015-03-04 20:43:40]

Patchset 11 (id:??) landed as
https://crrev.com/95221ffe02b9d0782202d80cf86766686afab13a
Cr-Commit-Position: refs/heads/master@{#319121}