Store base::Value in ContentSettingPatternSource instead of an enum

components/content_settings/core/common/content_settings.cc

Index: components/content_settings/core/common/content_settings.cc
diff --git a/components/content_settings/core/common/content_settings.cc b/components/content_settings/core/common/content_settings.cc
index 2333fe25072d1c4ea4680019d0bb961b10c3910f..69b51df0e1aa6d49de90e939cd49ada4f34b2818 100644
--- a/components/content_settings/core/common/content_settings.cc
+++ b/components/content_settings/core/common/content_settings.cc
@@ -9,6 +9,7 @@
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "build/build_config.h"
+//#include "components/content_settings/core/browser/content_settings_utils.h"

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

Is this needed?

[tbansal1, 2017/06/22 21:46:15]

Obviously not :P

 
 ContentSetting IntToContentSetting(int content_setting) {
   return ((content_setting < 0) ||
@@ -77,21 +78,39 @@ int ContentSettingTypeToHistogramValue(ContentSettingsType content_setting,
 ContentSettingPatternSource::ContentSettingPatternSource(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
-    ContentSetting setting,
+    const base::Value& setting_value,

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

To avoid additional copies, should we pass in a unique_ptr on construction? The
dictionaries could potentially be large.

[tbansal1, 2017/06/22 21:46:15]

Done.

     const std::string& source,
     bool incognito)
     : primary_pattern(primary_pattern),
       secondary_pattern(secondary_pattern),
-      setting(setting),
+      setting_value(base::MakeUnique<base::Value>(setting_value)),
       source(source),
       incognito(incognito) {}
 
-ContentSettingPatternSource::ContentSettingPatternSource()
-    : setting(CONTENT_SETTING_DEFAULT), incognito(false) {
-}
+ContentSettingPatternSource::ContentSettingPatternSource() : incognito(false) {}
 
 ContentSettingPatternSource::ContentSettingPatternSource(
-    const ContentSettingPatternSource& other) = default;
+    const ContentSettingPatternSource& other) {
+  primary_pattern = other.primary_pattern;
+  secondary_pattern = other.secondary_pattern;
+  source = other.source;
+  if (other.setting_value)
+    setting_value = other.setting_value->CreateDeepCopy();

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

I think CreateDeepCopy is deprecated? I think the base::Value copy constructor
is used instead.

[tbansal1, 2017/06/22 21:46:15]

Done.

+  incognito = other.incognito;
+}
+
+ContentSettingPatternSource& ContentSettingPatternSource::operator=(
+    const ContentSettingPatternSource& other) {
+  if (other.setting_value)
+    setting_value = other.setting_value->CreateDeepCopy();
+  primary_pattern = other.primary_pattern;
+  secondary_pattern = other.secondary_pattern;
+  source = other.source;
+  incognito = other.incognito;
+  return *this;

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

nit: please keep the order of assignments the same in these and match the order
of the member variables.

[tbansal1, 2017/06/22 21:46:15]

Done.

+}
+
+ContentSettingPatternSource::~ContentSettingPatternSource() {}

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

Did you consider exposing a GetContentSetting() function on this class rather
than calling ValueToContentSetting everywhere?

[tbansal1, 2017/06/22 21:46:16]

Done.

 
 RendererContentSettingRules::RendererContentSettingRules() {}