Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage

chrome/browser/content_settings/host_content_settings_map_unittest.cc

Index: chrome/browser/content_settings/host_content_settings_map_unittest.cc
diff --git a/chrome/browser/content_settings/host_content_settings_map_unittest.cc b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
index 4573d3d95e49b64b4a43fbdf751c69ff77f3f5ba..7c3b3a68544d8d02482ca078980d145885cfd284 100644
--- a/chrome/browser/content_settings/host_content_settings_map_unittest.cc
+++ b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
@@ -301,12 +301,16 @@ TEST_F(HostContentSettingsMapTest, Patterns) {
   GURL host1("http://example.com/");
   GURL host2("http://www.example.com/");
   GURL host3("http://example.org/");
+  ContentSettingsPattern pattern1 =
+      ContentSettingsPattern::FromString("[*.]example.com");
+  ContentSettingsPattern pattern2 =
+      ContentSettingsPattern::FromString("example.org");
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
             host_content_settings_map->GetContentSetting(
                 host1, host1, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
-  host_content_settings_map->SetContentSettingDefaultScope(
-      host1, GURL(), CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
-      CONTENT_SETTING_BLOCK);
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern1, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
                 host1, host1, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
@@ -316,14 +320,43 @@ TEST_F(HostContentSettingsMapTest, Patterns) {
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
             host_content_settings_map->GetContentSetting(
                 host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
-  host_content_settings_map->SetContentSettingDefaultScope(
-      host3, GURL(), CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
-      CONTENT_SETTING_BLOCK);
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern2, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
                 host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
 }
 
+TEST_F(HostContentSettingsMapTest, Origins) {
+  TestingProfile profile;
+  HostContentSettingsMap* host_content_settings_map =
+      HostContentSettingsMapFactory::GetForProfile(&profile);
+
+  GURL host1("http://example.com/");
+  GURL host2("http://www.example.com/");
+  GURL host3("http://example.org/");
+  ContentSettingsPattern pattern =
+      ContentSettingsPattern::FromString("example.com");

[raymes, 2016/06/23 01:15:19]

How about we use FromURLNoWildcard here (or SetDefaultScope on an origin-scoped
setting)

[lshang, 2016/06/23 04:29:42]

Done.

+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host1, host1, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host1, host1, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  // Changing a setting for one origin doesn't affect subdomains.

[raymes, 2016/06/23 01:15:19]

Maybe move this (and the comment below) as comments for the test?

[lshang, 2016/06/23 04:29:42]

Done.

+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host2, host2, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  // Changing a setting for one origin doesn't affect other origins.
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+}
+
 TEST_F(HostContentSettingsMapTest, Observer) {
   TestingProfile profile;
   HostContentSettingsMap* host_content_settings_map =
@@ -576,58 +609,113 @@ TEST_F(HostContentSettingsMapTest, NestedSettings) {
   HostContentSettingsMap* host_content_settings_map =
       HostContentSettingsMapFactory::GetForProfile(&profile);
 
-  GURL host("http://a.b.example.com/");
   GURL host1("http://example.com/");
   GURL host2("http://b.example.com/");
+  GURL host3("http://a.b.example.com/");
+  ContentSettingsPattern pattern1 =
+      ContentSettingsPattern::FromString("[*.]example.com");
+  ContentSettingsPattern pattern2 =
+      ContentSettingsPattern::FromString("[*.]b.example.com");
+  ContentSettingsPattern pattern3 =
+      ContentSettingsPattern::FromString("a.b.example.com");
 
-  host_content_settings_map->SetContentSettingDefaultScope(
-      host1, GURL(), CONTENT_SETTINGS_TYPE_POPUPS, std::string(),
-      CONTENT_SETTING_BLOCK);
+  // Test nested patterns for one type.
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_COOKIES, NULL));

[raymes, 2016/06/23 01:15:18]

nullptr

[lshang, 2016/06/23 04:29:42]

Done.

+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern1, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern2, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_ALLOW);
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern3, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host1, host1, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host2, host2, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  GURL host4("http://a.example.com/");

[raymes, 2016/06/23 01:15:19]

We should be consistent and either have all of these URLs defined at the top or
close to where they are used. 

[lshang, 2016/06/23 04:29:42]

Done.
Move them all at the top.

+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host4, host4, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  GURL host5("http://b.b.example.com/");
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host5, host5, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
 
-  host_content_settings_map->SetContentSettingDefaultScope(
-      host2, GURL(), CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
-      CONTENT_SETTING_BLOCK);
+  host_content_settings_map->ClearSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_COOKIES);
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_COOKIES, NULL));

