Remove HostContentSettingsMap::ContentTypeHasCompoundValue function

Remove HostContentSettingsMap::ContentTypeHasCompoundValue function

This CL removes HostContentSettingsMap::ContentTypeHasCompoundValue and changes
its callers to use ContentSettingsRegistry to check whether the type is a
content setting or website setting -- if it's a website setting it implies it
has a compound value. One callsite has also been changed to iterate over
ContentSettingsRegistry instead of WebsiteSettingsRegistry which removes the
need for the check (this required making ContentSettingsRegistry iterable).

BUG=526932
Committed: https://crrev.com/98ff54a4b68b4272c0e5cda62cc392c682dff65f
Cr-Commit-Position: refs/heads/master@{#351468}

[raymes, 2015-09-23 04:15:35]

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

[raymes, 2015-09-23 04:17:58]

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

[commit-bot: I haz the power, 2015-09-23 04:18:25]

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

[commit-bot: I haz the power, 2015-09-23 04:52:27]

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

[commit-bot: I haz the power, 2015-09-23 04:52:27]

Dry run: 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_...)

[Bernhard Bauer, 2015-09-23 11:30:38]

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:317: setting =
FixObsoleteCookiePromptMode(content_type_,
Actually, we can probably get rid of this migration code now (it's about 4 years
old at this point). Then we could also relax the restriction that non-content
website settings have to be a dictionary, simplify this so it will always make a
deep copy, and just DCHECK that the value is allowed (via the new
HostContentSettingsMap method).

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_registry.h:30: typedef
base::ScopedPtrMap<ContentSettingsType,
Prefer `using Map = ...` over typedef.

[raymes, 2015-09-24 03:07:51]

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:317: setting =
FixObsoleteCookiePromptMode(content_type_,
Let me know if I did what you were thinking :) 

One downside is that we no longer check that the value is not
CONTENT_SETTING_DEFAULT (it's allowed by IsValueAllowedForType). But I also feel
CONTENT_SETTING_DEFAULT can be confusing/ambiguous in many circumstances. It's a
value that can be used in the code but never persisted. Maybe we should remove
it eventually?

Also, I'm wondering whether if the value isn't allowed, we should actually not
load it (as opposed to just DCHECKing). AFAIK this value is coming from a file
and may not be valid. WDYT?

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_registry.h:30: typedef
base::ScopedPtrMap<ContentSettingsType,
On 2015/09/23 11:30:37, Bernhard Bauer wrote:
> Prefer `using Map = ...` over typedef.

Done.

[raymes, 2015-09-24 03:07:58]

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

[commit-bot: I haz the power, 2015-09-24 03:09:07]

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

[commit-bot: I haz the power, 2015-09-24 04:53:46]

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

[commit-bot: I haz the power, 2015-09-24 04:53:47]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2015-09-24 08:48:26]

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:317: setting =
FixObsoleteCookiePromptMode(content_type_,
On 2015/09/24 03:07:51, raymes wrote:
> Let me know if I did what you were thinking :) 
> 
> One downside is that we no longer check that the value is not
> CONTENT_SETTING_DEFAULT (it's allowed by IsValueAllowedForType). But I also
feel
> CONTENT_SETTING_DEFAULT can be confusing/ambiguous in many circumstances. It's
a
> value that can be used in the code but never persisted. Maybe we should remove
> it eventually?

CONTENT_SETTING_DEFAULT is useful for interfaces that deal with a plain
ContentSetting, to indicate the absence of a value. For base::Value* though,
absence is better indicated with a nullptr, so we could actually include the
check for DEFAULT.

> Also, I'm wondering whether if the value isn't allowed, we should actually not
> load it (as opposed to just DCHECKing). AFAIK this value is coming from a file
> and may not be valid. WDYT?

Here's my thinking: In a Debug build, we want to notify the user if the value is
invalid, to catch cases where e.g. we write out invalid data, and a DCHECK will
do that.

In a Release build, we don't want to crash (as we need to be able to deal with
arbitrary data there), but the question is whether we want to silently drop
those values or just keep ignoring them. I'm slightly leaning towards keeping
them, because that means we won't start deleting data if we get a bug in the
logic that causes us to think all values are invalid, and we also won't have to
deal with syncing between clients with different versions if we ever change the
logic. If you feel strongly about it though, I'd be okay with dropping the
values.

[raymes, 2015-09-28 06:45:23]

Thanks!

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

https://codereview.chromium.org/1318083005/diff/80001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:317: setting =
FixObsoleteCookiePromptMode(content_type_,
Sounds good. I've left the behavior as it is for now but I added a check for
CONTENT_SETTING_DEFAULT :)

I propose we change the name for CONTENT_SETTING_DEFAULT to
CONTENT_SETTING_INVALID. I think it can be easily confused to have something to
do with the default value but that's not necessarily true. WDYT?

[Bernhard Bauer, 2015-09-28 18:38:25]

LGTM

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:459:
nullptr, content_settings::ValueToContentSetting(value), type);
You can reuse |setting| here.

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:460: }
else {
You don't need the else if you return in the previous branch anyway. (Feel free
to put either of the branches first.)

[raymes, 2015-09-29 04:53:19]

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:459:
nullptr, content_settings::ValueToContentSetting(value), type);
On 2015/09/28 18:38:24, Bernhard Bauer wrote:
> You can reuse |setting| here.

Done.

