Stop inheriting push notification permissions from regular to incognito

components/content_settings/core/browser/host_content_settings_map.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Maps hostnames to custom content settings.  Written on the UI thread and read
 // on any thread.  One instance per profile.
 
 #ifndef COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_HOST_CONTENT_SETTINGS_MAP_H_
 #define COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_HOST_CONTENT_SETTINGS_MAP_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
 #include "base/prefs/pref_change_registrar.h"
 #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"
 #include "components/keyed_service/core/refcounted_keyed_service.h"
 
 class ExtensionService;
 class GURL;
 class PrefService;
 
 namespace base {
@@ skipping 198 matching lines @@
       const ContentSettingsPattern& secondary_pattern,
       ContentSettingsType content_type);
 
   // Adds/removes an observer for content settings changes.
   void AddObserver(content_settings::Observer* observer);
   void RemoveObserver(content_settings::Observer* 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);
 
  private:
   friend class base::RefCountedThreadSafe<HostContentSettingsMap>;
   friend class HostContentSettingsMapTest_NonDefaultSettings_Test;
 
   typedef std::map<ProviderType, content_settings::ProviderInterface*>
       ProviderMap;
   typedef ProviderMap::iterator ProviderIterator;
@@ skipping 37 matching lines @@
 
   // Returns the single content setting |value| with a toggle for if it
   // takes the global on/off switch into account.
   scoped_ptr<base::Value> GetWebsiteSettingInternal(
       const GURL& primary_url,
       const GURL& secondary_url,
       ContentSettingsType content_type,
       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
   // This starts as the thread ID of the thread that constructs this
   // object, and remains until used by a different thread, at which
   // point it is set to base::kInvalidThreadId. This allows us to
   // DCHECK on unsafe usage of content_settings_providers_ (they
   // should be set up on a single thread, after which they are
   // immutable).
   mutable base::PlatformThreadId used_from_thread_id_;
@@ skipping 11 matching lines @@
   ProviderMap content_settings_providers_;
 
   base::ThreadChecker thread_checker_;
 
   base::ObserverList<content_settings::Observer> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(HostContentSettingsMap);
 };
 
 #endif  // COMPONENTS_CONTENT_SETTINGS_CORE_BROWSER_HOST_CONTENT_SETTINGS_MAP_H_