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

chrome/browser/content_settings/content_settings_default_provider.cc

Index: chrome/browser/content_settings/content_settings_default_provider.cc
diff --git a/chrome/browser/content_settings/content_settings_default_provider.cc b/chrome/browser/content_settings/content_settings_default_provider.cc
index d1ff97751f78043ac69cc0b3469a54c374e1b846..d0dfaf9915a6a3456d5fc4c45cbf1c235e492e4c 100644
--- a/chrome/browser/content_settings/content_settings_default_provider.cc
+++ b/chrome/browser/content_settings/content_settings_default_provider.cc
@@ -70,6 +70,7 @@ class DefaultRuleIterator : public RuleIterator {
   }
 
  private:
+  // TODO(markusheintz): |ContentSetting| should be replace with a |Value|.

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

Nit: "replaced".

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

Done.

   ContentSetting setting_;
 };
 
@@ -109,8 +110,10 @@ DefaultProvider::DefaultProvider(PrefService* prefs, bool incognito)
 
   // Read global defaults.
   ReadDefaultSettings(true);
-  if (default_content_settings_.settings[CONTENT_SETTINGS_TYPE_COOKIES] ==
-      CONTENT_SETTING_BLOCK) {
+
+  int int_value;
+  default_settings_[CONTENT_SETTINGS_TYPE_COOKIES]->GetAsInteger(&int_value);

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

Can you use the ValueToContentSetting helper method here? That would also be
able to deal with edge cases like if there's no value for a content type.

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

Done.

+  if (IntToContentSetting(int_value) == CONTENT_SETTING_BLOCK) {
     UserMetrics::RecordAction(
         UserMetricsAction("CookieBlockingEnabledPerDefault"));
   } else {
@@ -126,12 +129,12 @@ DefaultProvider::DefaultProvider(PrefService* prefs, bool incognito)
 DefaultProvider::~DefaultProvider() {
 }
 
-void DefaultProvider::SetContentSetting(
+void DefaultProvider::SetWebsiteSetting(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     const ResourceIdentifier& resource_identifier,
-    ContentSetting setting) {
+    const Value* value) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prefs_);
 
@@ -157,27 +160,37 @@ void DefaultProvider::SetContentSetting(
     // |PrefService::SetInteger()| send out notifications. As a response, the
     // upper layers may call |GetAllContentSettingRules| which acquires |lock_|
     // again.
+    // TODO(markusheintz): Hardcoded default settings should be handled by the
+    // HostContentSettingsMap itself. The DefaultProvider should only deal with
+    // user set default values and should return NULL if no user defined
+    // settings are avaliable.
+    scoped_ptr<base::Value> default_value(
+          Value::CreateIntegerValue(kDefaultSettings[content_type]));

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

If you get the integer value from |value| you can avoid allocating a new value
here. 

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

Done.

     {
       base::AutoLock lock(lock_);
-      if (setting == CONTENT_SETTING_DEFAULT ||
-          setting == kDefaultSettings[content_type]) {
-        default_content_settings_.settings[content_type] =
-            kDefaultSettings[content_type];
+      if (value == NULL ||
+          value->Equals(default_value.get())) {
+        // If |value| is NULL we need to reset the default setting the the
+        // hardcoded default.
+        default_settings_[content_type].reset(
+            Value::CreateIntegerValue(kDefaultSettings[content_type]));
+
+        // If |value| is null or equal to the hardcoded default setting then
+        // remove the corresponding pref entry as it is not needed.
         default_settings_dictionary->RemoveWithoutPathExpansion(dictionary_path,
                                                                 NULL);
       } else {
-        default_content_settings_.settings[content_type] = setting;
+        default_settings_[content_type].reset(value->DeepCopy());
         default_settings_dictionary->SetWithoutPathExpansion(
-            dictionary_path, Value::CreateIntegerValue(setting));
+            dictionary_path, value->DeepCopy());
       }
     }
 
     // Keep the obsolete pref in sync as long as backwards compatibility is
     // required. This is required to keep sync working correctly.
     if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
-       prefs_->SetInteger(prefs::kGeolocationDefaultContentSetting,
-                          setting == CONTENT_SETTING_DEFAULT ?
-                              kDefaultSettings[content_type] : setting);
+       prefs_->Set(prefs::kGeolocationDefaultContentSetting,
+                   value == NULL ? *default_value : *value);
     }
   }
 
@@ -193,8 +206,14 @@ RuleIterator* DefaultProvider::GetRuleIterator(
     bool incognito) const {
   base::AutoLock lock(lock_);
   if (resource_identifier.empty()) {
-    return new DefaultRuleIterator(
-        default_content_settings_.settings[content_type]);
+    int int_value;

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

Initialize this value please.

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

Done.

+    ValueMap::const_iterator it(default_settings_.find(content_type));
+    if (it != default_settings_.end()) {
+      it->second->GetAsInteger(&int_value);
+    } else {
+      NOTREACHED();
+    }
+    return new DefaultRuleIterator(ContentSetting(int_value));
   } else {
     return new EmptyRuleIterator();
   }
@@ -255,76 +274,87 @@ void DefaultProvider::ReadDefaultSettings(bool overwrite) {
       prefs_->GetDictionary(prefs::kDefaultContentSettings);
 
   if (overwrite)
-    default_content_settings_ = ContentSettings();
+    default_settings_.clear();
 
   // Careful: The returned value could be NULL if the pref has never been set.
-  if (default_settings_dictionary) {
-    GetSettingsFromDictionary(default_settings_dictionary,
-                              &default_content_settings_);
-  }
+  if (default_settings_dictionary)
+    GetSettingsFromDictionary(default_settings_dictionary);
+
   ForceDefaultsToBeExplicit();
 }
 
 void DefaultProvider::ForceDefaultsToBeExplicit() {
   for (int i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) {
-    if (default_content_settings_.settings[i] == CONTENT_SETTING_DEFAULT)
-      default_content_settings_.settings[i] = kDefaultSettings[i];
+    ContentSettingsType type = ContentSettingsType(i);
+    if (!default_settings_[type].get())
+      default_settings_[type].reset(
+          Value::CreateIntegerValue(kDefaultSettings[i]));
   }
 }
 
 void DefaultProvider::GetSettingsFromDictionary(
-    const DictionaryValue* dictionary,
-    ContentSettings* settings) {
+    const DictionaryValue* dictionary) {
   for (DictionaryValue::key_iterator i(dictionary->begin_keys());
        i != dictionary->end_keys(); ++i) {
     const std::string& content_type(*i);
     for (size_t type = 0; type < CONTENT_SETTINGS_NUM_TYPES; ++type) {

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

Can you look up the |ContentSettingsType| for |content_type| (I know, confusing
names) directly with a helper method instead of this for loop?

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

Yes I could.But I thought I keep the change small first. There is so much more
to fix in this class.

       if (content_type == GetTypeName(ContentSettingsType(type))) {
-        int setting = CONTENT_SETTING_DEFAULT;
-        bool found = dictionary->GetIntegerWithoutPathExpansion(content_type,
-                                                                &setting);
+        base::Value* value = NULL;
+        bool found = dictionary->GetWithoutPathExpansion(content_type, &value);
+
         DCHECK(found);
-        settings->settings[type] = IntToContentSetting(setting);
+        default_settings_[ContentSettingsType(type)].reset(value->DeepCopy());

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

If we don't find a value, the |DeepCopy| will crash in a Release build. If you
want to avoid that, you could keep the old version and just change this line to
create an IntegerValue from the |setting|.

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

Done.

+
         break;
       }
     }
   }
   // Migrate obsolete cookie prompt mode/

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

Nit: Slash at the end of the comment

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

Done.

-  if (settings->settings[CONTENT_SETTINGS_TYPE_COOKIES] ==
-      CONTENT_SETTING_ASK)
-    settings->settings[CONTENT_SETTINGS_TYPE_COOKIES] = CONTENT_SETTING_BLOCK;
+  if (ValueEqualsContentSetting(

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

Oh, we have that method? Then you could use it in line 168 as well.

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

Obsolete as I removed the method ValueEqualsContentSetting.

+          default_settings_[CONTENT_SETTINGS_TYPE_COOKIES].get(),
+          CONTENT_SETTING_ASK)) {
+    default_settings_[CONTENT_SETTINGS_TYPE_COOKIES].reset(
+        Value::CreateIntegerValue(CONTENT_SETTING_BLOCK));
+  }
 
-  settings->settings[CONTENT_SETTINGS_TYPE_PLUGINS] =
-      ClickToPlayFixup(CONTENT_SETTINGS_TYPE_PLUGINS,
-                       settings->settings[CONTENT_SETTINGS_TYPE_PLUGINS]);
+  if (default_settings_[CONTENT_SETTINGS_TYPE_PLUGINS].get()) {
+    ContentSetting plugin_setting = ValueToContentSetting(
+        default_settings_[CONTENT_SETTINGS_TYPE_PLUGINS].get());
+    plugin_setting =
+        ClickToPlayFixup(CONTENT_SETTINGS_TYPE_PLUGINS, plugin_setting);
+    default_settings_[CONTENT_SETTINGS_TYPE_PLUGINS].reset(
+        Value::CreateIntegerValue(plugin_setting));
+  }
 }
 
 void DefaultProvider::MigrateObsoleteNotificationPref() {
   if (prefs_->HasPrefPath(prefs::kDesktopNotificationDefaultContentSetting)) {
-    ContentSetting setting = IntToContentSetting(
-        prefs_->GetInteger(prefs::kDesktopNotificationDefaultContentSetting));
-    SetContentSetting(
+    const base::Value* value = prefs_->FindPreference(
+        prefs::kDesktopNotificationDefaultContentSetting)->GetValue();
+    // Do not clear the old preference yet as long as we need to maintain
+    // backward compatibility.
+    SetWebsiteSetting(
         ContentSettingsPattern::Wildcard(),
         ContentSettingsPattern::Wildcard(),
         CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
         std::string(),
-        setting);
+        value);
     prefs_->ClearPref(prefs::kDesktopNotificationDefaultContentSetting);
   }
 }
 
 void DefaultProvider::MigrateObsoleteGeolocationPref() {
   if (prefs_->HasPrefPath(prefs::kGeolocationDefaultContentSetting)) {
-    ContentSetting setting = IntToContentSetting(
-        prefs_->GetInteger(prefs::kGeolocationDefaultContentSetting));
+    const base::Value* value = prefs_->FindPreference(
+        prefs::kGeolocationDefaultContentSetting)->GetValue();
     // Do not clear the old preference yet as long as we need to maintain
     // backward compatibility.
-    SetContentSetting(
+    SetWebsiteSetting(
         ContentSettingsPattern::Wildcard(),
         ContentSettingsPattern::Wildcard(),
         CONTENT_SETTINGS_TYPE_GEOLOCATION,
         std::string(),
-        setting);
+        value);
   }
 }