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

chrome/browser/content_settings/host_content_settings_map.cc

Index: chrome/browser/content_settings/host_content_settings_map.cc
diff --git a/chrome/browser/content_settings/host_content_settings_map.cc b/chrome/browser/content_settings/host_content_settings_map.cc
index b0baa0425bf6024d7d1ca7959f192a8d9a8dfac3..1936df01343a3f8e78c18c5b971f425c9ee966d7 100644
--- a/chrome/browser/content_settings/host_content_settings_map.cc
+++ b/chrome/browser/content_settings/host_content_settings_map.cc
@@ -83,6 +83,10 @@ ContentSetting GetDefaultSetting(
 
 }  // 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,
@@ -237,36 +241,58 @@ void HostContentSettingsMap::SetDefaultContentSetting(
   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::SetContentSetting(
+void HostContentSettingsMap::SetWebsiteSetting(
     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));
+    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->SetContentSetting(
+    provider->second->SetWebsiteSetting(
         primary_pattern,
         secondary_pattern,
         content_type,
         resource_identifier,
-        setting);
+        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);
+  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,
@@ -301,6 +327,12 @@ void HostContentSettingsMap::ClearSettingsForOneType(
   }
 }
 
+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) {