Implement preference and policy based content settings providers.

Implement preference and policy based content settings providers.

BUG=64753
TEST=*ContentSettings*.*

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

[jochen (gone - plz use gerrit), 2010-12-08 09:53:10]

please review

[gfeher, 2010-12-08 13:59:17]

Here is my first round of comments and questions.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:411: {
Why is this in a separate block?

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:418:
(*provider)->UpdateDefaultSetting(content_type, setting);
Adding CanUpdateDefaultSetting to the interface and using it here would make
this more easy to understand. (At least for me.)

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:162: // This should
only be called on the UI thread.
I know this was already here, but please document that this is only used in
tests.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:227:
content_settings_providers_deleter_;
What was the reason for not using linked_ptr or something like that?

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:242: // Used around
accesses to the settings objects to guarantee thread safety.
Please list here what members are protected with this lock. (I've found it hard
in the .cc file to figure this out and verify.)

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.cc:111:
std::string* name = Details<std::string>(details).ptr();
Here and everywhere else: please test that the source is the owning profile.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.h:72: // Used
around accesses to the settings_ object to guarantee thread safety.
s/settings_/managed_default_content_settings_/ ?
A similar question applies to the other provider.

[markusheintz_, 2010-12-08 14:03:06]

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:823: for
(provider_iterator provider = content_settings_providers_.begin();
Theoretically It is possible that a content setting type is managed, but a non
managed value is returned.

A content setting type is managed if on of the providers returns true. But if
the first provider in the vector provides a non managed value, than a non
managed value is returned.

Not sure if we should care about this. Just a thought

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/policy_content_settings_provider_unittest.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider_unittest.cc:69:
// for a specific content-settings-type.
sure can do, pls remove this TODO and add a bug and assign it to me in order to
track this.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/pref_content_settings_provider.h:42: virtual
void Observe(NotificationType type,
shoudn't the observe method be public?

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/pref_content_settings_provider.h:62: // Reads
the default settings from the prefereces service. If |overwrite| is
nit: s/prefereces/preference/

[gfeher, 2010-12-08 14:13:53]

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:162: // This should
only be called on the UI thread.
Sorry, this is not true.

[jochen (gone - plz use gerrit), 2010-12-08 14:54:01]

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:411: {
On 2010/12/08 13:59:17, gfeher wrote:
> Why is this in a separate block?

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:418:
(*provider)->UpdateDefaultSetting(content_type, setting);
On 2010/12/08 13:59:17, gfeher wrote:
> Adding CanUpdateDefaultSetting to the interface and using it here would make
> this more easy to understand. (At least for me.)

I think it's better to broadcast the update to all providers.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:823: for
(provider_iterator provider = content_settings_providers_.begin();
On 2010/12/08 14:03:06, markusheintz_ wrote:
> Theoretically It is possible that a content setting type is managed, but a non
> managed value is returned.
> 
> A content setting type is managed if on of the providers returns true. But if
> the first provider in the vector provides a non managed value, than a non
> managed value is returned.
> 
> Not sure if we should care about this. Just a thought

That's true. However, in that case, already the stack of providers is
incorrectly set up. Not sure whether we should bother with this here.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:162: // This should
only be called on the UI thread.
right :)

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:227:
content_settings_providers_deleter_;
On 2010/12/08 13:59:17, gfeher wrote:
> What was the reason for not using linked_ptr or something like that?

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.h:242: // Used around
accesses to the settings objects to guarantee thread safety.
On 2010/12/08 13:59:17, gfeher wrote:
> Please list here what members are protected with this lock. (I've found it
hard
> in the .cc file to figure this out and verify.)

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.cc:111:
std::string* name = Details<std::string>(details).ptr();
On 2010/12/08 13:59:17, gfeher wrote:
> Here and everywhere else: please test that the source is the owning profile.

If the provider is used for the OTR profile, it'll still want to honor the
changes of the regular profile.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.h:72: // Used
around accesses to the settings_ object to guarantee thread safety.
On 2010/12/08 13:59:17, gfeher wrote:
> s/settings_/managed_default_content_settings_/ ?
> A similar question applies to the other provider.

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File
chrome/browser/content_settings/policy_content_settings_provider_unittest.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider_unittest.cc:69:
// for a specific content-settings-type.
On 2010/12/08 14:03:06, markusheintz_ wrote:
> sure can do, pls remove this TODO and add a bug and assign it to me in order
to
> track this.

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/pref_content_settings_provider.h (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/pref_content_settings_provider.h:42: virtual
void Observe(NotificationType type,
On 2010/12/08 14:03:06, markusheintz_ wrote:
> shoudn't the observe method be public?

Done.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/pref_content_settings_provider.h:62: // Reads
the default settings from the prefereces service. If |overwrite| is
On 2010/12/08 14:03:06, markusheintz_ wrote:
> nit: s/prefereces/preference/

Done.

[gfeher, 2010-12-09 10:29:53]

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/host_content_settings_map.cc:418:
(*provider)->UpdateDefaultSetting(content_type, setting);
Ok.

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.cc:111:
std::string* name = Details<std::string>(details).ptr();
This raises an interesting question. When you register for these notifications,
you only specify the profile_, which might be the OTR one. Does that mean that
you will only receive notifications for profile_?

I've found the following line in extensions_service.cc.
if (profile_ != Source<Profile>(source).ptr()->GetOriginalProfile()) ...
Would something like this make sense for you?

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:183:
CHECK_NE(CONTENT_SETTING_DEFAULT, setting);
What does it mean, when all the providers provide CONTENT_SETTING_DEFAULT?

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:238: // Copies of
the pref data, so that we can read it on the IO thread.
The comment at the beginning of this file says that instances of
HostContentSettingsMap can be read on any thread.

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:239:
HostContentSettings host_content_settings_;
Is this (and the next one) the legacy system that will go away when you add
patter support to the HostContentSettingsProviders? (Just out of curiosity.)

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/policy_content_settings_provider.cc:184:
ContentSettings* settings) {
Aren't you always calling this with settings =
managed_default_content_settings_?

[jochen (gone - plz use gerrit), 2010-12-09 13:18:53]

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/1/chrome/browser/content_settings...
chrome/browser/content_settings/policy_content_settings_provider.cc:111:
std::string* name = Details<std::string>(details).ptr();
On 2010/12/09 10:29:53, gfeher wrote:
> This raises an interesting question. When you register for these
notifications,
> you only specify the profile_, which might be the OTR one. Does that mean that
> you will only receive notifications for profile_?
> 
> I've found the following line in extensions_service.cc.
> if (profile_ != Source<Profile>(source).ptr()->GetOriginalProfile()) ...
> Would something like this make sense for you?

Ok, I've played around with this.

For the PROFILE_DESTROYED notification, we're only registering for our owning
profile. For the PREF_CHANGED notifications, the source is a pref service, and
we're using the pref service to register for those notifications.

I've added DCHECKs for all of those

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:183:
CHECK_NE(CONTENT_SETTING_DEFAULT, setting);
On 2010/12/09 10:29:53, gfeher wrote:
> What does it mean, when all the providers provide CONTENT_SETTING_DEFAULT?

Added a comment

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.h (right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:238: // Copies of
the pref data, so that we can read it on the IO thread.
On 2010/12/09 10:29:53, gfeher wrote:
> The comment at the beginning of this file says that instances of
> HostContentSettingsMap can be read on any thread.

Done.

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.h:239:
HostContentSettings host_content_settings_;
On 2010/12/09 10:29:53, gfeher wrote:
> Is this (and the next one) the legacy system that will go away when you add
> patter support to the HostContentSettingsProviders? (Just out of curiosity.)

Yes, in the end the host content settings map should retrieve all these values
from providers

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
File chrome/browser/content_settings/policy_content_settings_provider.cc
(right):

http://codereview.chromium.org/5528010/diff/11001/chrome/browser/content_sett...
chrome/browser/content_settings/policy_content_settings_provider.cc:184:
ContentSettings* settings) {
On 2010/12/09 10:29:53, gfeher wrote:
> Aren't you always calling this with settings =
> managed_default_content_settings_?

Done.

[jochen (gone - plz use gerrit), 2010-12-09 15:14:09]

updated after offline feedback from Gabor

[gfeher, 2010-12-09 16:09:03]

LGTM with one more issue

http://codereview.chromium.org/5528010/diff/29001/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

http://codereview.chromium.org/5528010/diff/29001/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map.cc:830:
host_content_settings_.clear();
Please protect this with a lock while you are here.