Stop inheriting push notification permissions from regular to incognito

components/content_settings/core/browser/host_content_settings_map.h

Index: components/content_settings/core/browser/host_content_settings_map.h
diff --git a/components/content_settings/core/browser/host_content_settings_map.h b/components/content_settings/core/browser/host_content_settings_map.h
index d4935890889b6592f5ec035b6b560ba4bc67a63a..c9f05eb5f3e8b68377c0447f8e8ba38d4500b94d 100644
--- a/components/content_settings/core/browser/host_content_settings_map.h
+++ b/components/content_settings/core/browser/host_content_settings_map.h
@@ -19,6 +19,7 @@
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_checker.h"
 #include "components/content_settings/core/browser/content_settings_observer.h"
+#include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/content_settings_types.h"
@@ -237,6 +238,25 @@ class HostContentSettingsMap : public content_settings::Observer,
   // Schedules any pending lossy website settings to be written to disk.
   void FlushLossyWebsiteSettings();
 
+  // For internal use only, except in tests.
+  static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal(
+      const content_settings::ProviderInterface* provider,
+      const GURL& primary_url,
+      const GURL& secondary_url,
+      ContentSettingsType content_type,
+      const std::string& resource_identifier,
+      bool include_incognito,
+      ContentSettingsPattern* primary_pattern,
+      ContentSettingsPattern* secondary_pattern);
+
+  // For internal use only, except in tests.
+  static scoped_ptr<base::Value> GetContentSettingValueAndPatternsInternal(

[raymes, 2015/12/03 01:56:49]

It's strange to have public functions that are called "internal". I think these
should be private functions and friend the test classes/functions. That will
discourage others from using them in production code. 

[johnme, 2015/12/03 19:06:40]

I can do that for
GetContentSettingValueAndPatternsInternal(content_settings::ProviderInterface*
provider, ...), since it is only used by GetContentSettingValue in
components/content_settings/core/test/content_settings_test_utils.cc. But I
can't easily do that for
GetContentSettingValueAndPatternsInternal(content_settings::RuleIterator*
rule_iterator, ...) since its (only) usage is in the function
GetContentSettingFromStore in
chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc,
which has no corresponding header file.

What would you recommend I do here?

[raymes, 2015/12/03 22:19:42]

Yeah it's annoying isn't it - you would have to move that function out of the
anonymous namespace which is fine. But then you would probably have to forward
declare it in this file, which would be annoying. I'll give 2 more concrete
options that I would be ok with:
1) Inside content_settings_test_utils.h/cc create a new class called
ContentSettingsTestUtils. Make the existing functions in that file static
members of that class. Friend ContentSettingsTestUtils from
HostContentSettingsMap. Publicly expose GetContentSettingValueAndPatterns from
ContentSettingsTestUtils for use inside GetContentSettingFromStore. This would
probably be my preferred suggestion because we would consolidate all the test
APIs in a single place.

2) Make these 2 GetContentSettingValueAndPatterns functions standalone functions
inside host_content_settings_map.cc (in the anonymous namespace) and expose them
as public static functions called GetContentSettingValueAndPatternsForTest in
the interface. This would make it clear that they're really just internal
functions that are exposed for testing.
 

[johnme, 2015/12/04 17:14:32]

Done #1 (called the class content_settings::TestUtils).

+      content_settings::RuleIterator* rule_iterator,
+      const GURL& primary_url,
+      const GURL& secondary_url,
+      ContentSettingsPattern* primary_pattern,
+      ContentSettingsPattern* secondary_pattern);
+
   // Passes ownership of |clock|.
   void SetPrefClockForTesting(scoped_ptr<base::Clock> clock);
 
@@ -294,6 +314,12 @@ class HostContentSettingsMap : public content_settings::Observer,
       const std::string& resource_identifier,
       content_settings::SettingInfo* info) const;
 
+  // Prevents content settings marked INHERIT_IN_INCOGNITO_EXCEPT_ALLOW from
+  // inheriting CONTENT_SETTING_ALLOW settings from regular to incognito.
+  static ContentSetting CoerceSettingInheritedToIncognito(
+      ContentSettingsType content_type,
+      ContentSetting setting);
+
   content_settings::PrefProvider* GetPrefProvider();
 
 #ifndef NDEBUG