Add a global on/off switch for content settings and expose a toggle on the Website Settings options…

Add a global on/off switch for content settings and expose a toggle on the Website Settings options page.

TBR=miguelg@chromium.org
BUG=372607
Committed: https://crrev.com/d5770d216672950727a76e546aab7b610f78050b
Cr-Commit-Position: refs/heads/master@{#295572}

[Daniel Nishi, 2014-09-10 01:16:33]

dhnishi@chromium.org changed reviewers:
+ bauerb@chromium.org, gbillock@chromium.org

[Daniel Nishi, 2014-09-10 01:16:34]

gbillock@chromium.org: Please review changes in 
chrome/browser/ui/website_settings/website_settings.cc. 

bauerb@chromium.org: Please review changes in rest.

Thanks!

[Bernhard Bauer, 2014-09-10 09:07:18]

+Dominic FYI

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:26:
base::DictionaryValue* override_content_settings =
Inline this?

Actually, there is a version of RegisterDictionaryPref that uses an empty
dictionary as default.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:50: if
(is_incognito_)
So, in incognito, settings are silently immutable? That's a bit strange.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:61: bool
OverrideProvider::IsEnabled(ContentSettingsType content_type) const {
There is of course another way: You could make this an actual
ContentSettingProvider, and insert it just above regular user prefs. That would
also allow e.g. enterprise policies to override it (admittedly I can't see a
huge use case for forcing some content type *on*, but hey, if The Admin wants
that, The Admin gets that).

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:71:
base::AutoLock lock(lock_);
If this method is only called during construction, you don't need the lock.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
2014, and I think the (c) shouldn't be there for new files.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:10:
#include "base/basictypes.h"
Do you actually need basictypes here, or base/macros.h (for
DISALLOW_COPY_AND_ASSIGN)?

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:23: //
globally disabled.
What's the thread safety story here?

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:31: //
Returns if |content_type| is enabled.
Note that content settings in principle are a general map, and not every content
type necessarily has a "disabled" setting. I think you ran into that with media
stream.

If we keep this interface (see my comment in the .cc file), we should make it
clearer what enabled and disabled means (i.e. enabled means look at other
content settings and return whatever setting is stored there, disabled means
forcing the setting to a disabled value, whatever that is for the content type
at hand).

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:264: // The
ownership of the resulting |Value| is transfered to the caller.
If you transfer ownership, consider returning a scoped_ptr.

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:995: const char kOverrideContentSettings[] =
"profile.override_content_settings";
There are some existing prefs/switches for turning individual content settings
off. If we add a new general pref, we should migrate the existing stuff to that.

[Daniel Nishi, 2014-09-10 16:38:29]

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:26:
base::DictionaryValue* override_content_settings =
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> Inline this?
> 
> Actually, there is a version of RegisterDictionaryPref that uses an empty
> dictionary as default.

Using the empty dictionary version.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:50: if
(is_incognito_)
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> So, in incognito, settings are silently immutable? That's a bit strange.

The only way to modify it right now would be through the chrome://settings page,
which couldn't be run in incognito as far as I know. It seemed like adding in
the ability to change it in incognito would be better suited to a patch where it
was surfaced when that was possible.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:61: bool
OverrideProvider::IsEnabled(ContentSettingsType content_type) const {
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> There is of course another way: You could make this an actual
> ContentSettingProvider, and insert it just above regular user prefs. That
would
> also allow e.g. enterprise policies to override it (admittedly I can't see a
> huge use case for forcing some content type *on*, but hey, if The Admin wants
> that, The Admin gets that).

Would taking this path with some special cases for handling provider->first ==
OVERRIDE_PROVIDER during provider iterations be preferable to it not being a
ContentSettingsProvider?

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:71:
base::AutoLock lock(lock_);
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> If this method is only called during construction, you don't need the lock.

Done.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2014/09/10 09:07:18, Bernhard Bauer wrote:
> 2014, and I think the (c) shouldn't be there for new files.

Done.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:10:
#include "base/basictypes.h"
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> Do you actually need basictypes here, or base/macros.h (for
> DISALLOW_COPY_AND_ASSIGN)?

The latter.

Done.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:23: //
globally disabled.
On 2014/09/10 09:07:18, Bernhard Bauer wrote:
> What's the thread safety story here?

I think now that I've locked checking IsEnabled(), the locks should keep the
thread safety for IsEnabled() and SetContentSetting().

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.h:31: //
Returns if |content_type| is enabled.
On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> Note that content settings in principle are a general map, and not every
content
> type necessarily has a "disabled" setting. I think you ran into that with
media
> stream.
> 
> If we keep this interface (see my comment in the .cc file), we should make it
> clearer what enabled and disabled means (i.e. enabled means look at other
> content settings and return whatever setting is stored there, disabled means
> forcing the setting to a disabled value, whatever that is for the content type
> at hand).

I've clarified the comment.

I think for how it is currently exposed, we should be fine. It'd be a bit dicier
if something like handlers or zoom was exposed as being globally blocked, but I
think our current set shouldn't bring up too many problems.

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/host_content_settings_map.h (right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/host_content_settings_map.h:264: // The
ownership of the resulting |Value| is transfered to the caller.
On 2014/09/10 09:07:18, Bernhard Bauer wrote:
> If you transfer ownership, consider returning a scoped_ptr.

This made me realize that GetWebsiteSetting() was never used outside of
immediately putting the base::Value* into a scoped_ptr.

I've changed the ownership passing all the way up to passing a scoped_ptr.

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:995: const char kOverrideContentSettings[] =
"profile.override_content_settings";
On 2014/09/10 09:07:18, Bernhard Bauer wrote:
> There are some existing prefs/switches for turning individual content settings
> off. If we add a new general pref, we should migrate the existing stuff to
that.

Do you know an example of one off the top of your head?

I was able to find the kWebkitPluginsEnabled, kWebkitJavascriptEnabled, and
kGeolocationEnabled (Android-only) so far.

[Bernhard Bauer, 2014-09-10 17:44:54]

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:50: if
(is_incognito_)
On 2014/09/10 16:38:28, Daniel Nishi wrote:
> On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> > So, in incognito, settings are silently immutable? That's a bit strange.
> 
> The only way to modify it right now would be through the chrome://settings
page,
> which couldn't be run in incognito as far as I know. It seemed like adding in
> the ability to change it in incognito would be better suited to a patch where
it
> was surfaced when that was possible.

Right... I think I'm okay with not having the ability to change these settings
in incognito, but then we shouldn't silently ignore changes, but rather DCHECK.

(If we were to have that ability, we would only remove the call to change the
pref dictionary if we're in incognito. That way you should get a non-persistent
copy of the regular settings).

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:61: bool
OverrideProvider::IsEnabled(ContentSettingsType content_type) const {
On 2014/09/10 16:38:28, Daniel Nishi wrote:
> On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> > There is of course another way: You could make this an actual
> > ContentSettingProvider, and insert it just above regular user prefs. That
> would
> > also allow e.g. enterprise policies to override it (admittedly I can't see a
> > huge use case for forcing some content type *on*, but hey, if The Admin
wants
> > that, The Admin gets that).
> 
> Would taking this path with some special cases for handling provider->first ==
> OVERRIDE_PROVIDER during provider iterations be preferable to it not being a
> ContentSettingsProvider?

I think so... if there aren't too many special cases. Where would we need those?

Also, there's still the question of whether we want to override content setting
providers that are higher precedence than user settings (enterprise policies and
extensions). Specifically, for enterprise that could be problematic.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:44:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
There is a DCHECK_CURRENTLY_ON macro that will give more informative error
messages on failure.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:69: const
base::DictionaryValue* override_settings_dictionary =
I would DCHECK that you're on the UI thread here. PrefService will do that as
well, but if you're checking it in other places in this class, doing it here as
well would be consistent.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:278:
content_settings_->GetWebsiteSettingWithoutOverride(
Why do you need the setting without override here?

[Daniel Nishi, 2014-09-10 17:55:53]

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:61: bool
OverrideProvider::IsEnabled(ContentSettingsType content_type) const {
On 2014/09/10 17:44:53, Bernhard Bauer wrote:
> On 2014/09/10 16:38:28, Daniel Nishi wrote:
> > On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> > > There is of course another way: You could make this an actual
> > > ContentSettingProvider, and insert it just above regular user prefs. That
> > would
> > > also allow e.g. enterprise policies to override it (admittedly I can't see
a
> > > huge use case for forcing some content type *on*, but hey, if The Admin
> wants
> > > that, The Admin gets that).
> > 
> > Would taking this path with some special cases for handling provider->first
==
> > OVERRIDE_PROVIDER during provider iterations be preferable to it not being a
> > ContentSettingsProvider?
> 
> I think so... if there aren't too many special cases. Where would we need
those?
> 
> Also, there's still the question of whether we want to override content
setting
> providers that are higher precedence than user settings (enterprise policies
and
> extensions). Specifically, for enterprise that could be problematic.

It mainly comes from the distinction of trying to figure out a content setting
exception versus the effective content setting. If I'm trying to display what
the website's exception is, like in the Website Settings dialog, saying it is
blocked due to the global on/off would hide the information about the
user-defined exception.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/ui/websit...
File chrome/browser/ui/website_settings/website_settings.cc (right):

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/ui/websit...
chrome/browser/ui/website_settings/website_settings.cc:278:
content_settings_->GetWebsiteSettingWithoutOverride(
On 2014/09/10 17:44:54, Bernhard Bauer wrote:
> Why do you need the setting without override here?

The Website Settings site info dialog, which would typically show the site
specific content settings, would instead just show that it is blocked globally.

It seemed more consistent to not show the global setting and instead show the
one defined for this specific site.

[Daniel Nishi, 2014-09-10 23:30:21]

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:50: if
(is_incognito_)
On 2014/09/10 17:44:53, Bernhard Bauer wrote:
> On 2014/09/10 16:38:28, Daniel Nishi wrote:
> > On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> > > So, in incognito, settings are silently immutable? That's a bit strange.
> > 
> > The only way to modify it right now would be through the chrome://settings
> page,
> > which couldn't be run in incognito as far as I know. It seemed like adding
in
> > the ability to change it in incognito would be better suited to a patch
where
> it
> > was surfaced when that was possible.
> 
> Right... I think I'm okay with not having the ability to change these settings
> in incognito, but then we shouldn't silently ignore changes, but rather
DCHECK.
> 
> (If we were to have that ability, we would only remove the call to change the
> pref dictionary if we're in incognito. That way you should get a
non-persistent
> copy of the regular settings).

Done.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:44:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2014/09/10 17:44:53, Bernhard Bauer wrote:
> There is a DCHECK_CURRENTLY_ON macro that will give more informative error
> messages on failure.

Done.

https://codereview.chromium.org/542253003/diff/40001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:69: const
base::DictionaryValue* override_settings_dictionary =
On 2014/09/10 17:44:54, Bernhard Bauer wrote:
> I would DCHECK that you're on the UI thread here. PrefService will do that as
> well, but if you're checking it in other places in this class, doing it here
as
> well would be consistent.

Done.

[Bernhard Bauer, 2014-09-11 16:49:57]

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/1/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_override_provider.cc:61: bool
OverrideProvider::IsEnabled(ContentSettingsType content_type) const {
On 2014/09/10 17:55:52, Daniel Nishi wrote:
> On 2014/09/10 17:44:53, Bernhard Bauer wrote:
> > On 2014/09/10 16:38:28, Daniel Nishi wrote:
> > > On 2014/09/10 09:07:17, Bernhard Bauer wrote:
> > > > There is of course another way: You could make this an actual
> > > > ContentSettingProvider, and insert it just above regular user prefs.
That
> > > would
> > > > also allow e.g. enterprise policies to override it (admittedly I can't
see
> a
> > > > huge use case for forcing some content type *on*, but hey, if The Admin
> > wants
> > > > that, The Admin gets that).
> > > 
> > > Would taking this path with some special cases for handling
provider->first
> ==
> > > OVERRIDE_PROVIDER during provider iterations be preferable to it not being
a
> > > ContentSettingsProvider?
> > 
> > I think so... if there aren't too many special cases. Where would we need
> those?
> > 
> > Also, there's still the question of whether we want to override content
> setting
> > providers that are higher precedence than user settings (enterprise policies
> and
> > extensions). Specifically, for enterprise that could be problematic.
> 
> It mainly comes from the distinction of trying to figure out a content setting
> exception versus the effective content setting. If I'm trying to display what
> the website's exception is, like in the Website Settings dialog, saying it is
> blocked due to the global on/off would hide the information about the
> user-defined exception.

Hm... I think in the content settings UI we get around that by getting a list of
all the content setting  rules, together with their source.

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/542253003/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:995: const char kOverrideContentSettings[] =
"profile.override_content_settings";
On 2014/09/10 16:38:29, Daniel Nishi wrote:
> On 2014/09/10 09:07:18, Bernhard Bauer wrote:
> > There are some existing prefs/switches for turning individual content
settings
> > off. If we add a new general pref, we should migrate the existing stuff to
> that.
> 
> Do you know an example of one off the top of your head?
> 
> I was able to find the kWebkitPluginsEnabled, kWebkitJavascriptEnabled, and
> kGeolocationEnabled (Android-only) so far. 

Yeah, those were the ones I had in mind. Although now that I think about it, we
don't actually have UI to change these...

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:45:
scoped_ptr<base::Value> value_;
If we remove the ALLOW setting (instead falling through to user prefs), you
don't need the value at all anymore! You'll just need a flag to say whether
you're finished with the "iteration" (over a single element).

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:116: if
(setting != CONTENT_SETTING_ALLOW && setting != CONTENT_SETTING_BLOCK)
This should be a DCHECK.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.h:64: typedef
std::map<ContentSettingsType, ValuePtr> ValueMap;
If this provider is only used to turn content types off, you can actually get by
with a set of types (or an array, as ContentSettingsType is an enum). You're
never going to store more than a single bit per content type.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:293: if
(provider->first == OVERRIDE_PROVIDER)
If you use a new method to change overrides, you can simply return false in your
implementation of SetWebsiteSetting.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:455:
override->SetWebsiteSetting(ContentSettingsPattern::Wildcard(),
If you already cast to OverrideProvider, you can add a non-OVERRIDE method to
the class and call that (then you don't need to pass parameters like the
wildcard or the empty resource identifier).

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:838:
CONTENT_SETTING_ALLOW) {
I think you can avoid this by not storing a value in the override provider at
all if it's set to ALLOW.

[Daniel Nishi, 2014-09-11 20:58:13]

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:45:
scoped_ptr<base::Value> value_;
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> If we remove the ALLOW setting (instead falling through to user prefs), you
> don't need the value at all anymore! You'll just need a flag to say whether
> you're finished with the "iteration" (over a single element).

Done.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.cc:116: if
(setting != CONTENT_SETTING_ALLOW && setting != CONTENT_SETTING_BLOCK)
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> This should be a DCHECK.

Function now just returns false.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/content_settings_override_provider.h:64: typedef
std::map<ContentSettingsType, ValuePtr> ValueMap;
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> If this provider is only used to turn content types off, you can actually get
by
> with a set of types (or an array, as ContentSettingsType is an enum). You're
> never going to store more than a single bit per content type.

Done.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
File chrome/browser/content_settings/host_content_settings_map.cc (right):

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:293: if
(provider->first == OVERRIDE_PROVIDER)
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> If you use a new method to change overrides, you can simply return false in
your
> implementation of SetWebsiteSetting.

Done.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:455:
override->SetWebsiteSetting(ContentSettingsPattern::Wildcard(),
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> If you already cast to OverrideProvider, you can add a non-OVERRIDE method to
> the class and call that (then you don't need to pass parameters like the
> wildcard or the empty resource identifier).

Done.

https://codereview.chromium.org/542253003/diff/80001/chrome/browser/content_s...
chrome/browser/content_settings/host_content_settings_map.cc:838:
CONTENT_SETTING_ALLOW) {
On 2014/09/11 16:49:56, Bernhard Bauer wrote:
> I think you can avoid this by not storing a value in the override provider at
> all if it's set to ALLOW.

Done.

[Bernhard Bauer, 2014-09-12 09:37:38]

Heads-up: https://codereview.chromium.org/545413002/ might change how content
setting providers are registered.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:35:
is_done_ = false;
You want true here?

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:128: if
(allowed_settings_dictionary) {
I don't think this check is necessary. If you registered the preference, you can
expect to get it back from the PrefService.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:133: for
(size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {
You could do this the other way around: Iterate over the array, and set the flag
to true if you find the type in the dictionary. That way you also don't need
ClearBlockedSettings (which is only called here, right?).

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.h:8: #include
<map>
This is unnecessary now.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.h:25: //
globally disabled.
Please add a comment about thread safety: writing only allowed on UI thread,
reading allowed on any thread (following the model of the HCSM).

[Daniel Nishi, 2014-09-12 16:40:36]

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:35:
is_done_ = false;
On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> You want true here?

Done.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:128: if
(allowed_settings_dictionary) {
On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> I don't think this check is necessary. If you registered the preference, you
can
> expect to get it back from the PrefService.

Done.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:133: for
(size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {
On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> You could do this the other way around: Iterate over the array, and set the
flag
> to true if you find the type in the dictionary. That way you also don't need
> ClearBlockedSettings (which is only called here, right?).

I went the other direction where it sets the flag to false if it finds the type
because I think more will be enabled globally than disabled.

ClearBlockedSettings removed.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.h
(right):

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.h:8: #include
<map>
On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> This is unnecessary now.

Done.

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.h:25: //
globally disabled.
On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> Please add a comment about thread safety: writing only allowed on UI thread,
> reading allowed on any thread (following the model of the HCSM).

Done.

[Bernhard Bauer, 2014-09-12 17:16:37]

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/100001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:133: for
(size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {
On 2014/09/12 16:40:35, Daniel Nishi wrote:
> On 2014/09/12 09:37:38, Bernhard Bauer wrote:
> > You could do this the other way around: Iterate over the array, and set the
> flag
> > to true if you find the type in the dictionary. That way you also don't need
> > ClearBlockedSettings (which is only called here, right?).
> 
> I went the other direction where it sets the flag to false if it finds the
type
> because I think more will be enabled globally than disabled.

OK. Either one is fine with me, but we should document in pref_names.cc what
value we set in the dictionary, and what it (and its absence) means.

> ClearBlockedSettings removed.

https://codereview.chromium.org/542253003/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:130:
allowed_settings_[content_setting] = false;
I would probably directly assign !dictionary->HasKey(...).

[Daniel Nishi, 2014-09-12 19:51:43]

https://codereview.chromium.org/542253003/diff/120001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/120001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:130:
allowed_settings_[content_setting] = false;
On 2014/09/12 17:16:36, Bernhard Bauer wrote:
> I would probably directly assign !dictionary->HasKey(...).

Done.

[Bernhard Bauer, 2014-09-15 09:17:16]

LGTM with nits:

https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:113:
GetTypeName(content_type), new base::FundamentalValue(true));
You don't actually use the exact value anymore, but I would use false, because
that indicates "blocked" to me more than true does (in case someone e.g. looks
at the Preferences file on disk).

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

https://codereview.chromium.org/542253003/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:995: // If a value is not set, it means the setting
is allowed.
Please also explain that in that case, we will fall through to other content
settings (which might also block it).

[Daniel Nishi, 2014-09-15 17:40:51]

gbillock: Ping! :)

https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_override_provider.cc
(right):

https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_override_provider.cc:113:
GetTypeName(content_type), new base::FundamentalValue(true));
On 2014/09/15 09:17:16, Bernhard Bauer wrote:
> You don't actually use the exact value anymore, but I would use false, because
> that indicates "blocked" to me more than true does (in case someone e.g. looks
> at the Preferences file on disk).

Done.

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

https://codereview.chromium.org/542253003/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:995: // If a value is not set, it means the setting
is allowed.
On 2014/09/15 09:17:16, Bernhard Bauer wrote:
> Please also explain that in that case, we will fall through to other content
> settings (which might also block it).

Done.

[Greg Billock, 2014-09-17 21:59:19]

On 2014/09/15 17:40:51, Daniel Nishi wrote:
> gbillock: Ping! :)
> 
>
https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
> File chrome/browser/content_settings/content_settings_override_provider.cc
> (right):
> 
>
https://codereview.chromium.org/542253003/diff/140001/chrome/browser/content_...
> chrome/browser/content_settings/content_settings_override_provider.cc:113:
> GetTypeName(content_type), new base::FundamentalValue(true));
> On 2014/09/15 09:17:16, Bernhard Bauer wrote:
> > You don't actually use the exact value anymore, but I would use false,
because
> > that indicates "blocked" to me more than true does (in case someone e.g.
looks
> > at the Preferences file on disk).
> 
> Done.
> 
>
https://codereview.chromium.org/542253003/diff/140001/chrome/common/pref_name...
> File chrome/common/pref_names.cc (right):
> 
>
https://codereview.chromium.org/542253003/diff/140001/chrome/common/pref_name...
> chrome/common/pref_names.cc:995: // If a value is not set, it means the
setting
> is allowed.
> On 2014/09/15 09:17:16, Bernhard Bauer wrote:
> > Please also explain that in that case, we will fall through to other content
> > settings (which might also block it).
> 
> Done.

lgtm website_settings

[Daniel Nishi, 2014-09-17 22:00:27]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-17 22:02:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/542253003/200001

[commit-bot: I haz the power, 2014-09-17 22:05:50]

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

[commit-bot: I haz the power, 2014-09-17 22:05:51]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/57392)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Daniel Nishi, 2014-09-17 22:25:44]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-17 22:26:52]

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

[commit-bot: I haz the power, 2014-09-17 22:40:27]

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

[commit-bot: I haz the power, 2014-09-17 22:40:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Nishi, 2014-09-17 22:53:08]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-17 22:54:21]

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

[commit-bot: I haz the power, 2014-09-18 00:12:46]

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

[commit-bot: I haz the power, 2014-09-18 00:12:47]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniel Nishi, 2014-09-18 21:28:16]

The CQ bit was checked by dhnishi@chromium.org

[commit-bot: I haz the power, 2014-09-18 21:29:41]

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

[commit-bot: I haz the power, 2014-09-18 21:59:26]

Committed patchset #14 (id:260001) as a8f1e8b78993a3c06d3a76bb7f5bd518522961e1

[commit-bot: I haz the power, 2014-09-18 22:00:02]

Patchset 14 (id:??) landed as
https://crrev.com/d5770d216672950727a76e546aab7b610f78050b
Cr-Commit-Position: refs/heads/master@{#295572}