Replace SetContentSetting method of the content_settings::Provider interface with GetWebsiteSetting

chrome/browser/content_settings/host_content_settings_map.cc

 // Copyright (c) 2011 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 "chrome/browser/content_settings/host_content_settings_map.h"
 
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
@@ skipping 65 matching lines @@
     if (rule.primary_pattern == wildcard &&
         rule.secondary_pattern == wildcard) {
       return content_settings::ValueToContentSetting(rule.value.get());
     }
   }
   return CONTENT_SETTING_DEFAULT;
 }
 
 }  // namespace
 
+namespace base {
+class Value;

[Bernhard Bauer, 2011/11/11 14:00:40]

Here's a hint: You never need to forward-declare a class in a .cc file. If
you're just passing a pointer to somebody else, the class is already
forward-declared in another header (or otherwise that header wouldn't have been
able to declare the method you get it from), and if you actually need to
definition of the class, you directly #include its header.

[markusheintz_, 2011/11/14 11:15:10]

Another change in the matrix.

+}
+
 HostContentSettingsMap::HostContentSettingsMap(
     PrefService* prefs,
     ExtensionService* extension_service,
     bool incognito)
     : prefs_(prefs),
       is_off_the_record_(incognito) {
   content_settings::ObservableProvider* policy_provider =
       new content_settings::PolicyProvider(prefs_);
   policy_provider->AddObserver(this);
   content_settings_providers_[POLICY_PROVIDER] = policy_provider;
@@ skipping 134 matching lines @@
                           false);
   }
 }
 
 void HostContentSettingsMap::SetDefaultContentSetting(
     ContentSettingsType content_type,
     ContentSetting setting) {
   DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
   DCHECK(IsSettingAllowedForType(setting, content_type));
 
-  content_settings_providers_[DEFAULT_PROVIDER]->SetContentSetting(
+  scoped_ptr<base::Value> value(Value::CreateIntegerValue(setting));
+  content_settings_providers_[DEFAULT_PROVIDER]->SetWebsiteSetting(
       ContentSettingsPattern::Wildcard(),
       ContentSettingsPattern::Wildcard(),
       content_type,
       std::string(),
-      setting);
+      value.get());
+}
+
+void HostContentSettingsMap::SetWebsiteSetting(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    const std::string& resource_identifier,
+    base::Value* in_value) {
+  // The HostContentSettingsMap takes ownership of the passed value but the
+  // PrefProviders don't. PrefProvider make a copy of passed values. Therefor
+  // we put the passed |value| in a scoped pointer to clean it up.
+  scoped_ptr<base::Value> value(in_value);

[Bernhard Bauer, 2011/11/11 14:00:40]

So... maybe the PrefProviders should take ownership of the passed value? So far
I've seen them copy the value every time, and the passed value is freshly
constructed directly before, so there's an unnecessary allocation (plus the
interfaces of HostContentSettingsMap and Provider are slightly different).

The Provider could return a bool stating whether it takes ownership, and we
could stop the loop as soon as one provider returns true.

[markusheintz_, 2011/11/14 11:15:10]

Done.

+
+  DCHECK(IsValueAllowedForType(value.get(), content_type));
+  DCHECK(content_settings::SupportsResourceIdentifier(content_type) ||
+         resource_identifier.empty());
+  for (ProviderIterator provider = content_settings_providers_.begin();
+       provider != content_settings_providers_.end();
+       ++provider) {
+    provider->second->SetWebsiteSetting(
+        primary_pattern,
+        secondary_pattern,
+        content_type,
+        resource_identifier,
+        value.get());
+  }
 }
 
 void HostContentSettingsMap::SetContentSetting(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     const std::string& resource_identifier,
     ContentSetting setting) {
   DCHECK_NE(content_type, CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE);
-  DCHECK(IsSettingAllowedForType(setting, content_type));
-  DCHECK(content_settings::SupportsResourceIdentifier(content_type) ||
-         resource_identifier.empty());
-  for (ProviderIterator provider = content_settings_providers_.begin();
-       provider != content_settings_providers_.end();
-       ++provider) {
-    provider->second->SetContentSetting(
-        primary_pattern,
-        secondary_pattern,
-        content_type,
-        resource_identifier,
-        setting);
-  }
+  scoped_ptr<base::Value> value;

[Bernhard Bauer, 2011/11/11 14:00:40]

You don't need a scoped_ptr if you're releasing it anyway.

[markusheintz_, 2011/11/14 11:15:10]

Yeah. I saw this somewhere else and though it might be a good practice to be
save. Certainly an overkill here. But you can't be save enough.

I changed it anyway. 

+  if (setting != CONTENT_SETTING_DEFAULT)
+    value.reset(Value::CreateIntegerValue(setting));
+  SetWebsiteSetting(primary_pattern,
+                    secondary_pattern,
+                    content_type,
+                    resource_identifier,
+                    value.release());
 }
 
 void HostContentSettingsMap::AddExceptionForURL(
     const GURL& primary_url,
     const GURL& secondary_url,
     ContentSettingsType content_type,
     const std::string& resource_identifier,
     ContentSetting setting) {
   // TODO(markusheintz): Until the UI supports pattern pairs, both urls must
   // match.
@@ skipping 16 matching lines @@
 
 void HostContentSettingsMap::ClearSettingsForOneType(
     ContentSettingsType content_type) {
   for (ProviderIterator provider = content_settings_providers_.begin();
        provider != content_settings_providers_.end();
        ++provider) {
     provider->second->ClearAllContentSettingsRules(content_type);
   }
 }
 
+bool HostContentSettingsMap::IsValueAllowedForType(
+    const base::Value* value, ContentSettingsType type) {
+  return IsSettingAllowedForType(
+      content_settings::ValueToContentSetting(value), type);
+}
+
 // static
 bool HostContentSettingsMap::IsSettingAllowedForType(
     ContentSetting setting, ContentSettingsType content_type) {
   // Intents content settings are hidden behind a switch for now.
   if (content_type == CONTENT_SETTINGS_TYPE_INTENTS &&
       !CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableWebIntents))
     return false;
 
   // DEFAULT, ALLOW and BLOCK are always allowed.
@@ skipping 130 matching lines @@
     }
   }
 
   if (info) {
     info->source = content_settings::SETTING_SOURCE_NONE;
     info->primary_pattern = ContentSettingsPattern();
     info->secondary_pattern = ContentSettingsPattern();
   }
   return NULL;
 }