Stop inheriting push notification permissions from regular to incognito

components/content_settings/core/browser/content_settings_utils.cc

Index: components/content_settings/core/browser/content_settings_utils.cc
diff --git a/components/content_settings/core/browser/content_settings_utils.cc b/components/content_settings/core/browser/content_settings_utils.cc
index b989875867893cfee66aa59e6d5033c6af88989d..06cf8802c2a00068deeba5af5e8bf5e6642f076c 100644
--- a/components/content_settings/core/browser/content_settings_utils.cc
+++ b/components/content_settings/core/browser/content_settings_utils.cc
@@ -136,7 +136,7 @@ scoped_ptr<base::Value> ContentSettingToValue(ContentSetting setting) {
   return make_scoped_ptr(new base::FundamentalValue(setting));
 }
 
-base::Value* GetContentSettingValueAndPatterns(
+scoped_ptr<base::Value> GetContentSettingValueAndPatterns(
     const ProviderInterface* provider,
     const GURL& primary_url,
     const GURL& secondary_url,
@@ -150,8 +150,9 @@ base::Value* GetContentSettingValueAndPatterns(
     // |RuleIterator| gets out of scope before we get a rule iterator for the
     // normal mode.
     scoped_ptr<RuleIterator> incognito_rule_iterator(
-        provider->GetRuleIterator(content_type, resource_identifier, true));
-    base::Value* value = GetContentSettingValueAndPatterns(
+        provider->GetRuleIterator(content_type, resource_identifier,
+                                  true /* incognito */));
+    scoped_ptr<base::Value> value = GetContentSettingValueAndPatterns(
         incognito_rule_iterator.get(), primary_url, secondary_url,
         primary_pattern, secondary_pattern);
     if (value)
@@ -159,13 +160,20 @@ base::Value* GetContentSettingValueAndPatterns(
   }
   // No settings from the incognito; use the normal mode.
   scoped_ptr<RuleIterator> rule_iterator(
-      provider->GetRuleIterator(content_type, resource_identifier, false));
-  return GetContentSettingValueAndPatterns(
+      provider->GetRuleIterator(content_type, resource_identifier,
+                                false /* incognito */));
+  scoped_ptr<base::Value> value = GetContentSettingValueAndPatterns(
       rule_iterator.get(), primary_url, secondary_url,
       primary_pattern, secondary_pattern);
+  if (value && include_incognito) {
+    value = ContentSettingToValue(
+        HostContentSettingsMap::CoerceSettingInheritedToIncognito(
+            content_type, ValueToContentSetting(value.get())));
+  }

[raymes, 2015/12/01 03:19:29]

What I was suggesting is to move this code into
HostContentSettingsMap::GetWebsiteSettingInternal (which calls this function)
instead of having it here. Will that break something?

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

Either that, or we can just simply wrap the callsites of
GetContentSettingValueAndPatterns with CoerceSettingInheritedToIncognito instead
of calling it here directly.

It's just now weird that HCSM calls a method in utils/ which in turn uses a
method from HCSM.

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

I'm not quite sure what you're asking. If you're suggesting to move just the
added code to HostContentSettingsMap::GetWebsiteSettingInternal (i.e. wrap the
result of GetContentSettingValueAndPatterns), then that doesn't work because
Coerce should only be called for settings inherited from regular to incognito,
not for incognito-specific settings, and there's no way to distinguish the two
at this point.

One solution would be to return a pair<scoped_ptr<base::Value>, bool>, where the
bool allows the caller to tell whether the setting is incognito-specific.

Another option would be to move the GetContentSettingValueAndPatterns functions
into HostContentSettingsMap. I've tried that in this iteration of the patch. The
functions are also used in two other places,
components/content_settings/core/test/content_settings_test_utils.cc and
chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc,
so I renamed them to *Internal but left them public, with a comment noting that
they are only public for tests. This allowed me to stop marking
GetContentSettingValueAndPatterns as a friend function of
HostContentSettingsMap.

Does that seem better?

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

Wrapping doesn't work, as Coerce should only be called for settings inherited
from regular to incognito, not for incognito-specific settings. See also my
reply to raymes.

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

I don't quite understand why wrapping doesn't work? Shouldn't this code do the
equivalent of what you have here (this code would be inside
GetWebsiteSettingInternal):

scoped_ptr<base::Value> val =
GetContentSettingValueAndPatterns(provider->second, primary_url, secondary_url,
content_type, resource_identifier, is_off_the_record_, primary_pattern,
secondary_pattern);

if (value && is_off_the_record_) {
  value = ContentSettingToValue(CoerceSettingInheritedToIncognito(content_type,
ValueToContentSetting(value.get())));
}

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

That doesn't work: suppose the provider has an ALLOW rule that is
incognito-specific, and is_off_the_record_ is true.
GetContentSettingValueAndPatterns's include_incognito will be true, so it will
return the ALLOW Value. And since is_off_the_record_ is true, it will coerce
this to ASK. But in this case, GetWebsiteSettingsInternal should *not* have
coerced ALLOW to ASK, since the setting was incognito-specific (only settings
inherited from regular to incognito should be coerced).

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

I'm confused though - AFAICT the code I wrote is functionally equivalent to the
code you had in your CL. Sorry I'm probably missing something but it doesn't
matter too much given your new CL.

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

My code does (simplified):

if (include_incognito && has_incognito_specific_setting)
  return incognito_specific_setting;
else if (has_regular_setting)
  return Coerce(regular_setting);
else
  return Value();

The code you wrote does (simplified):

if (include_incognito && has_incognito_specific_setting)
  return Coerce(incognito_specific_setting);
else if (has_regular_setting)
  return Coerce(regular_setting);
else
  return Value();

The extra Coerce is the problem.

+  return value;
 }
 
-base::Value* GetContentSettingValueAndPatterns(
+scoped_ptr<base::Value> GetContentSettingValueAndPatterns(
     RuleIterator* rule_iterator,
     const GURL& primary_url,
     const GURL& secondary_url,
@@ -179,10 +187,10 @@ base::Value* GetContentSettingValueAndPatterns(
         *primary_pattern = rule.primary_pattern;
       if (secondary_pattern)
         *secondary_pattern = rule.secondary_pattern;
-      return rule.value.get()->DeepCopy();
+      return make_scoped_ptr(rule.value.get()->DeepCopy());
     }
   }
-  return NULL;
+  return scoped_ptr<base::Value>();
 }
 
 void GetRendererContentSettingRules(const HostContentSettingsMap* map,