Remove HostContentSettingsMap::ContentTypeHasCompoundValue function

chrome/browser/profile_resetter/profile_resetter_unittest.cc

Index: chrome/browser/profile_resetter/profile_resetter_unittest.cc
diff --git a/chrome/browser/profile_resetter/profile_resetter_unittest.cc b/chrome/browser/profile_resetter/profile_resetter_unittest.cc
index 3ad6bba27948f6887558f1767bb452e13735b46e..1d7f32f373fbcabdfa9004e7f2eea1feb2f6a177 100644
--- a/chrome/browser/profile_resetter/profile_resetter_unittest.cc
+++ b/chrome/browser/profile_resetter/profile_resetter_unittest.cc
@@ -25,9 +25,10 @@
 #include "chrome/browser/web_data_service_factory.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
+#include "components/content_settings/core/browser/content_settings_info.h"
+#include "components/content_settings/core/browser/content_settings_registry.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/browser/website_settings_info.h"
-#include "components/content_settings/core/browser/website_settings_registry.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/search_engines/template_url_service_client.h"
 #include "content/public/browser/web_contents.h"
@@ -491,12 +492,11 @@ TEST_F(ProfileResetterTest, ResetContentSettings) {
 
   // TODO(raymes): Clean up this test so that we don't have such ugly iteration
   // over the content settings.
-  content_settings::WebsiteSettingsRegistry* registry =
-      content_settings::WebsiteSettingsRegistry::GetInstance();
-  for (const content_settings::WebsiteSettingsInfo* info : *registry) {
-    ContentSettingsType content_type = info->type();
-    if (content_type == CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE ||
-        content_type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT ||
+  content_settings::ContentSettingsRegistry* registry =
+      content_settings::ContentSettingsRegistry::GetInstance();
+  for (const content_settings::ContentSettingsInfo* info : *registry) {

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

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.

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

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

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

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.

+    ContentSettingsType content_type = info->website_settings_info()->type();
+    if (content_type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT ||
         content_type == CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS) {
       // These types are excluded because one can't call
       // GetDefaultContentSetting() for them.
@@ -520,8 +520,7 @@ TEST_F(ProfileResetterTest, ResetContentSettings) {
       host_content_settings_map->SetDefaultContentSetting(content_type,
                                                           wildcard_setting);
     }
-    if (!HostContentSettingsMap::ContentTypeHasCompoundValue(content_type) &&
-        HostContentSettingsMap::IsSettingAllowedForType(
+    if (HostContentSettingsMap::IsSettingAllowedForType(
             profile()->GetPrefs(), site_setting, content_type)) {
       host_content_settings_map->SetContentSetting(
           pattern, ContentSettingsPattern::Wildcard(), content_type,
@@ -535,11 +534,9 @@ TEST_F(ProfileResetterTest, ResetContentSettings) {
 
   ResetAndWait(ProfileResetter::CONTENT_SETTINGS);
 
-  for (const content_settings::WebsiteSettingsInfo* info : *registry) {
-    ContentSettingsType content_type = info->type();
-    if (HostContentSettingsMap::ContentTypeHasCompoundValue(content_type) ||
-        content_type == CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE ||
-        content_type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT ||
+  for (const content_settings::ContentSettingsInfo* info : *registry) {
+    ContentSettingsType content_type = info->website_settings_info()->type();
+    if (content_type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT ||
         content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM ||
         content_type == CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS)
       continue;
@@ -548,15 +545,9 @@ TEST_F(ProfileResetterTest, ResetContentSettings) {
                                                               NULL);
     EXPECT_TRUE(default_settings.count(content_type));
     EXPECT_EQ(default_settings[content_type], default_setting);
-    if (!HostContentSettingsMap::ContentTypeHasCompoundValue(content_type)) {
-      ContentSetting site_setting =
-          host_content_settings_map->GetContentSetting(
-              GURL("example.org"),
-              GURL(),
-              content_type,
-              std::string());
-      EXPECT_EQ(default_setting, site_setting);
-    }
+    ContentSetting site_setting = host_content_settings_map->GetContentSetting(
+        GURL("example.org"), GURL(), content_type, std::string());
+    EXPECT_EQ(default_setting, site_setting);
 
     ContentSettingsForOneType host_settings;
     host_content_settings_map->GetSettingsForOneType(