Stop inheriting push notification permissions from regular to incognito

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 04eac30aaa1dbde21d758886ecdcd97193367931..a5bc8a92501da8c06d71bafc0c0affe7588f1051 100644
--- a/chrome/browser/content_settings/host_content_settings_map_unittest.cc
+++ b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
@@ -678,11 +678,13 @@ TEST_F(HostContentSettingsMapTest, NestedSettings) {
 
 TEST_F(HostContentSettingsMapTest, OffTheRecord) {
   TestingProfile profile;
+  Profile* otr_profile = profile.GetOffTheRecordProfile();
   HostContentSettingsMap* host_content_settings_map =
       HostContentSettingsMapFactory::GetForProfile(&profile);
-  scoped_refptr<HostContentSettingsMap> otr_map(
-      new HostContentSettingsMap(profile.GetPrefs(),
-                                 true));
+  HostContentSettingsMap* otr_map1 =

[msramek, 2015/11/27 16:30:04]

otr_map1 and otr_map2 are the same, this is an implementation detail of
HostContentSettingsMapFactory::BuildServiceInstanceFor, or am I missing
something?

This seems quite unnecessary and IMHO decreases the readability of this test.

[johnme, 2015/11/30 14:56:58]

They're not quite the same. otr_map2 uses profile.getPrefs(), whereas otr_map1
uses the prefs from TestingProfile::CreateIncognitoPrefService (the equivalent
of ProfileImpl::GetOffTheRecordPrefs), which are built using
CreateIncognitoPrefServiceSyncable.

According to the comment in chrome/browser/prefs/pref_service_syncable_util.h,
the incognito pref service "shares most prefs but uses a fresh non-persistent
overlay for the user pref store and an individual extension pref store (to cache
the effective extension prefs for incognito windows)".

I guess that's equivalent as far as this test is concerned, but it still feels a
little odd to be testing something different from the codepath normally used at
runtime.

I've updated this test to only use GetForProfile(otr_profile), since it seems
more accurate and easier to read, but if you'd rather I can change this back to
how it was before.

[msramek, 2015/12/01 13:41:43]

If we want to test the equivalence between the two instantiations, perhaps we
can have a separate test (and I guess not in this CL). Here, I would definitely
keep just one of them. I agree that using
HostContentSettingsMap::GetForProfile() is more accurate and easier to read.

[johnme, 2015/12/02 15:13:16]

Acknowledged.

+      HostContentSettingsMapFactory::GetForProfile(otr_profile);
+  scoped_refptr<HostContentSettingsMap> otr_map2(
+      new HostContentSettingsMap(profile.GetPrefs(), true /* incognito */));
 
   GURL host("http://example.com/");
   ContentSettingsPattern pattern =
@@ -692,7 +694,10 @@ TEST_F(HostContentSettingsMapTest, OffTheRecord) {
             host_content_settings_map->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            otr_map->GetContentSetting(
+            otr_map1->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            otr_map2->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
 
   // Changing content settings on the main map should also affect the
@@ -707,12 +712,20 @@ TEST_F(HostContentSettingsMapTest, OffTheRecord) {
             host_content_settings_map->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
-            otr_map->GetContentSetting(
+            otr_map1->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            otr_map2->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
 
-  // Changing content settings on the incognito map should NOT affect the
+  // Changing content settings on the incognito maps should NOT affect the
   // main map.
-  otr_map->SetContentSetting(pattern,
+  otr_map1->SetContentSetting(pattern,
+                             ContentSettingsPattern::Wildcard(),
+                             CONTENT_SETTINGS_TYPE_IMAGES,
+                             std::string(),
+                             CONTENT_SETTING_ALLOW);
+  otr_map2->SetContentSetting(pattern,
                              ContentSettingsPattern::Wildcard(),
                              CONTENT_SETTINGS_TYPE_IMAGES,
                              std::string(),
@@ -721,10 +734,127 @@ TEST_F(HostContentSettingsMapTest, OffTheRecord) {
             host_content_settings_map->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
-            otr_map->GetContentSetting(
+            otr_map1->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            otr_map2->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_IMAGES, std::string()));
 
-  otr_map->ShutdownOnUIThread();
+  otr_map2->ShutdownOnUIThread();
+}
+
+TEST_F(HostContentSettingsMapTest, OffTheRecordPartialInheritPref) {
+  // Permissions marked INHERIT_IN_INCOGNITO_EXCEPT_ALLOW in
+  // ContentSettingsRegistry (e.g. push & notifications) only inherit blocks
+  // from the regular to incognito pref provider.
+  TestingProfile profile;
+  Profile* otr_profile = profile.GetOffTheRecordProfile();
+  HostContentSettingsMap* host_content_settings_map =
+      HostContentSettingsMapFactory::GetForProfile(&profile);
+  HostContentSettingsMap* otr_map =
+      HostContentSettingsMapFactory::GetForProfile(otr_profile);
+
+  GURL host("http://example.com/");
+  ContentSettingsPattern pattern =
+       ContentSettingsPattern::FromString("[*.]example.com");
+
+  EXPECT_EQ(CONTENT_SETTING_ASK, host_content_settings_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ASK, otr_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+
+  // BLOCK should be inherited from the main map to the incognito map.
+  host_content_settings_map->SetContentSetting(
+      pattern,
+      ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(),
+      CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(CONTENT_SETTING_BLOCK, host_content_settings_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK, otr_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+
+  // ALLOW should not be inherited from the main map to the incognito map (but
+  // it still overwrites the BLOCK, hence incognito reverts to ASK).
+  host_content_settings_map->SetContentSetting(
+      pattern,
+      ContentSettingsPattern::Wildcard(),
+      CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(),
+      CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(CONTENT_SETTING_ALLOW, host_content_settings_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ASK, otr_map->GetContentSetting(
+      host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string()));
+}
+
+TEST_F(HostContentSettingsMapTest, OffTheRecordPartialInheritDefault) {
+  // Permissions marked INHERIT_IN_INCOGNITO_EXCEPT_ALLOW in
+  // ContentSettingsRegistry (e.g. push & notifications) only inherit blocks
+  // from the regular to incognito default provider.
+  TestingProfile profile;
+  Profile* otr_profile = profile.GetOffTheRecordProfile();
+  HostContentSettingsMap* host_content_settings_map =
+      HostContentSettingsMapFactory::GetForProfile(&profile);
+  HostContentSettingsMap* otr_map =
+      HostContentSettingsMapFactory::GetForProfile(otr_profile);
+
+  GURL host("http://example.com/");
+
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            host_content_settings_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            host_content_settings_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            otr_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            otr_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
+
+  // BLOCK should be inherited from the main map to the incognito map.
+  host_content_settings_map->SetDefaultContentSetting(
+      CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            host_content_settings_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            otr_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_BLOCK,
+            otr_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
+
+  // ALLOW should not be inherited from the main map to the incognito map (but
+  // it still overwrites the BLOCK, hence incognito reverts to ASK).
+  host_content_settings_map->SetDefaultContentSetting(
+      CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            otr_map->GetDefaultContentSetting(
+                CONTENT_SETTINGS_TYPE_NOTIFICATIONS, NULL));
+  EXPECT_EQ(CONTENT_SETTING_ASK,
+            otr_map->GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                std::string()));
 }
 
 // For a single Unicode encoded pattern, check if it gets converted to punycode