Remove HostContentSettingsMap::IsSettingAllowedForType

Remove HostContentSettingsMap::IsSettingAllowedForType

This removes HostContentSettingsMap::IsSettingAllowedForType replacing it with a
property on ContentSettingsInfo which specifies allowable types. Some of the
registration code has been simplified by using overloaded functions.

BUG=526932
Committed: https://crrev.com/3028730af43f92a339901d25f96a3b8d35cdf775
Cr-Commit-Position: refs/heads/master@{#353488}

[raymes, 2015-09-28 07:09:21]

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

[Bernhard Bauer, 2015-09-28 18:46:09]

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_info.h:23: const
std::vector<ContentSetting>& valid_settings);
std::set? ContentSetting is an integer enum after all.

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:46: } else {
You don't need the else if you return in the previous branch anyway; see my
comment on https://codereview.chromium.org/1318083005/.

[raymes, 2015-09-29 05:51:26]

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_info.h (right):

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_info.h:23: const
std::vector<ContentSetting>& valid_settings);
On 2015/09/28 18:46:09, Bernhard Bauer wrote:
> std::set? ContentSetting is an integer enum after all.

Done.

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:46: } else {
On 2015/09/28 18:46:09, Bernhard Bauer wrote:
> You don't need the else if you return in the previous branch anyway; see my
> comment on https://codereview.chromium.org/1318083005/.

Done.

[raymes, 2015-09-30 23:48:22]

Ping :)

