Store base::Value in ContentSettingPatternSource instead of an enum

components/content_settings/core/browser/host_content_settings_map.cc

Index: components/content_settings/core/browser/host_content_settings_map.cc
diff --git a/components/content_settings/core/browser/host_content_settings_map.cc b/components/content_settings/core/browser/host_content_settings_map.cc
index 92c7e2a322c3fb6d6fb80885a36d9bb6fa7d5ca9..de58c6968c746cb190268440a8a00e4f684b6162 100644
--- a/components/content_settings/core/browser/host_content_settings_map.cc
+++ b/components/content_settings/core/browser/host_content_settings_map.cc
@@ -29,6 +29,7 @@
 #include "components/content_settings/core/browser/content_settings_utils.h"
 #include "components/content_settings/core/browser/website_settings_registry.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
+#include "components/content_settings/core/common/content_settings_utils.h"
 #include "components/content_settings/core/common/pref_names.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
@@ -529,8 +530,10 @@ void HostContentSettingsMap::MigrateDomainScopedSettings(bool after_sync) {
       if (setting_entry.source != "preference")
         continue;
       // Migrate ALLOW settings only.
-      if (setting_entry.setting != CONTENT_SETTING_ALLOW)
+      if (content_settings::ValueToContentSetting(
+              setting_entry.setting_value.get()) != CONTENT_SETTING_ALLOW) {
         continue;
+      }
       // Skip default settings.
       if (setting_entry.primary_pattern == ContentSettingsPattern::Wildcard())
         continue;
@@ -746,12 +749,20 @@ void HostContentSettingsMap::AddSettingsForOneType(
         rule.value.get() &&
         rule.primary_pattern != ContentSettingsPattern::Wildcard()) {
       setting_value = CONTENT_SETTING_ALLOW;
+      settings->push_back(ContentSettingPatternSource(
+          rule.primary_pattern, rule.secondary_pattern,
+          *(content_settings::ContentSettingToValue(setting_value)),
+          kProviderNamesSourceMap[provider_type].provider_name, incognito));
     } else {
-      setting_value = content_settings::ValueToContentSetting(rule.value.get());
+      // TODO (tbansal): Consider storing the |rule.value| directly in
+      // ContentSettingPatternSource. Also, consider passing  |content_type|
+      // to content_settings::ValueToContentSetting and
+      // content_settings::ContentSettingToValue.
+      // setting_value = content_settings::ValueToContentSetting();
+      settings->push_back(ContentSettingPatternSource(
+          rule.primary_pattern, rule.secondary_pattern, *(rule.value),
+          kProviderNamesSourceMap[provider_type].provider_name, incognito));

[raymes, 2017/06/22 03:41:26]

I'm not sure I understand the changes here. Could we just preserve the existing
logic? i.e.

base::Value setting_value;

if ... {
  ...
  setting_value = base::Value(CONTENT_SETTING_ALLOW);
} else {
  setting_value = base::Value(*rule.value);
}
...

     }
-    settings->push_back(ContentSettingPatternSource(
-        rule.primary_pattern, rule.secondary_pattern, setting_value,
-        kProviderNamesSourceMap[provider_type].provider_name, incognito));
   }
 }