Stop inheriting push notification permissions from regular to incognito

components/content_settings/core/browser/content_settings_utils.cc

 // 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.
 
 #include "components/content_settings/core/browser/content_settings_utils.h"
 
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 118 matching lines @@
 }
 
 scoped_ptr<base::Value> ContentSettingToValue(ContentSetting setting) {
   if (setting <= CONTENT_SETTING_DEFAULT ||
       setting >= CONTENT_SETTING_NUM_SETTINGS) {
     return nullptr;
   }
   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,
     ContentSettingsType content_type,
     const std::string& resource_identifier,
     bool include_incognito,
     ContentSettingsPattern* primary_pattern,
     ContentSettingsPattern* secondary_pattern) {
   if (include_incognito) {
     // Check incognito-only specific settings. It's essential that the
     // |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)
       return value;
   }
   // 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 &&

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

Please add a comment what is happening here (maybe an empty line as well, so
it's easier to read).

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

Done (in header declaration of helper function).

+      ValueToContentSetting(value.get()) == CONTENT_SETTING_ALLOW) {
+    if (WebsiteSettingsRegistry::GetInstance()->Get(content_type)
+                                              ->incognito_behavior()
+        == WebsiteSettingsInfo::INHERIT_IN_INCOGNITO_EXCEPT_ALLOW) {
+      value = ContentSettingToValue(CONTENT_SETTING_ASK);

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

As mentioned in HostContentSettingsMap, please DCHECK that ASK is a valid value.

By the way, now we have this logic for exceptions and for default values in
different files. It would be less error-prone to generalize this (since the code
is similar), and make it a helper function in host_content_settings_map.cc.

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

Done - HostContentSettingsMap::CoerceSettingInheritedToIncognito. Note that I
had to mark this function a friend of HostContentSettingsMap; alternatively I
could make CoerceSettingInheritedToIncognito public, but I'm not sure that makes
sense?

+    }
+  }
+  return value;
 }
 
-base::Value* GetContentSettingValueAndPatterns(
+scoped_ptr<base::Value> GetContentSettingValueAndPatterns(
     RuleIterator* rule_iterator,
     const GURL& primary_url,
     const GURL& secondary_url,
     ContentSettingsPattern* primary_pattern,
     ContentSettingsPattern* secondary_pattern) {
   while (rule_iterator->HasNext()) {
     const Rule& rule = rule_iterator->Next();
     if (rule.primary_pattern.Matches(primary_url) &&
         rule.secondary_pattern.Matches(secondary_url)) {
       if (primary_pattern)
         *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,
                                     RendererContentSettingRules* rules) {
   map->GetSettingsForOneType(
       CONTENT_SETTINGS_TYPE_IMAGES,
       ResourceIdentifier(),
       &(rules->image_rules));
   map->GetSettingsForOneType(
       CONTENT_SETTINGS_TYPE_JAVASCRIPT,
       ResourceIdentifier(),
       &(rules->script_rules));
 }
 
 }  // namespace content_settings