On Tue, 29 Sep 2015 at 15:51 <raymes@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
> File components/content_settings/core/browser/content_settings_info.h
> (right):
>
>
>
https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
> components/content_settings/core/browser/content_settings_info.h:23
>
<https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...>
> :
> const std::vector<ContentSetting>& valid_settings);
> On 2015/09/28 18:46:09, Bernhard Bauer wrote:
> > std::set? ContentSetting is an integer enum after all.
>
> Done.
>
>
>
https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
> File components/content_settings/core/browser/content_settings_pref.cc
> (right):
>
>
>
https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...
> components/content_settings/core/browser/content_settings_pref.cc:46
>
<https://codereview.chromium.org/1320673013/diff/80001/components/content_sett...>:
> }
> else {
> On 2015/09/28 18:46:09, Bernhard Bauer wrote:
> > You don't need the else if you return in the previous branch anyway;
> see my
> > comment on https://codereview.chromium.org/1318083005/.
>
> Done.
>
> https://codereview.chromium.org/1320673013/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Bernhard Bauer, 2015-10-01 07:55:49]

LGTM, sorry!

[raymes, 2015-10-01 23:48:15]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-10-01 23:49:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673013/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673013/100001

[commit-bot: I haz the power, 2015-10-01 23:53:59]

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

[commit-bot: I haz the power, 2015-10-01 23:54:00]

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

[raymes, 2015-10-06 01:28:57]

raymes@chromium.org changed reviewers:
+ benwells@chromium.org, engedy@chromium.org

[raymes, 2015-10-06 01:28:58]

+OWNERS

benwells for
chrome/browser/extensions/api/content_settings/content_settings_api.cc

engedy for
chrome/browser/profile_resetter/profile_resetter.cc
chrome/browser/profile_resetter/profile_resetter_unittest.cc

[raymes, 2015-10-06 01:31:09]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 01:32:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673013/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673013/120001

[commit-bot: I haz the power, 2015-10-06 01:58:10]

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

[commit-bot: I haz the power, 2015-10-06 01:58:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benwells, 2015-10-06 02:28:14]

lgtm (once tests fixed)

Have you considered adding convenience functions for the 

content_settings::ContentSettingsRegistry::GetInstance()->Get(xxxx)->IsBlah

pattern? Something like

content_settings::ContentSettingsRegistry::IsBlah(xxxx)?

[benwells, 2015-10-06 02:28:19]

lgtm (once tests fixed)

Have you considered adding convenience functions for the 

content_settings::ContentSettingsRegistry::GetInstance()->Get(xxxx)->IsBlah

pattern? Something like

content_settings::ContentSettingsRegistry::IsBlah(xxxx)?

[engedy, 2015-10-06 08:25:18]

chrome/browser/profile_resetter/ LGTM.

[engedy, 2015-10-06 08:53:26]

... % comments.

https://codereview.chromium.org/1320673013/diff/120001/chrome/browser/profile...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/1320673013/diff/120001/chrome/browser/profile...
chrome/browser/profile_resetter/profile_resetter.cc:229: if
(HostContentSettingsMap::IsSettingAllowedForType(
I think something like this check needs to remain to prevent trying to set the
default value to content settings such as:

CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS
CONTENT_SETTINGS_TYPE_MIXEDSCRIPT
CONTENT_SETTINGS_TYPE_MEDIASTREAM

https://codereview.chromium.org/1320673013/diff/120001/components/content_set...
File components/content_settings/core/browser/content_settings_info.cc (right):

https://codereview.chromium.org/1320673013/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_info.cc:21: bool
ContentSettingsInfo::IsSettingValid(ContentSetting setting) const {
Either this, or something up the call hierarchy starting from
IsDefaultSettingAllowedForType() should return true for CONTENT_SETTING_DEFAULT.

Alternatively, the infrastructure could provide an explicit way to remove the
default value for a content setting.

[raymes, 2015-10-11 23:20:34]

I have thought about it but I don't think it's good to encourage that, instead
it would be better if we passed around these ContentSettingsInfo objects to help
reduce map lookups and unwind the code.

[raymes, 2015-10-12 00:03:12]

Thanks!

https://codereview.chromium.org/1320673013/diff/120001/chrome/browser/profile...
File chrome/browser/profile_resetter/profile_resetter.cc (left):

https://codereview.chromium.org/1320673013/diff/120001/chrome/browser/profile...
chrome/browser/profile_resetter/profile_resetter.cc:229: if
(HostContentSettingsMap::IsSettingAllowedForType(
This should still work - CONTENT_SETTING_DEFAULT results in clearing the content
setting, which just won't do anything important for those settings.

https://codereview.chromium.org/1320673013/diff/120001/components/content_set...
File components/content_settings/core/browser/content_settings_info.cc (right):

https://codereview.chromium.org/1320673013/diff/120001/components/content_set...
components/content_settings/core/browser/content_settings_info.cc:21: bool
ContentSettingsInfo::IsSettingValid(ContentSetting setting) const {
Good catch. I agree with what you're saying in the second part of the comment.
DEFAULT is confusing. It's not really a valid value for these content settings -
instead it's a sentinel value indicating the setting should be cleared. Instead
of making it an allowed value, I think the call sites should be aware of this.
I've made the changes to the code to correct that. Thanks!

[raymes, 2015-10-12 00:03:18]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-10-12 00:03:18]

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

[raymes, 2015-10-12 00:03:19]

The CQ bit was unchecked by raymes@chromium.org

[raymes, 2015-10-12 00:03:40]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-10-12 00:03:40]

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

[commit-bot: I haz the power, 2015-10-12 00:04:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673013/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673013/140001

[commit-bot: I haz the power, 2015-10-12 00:06:18]

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

[commit-bot: I haz the power, 2015-10-12 00:06:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2015-10-12 02:19:03]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-10-12 02:19:03]

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

[commit-bot: I haz the power, 2015-10-12 02:19:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673013/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673013/160001

[commit-bot: I haz the power, 2015-10-12 02:54:02]

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

[commit-bot: I haz the power, 2015-10-12 02:54:03]

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

[raymes, 2015-10-12 03:29:03]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-10-12 03:29:03]

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

[commit-bot: I haz the power, 2015-10-12 03:29:17]

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

[commit-bot: I haz the power, 2015-10-12 06:13:28]

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

[commit-bot: I haz the power, 2015-10-12 06:13:29]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[raymes, 2015-10-12 06:42:18]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-10-12 06:42:18]

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

[commit-bot: I haz the power, 2015-10-12 06:42:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320673013/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320673013/200001

[commit-bot: I haz the power, 2015-10-12 07:32:58]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-10-12 07:33:49]

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