https://codereview.chromium.org/1318083005/diff/160001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:460: }
else {
On 2015/09/28 18:38:24, Bernhard Bauer wrote:
> You don't need the else if you return in the previous branch anyway. (Feel
free
> to put either of the branches first.)

Done.

[raymes, 2015-09-29 04:53:43]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-09-29 04:53:43]

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/1318083005/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-09-29 04:54:14]

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

[commit-bot: I haz the power, 2015-09-29 05:03:58]

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

[commit-bot: I haz the power, 2015-09-29 05:03:59]

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

[raymes, 2015-09-29 05:14:57]

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

[raymes, 2015-09-29 05:15:09]

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

[raymes, 2015-09-29 05:15:10]

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

[engedy, 2015-09-29 12:26:18]

c/b/profile_resetter LGTM. The unittest after the change seems to be verifying
the same set of expectations that the original unit test was intended to verify.
Feel free to go ahead and land this CL.

On the other hand, the unit test before this change seems to have been broken in
the sense that it did not verify the intended set of expectations, furthermore,
the intended set of expectations was seemingly incorrect too.

Adding Martin, who is a lot more familiar with content settings than I am, to
sanity check and decide if we need a follow-up CL.

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:497: for (const
content_settings::ContentSettingsInfo* info : *registry) {
This changes behavior locally for three (non-content-setting) website settings,
namely:
 -- CONTENT_SETTINGS_TYPE_APP_BANNER,
 -- CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, 
 -- CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS.

However, I believe the intention would have been to add them here in the first
place, but they were introduced after this unittest had been written, and were
forgotten.

Given that all these settings are of compound type, most of the expectations
contained in this unittest were not enforced in case of these settings anyway.
Checking the "default default setting" was, but I think that was a mistake, and
luckily enough the implementation failed gracefully enough and returned
CONTENT_SETTING_DEFAULT both in this and the subsequent for-loop.

All-in-all, after this change, now that all non-content-setting website settings
are removed, this unit test seems to be checking what it was originally intended
to check.

On the other hand, I think those original intentions were wrong. Even for
website settings of compound type, the profile resetter is expected to clear all
site-specific settings. This is currently not verified by this unittest.

@Martin, does this sound reasonable to you? If verifying some expectations are
indeed missing from this unittest, could you please file a bug for that? I don't
think this blocks this CL, though, it should be addressed in a follow-up CL.

[engedy, 2015-09-29 12:26:45]

engedy@chromium.org changed reviewers:
+ msramek@chromium.org

[engedy, 2015-09-29 12:26:45]

Actually adding Martin.

[msramek, 2015-09-29 12:48:53]

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:497: for (const
content_settings::ContentSettingsInfo* info : *registry) {
On 2015/09/29 12:26:18, engedy wrote:
> This changes behavior locally for three (non-content-setting) website
settings,
> namely:
>  -- CONTENT_SETTINGS_TYPE_APP_BANNER,
>  -- CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT, 
>  -- CONTENT_SETTINGS_TYPE_SSL_CERT_DECISIONS.
> 
> However, I believe the intention would have been to add them here in the first
> place, but they were introduced after this unittest had been written, and were
> forgotten.
> 
> Given that all these settings are of compound type, most of the expectations
> contained in this unittest were not enforced in case of these settings anyway.
> Checking the "default default setting" was, but I think that was a mistake,
and
> luckily enough the implementation failed gracefully enough and returned
> CONTENT_SETTING_DEFAULT both in this and the subsequent for-loop.
> 
> All-in-all, after this change, now that all non-content-setting website
settings
> are removed, this unit test seems to be checking what it was originally
intended
> to check.
> 
> On the other hand, I think those original intentions were wrong. Even for
> website settings of compound type, the profile resetter is expected to clear
all
> site-specific settings. This is currently not verified by this unittest.
> 
> @Martin, does this sound reasonable to you? If verifying some expectations are
> indeed missing from this unittest, could you please file a bug for that? I
don't
> think this blocks this CL, though, it should be addressed in a follow-up CL.

I agree with your assessment. Filed crbug.com/537190.

[engedy, 2015-09-29 13:05:14]

Thank you!

[engedy, 2015-09-29 13:05:43]

engedy@chromium.org changed reviewers:
- msramek@chromium.org

[raymes, 2015-09-30 00:42:46]

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
File chrome/browser/profile_resetter/profile_resetter_unittest.cc (right):

https://codereview.chromium.org/1318083005/diff/180001/chrome/browser/profile...
chrome/browser/profile_resetter/profile_resetter_unittest.cc:497: for (const
content_settings::ContentSettingsInfo* info : *registry) {
I agree with you too. Hopefully these changes are making the code clearer :) 

You'll notice that on line 518 and 523 below we call
HostContentSettingsMap::IsSettingAllowedForType. This will return false for all
"compound" types and so those branches won't be entered for these types but the
only code that does any EXPECT calls is in that branch. So I don't think this CL
changes the behavior but I do agree that it looks like we should be testing
those types.

[raymes, 2015-09-30 00:42:51]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2015-09-30 00:43:52]

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

[commit-bot: I haz the power, 2015-09-30 01:38:59]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-30 01:39:44]

Patchset 10 (id:??) landed as
https://crrev.com/98ff54a4b68b4272c0e5cda62cc392c682dff65f
Cr-Commit-Position: refs/heads/master@{#351468}