[raymes, 2016/06/23 01:15:19]

nullptr

[lshang, 2016/06/23 04:29:42]

Done.

 
-  host_content_settings_map->SetContentSettingDefaultScope(
-      host, GURL(), CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, std::string(),
+  GURL https_host2("https://b.example.com/");
+  GURL https_host3("https://a.b.example.com/");
+  ContentSettingsPattern pattern4 =
+      ContentSettingsPattern::FromString("b.example.com");
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern4, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
+  // Pattern "b.example.com" will affect (http|https)://b.example.com
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host2, host2, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                https_host2, https_host2, CONTENT_SETTINGS_TYPE_COOKIES,
+                std::string()));
+  // Pattern "b.example.com" will not affect (http|https)://a.b.example.com
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                https_host3, https_host3, CONTENT_SETTINGS_TYPE_COOKIES,
+                std::string()));
+
+  host_content_settings_map->ClearSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_COOKIES);
+
+  // Nested setting of one type doesn't affect other types.

[raymes, 2016/06/23 01:15:19]

I think we could move this (from here down) into a separate test function. See
the comment below for more thoughts.

[lshang, 2016/06/23 04:29:43]

Done.

+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern1, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_POPUPS, std::string(), CONTENT_SETTING_BLOCK);
+
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern2, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), CONTENT_SETTING_BLOCK);
+
+  host_content_settings_map->SetContentSettingCustomScope(
+      pattern3, ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, std::string(),
       CONTENT_SETTING_BLOCK);
   host_content_settings_map->SetDefaultContentSetting(
       CONTENT_SETTINGS_TYPE_JAVASCRIPT, CONTENT_SETTING_BLOCK);
 
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
+                host3, host3, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
-  EXPECT_EQ(CONTENT_SETTING_BLOCK,
-            host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_JAVASCRIPT, std::string()));
+                host3, host3, CONTENT_SETTINGS_TYPE_COOKIES, std::string()));
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
+                host3, host3, CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
                 std::string()));
-  EXPECT_EQ(CONTENT_SETTING_ASK,
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
             host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_GEOLOCATION, std::string()));
+                host3, host3, CONTENT_SETTINGS_TYPE_JAVASCRIPT, std::string()));

[raymes, 2016/06/23 01:15:19]

It's not completely clear to me why we need to have all those types here.
Couldn't we just set one pattern for one type and then check that geolocation
still returns the correct value? 

Also I don't think it's important to specifically test that nested settings
don't impact other types, so we could just call the test TypeSettingsAreIsolated
or something (if you can think of a better name use that :)

[lshang, 2016/06/23 04:29:43]

I agree.

   EXPECT_EQ(
       CONTENT_SETTING_ASK,
       host_content_settings_map->GetContentSetting(
-          host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
-  EXPECT_EQ(CONTENT_SETTING_ASK,
-            host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_FULLSCREEN, std::string()));
-#if !defined(OS_ANDROID)
-  EXPECT_EQ(CONTENT_SETTING_ASK,
-            host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_MOUSELOCK, std::string()));
-#endif
-  EXPECT_EQ(CONTENT_SETTING_BLOCK,
-            host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_KEYGEN, std::string()));
-  EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            host_content_settings_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_AUTOPLAY, std::string()));
+          host3, host3, CONTENT_SETTINGS_TYPE_GEOLOCATION, std::string()));
 }
 
 TEST_F(HostContentSettingsMapTest, OffTheRecord) {