Store base::Value in ContentSettingPatternSource instead of an enum

components/content_settings/core/common/content_settings.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 "components/content_settings/core/common/content_settings.h"
 
 #include "base/containers/hash_tables.h"
 #include "base/logging.h"
 #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) ||
           (content_setting >= CONTENT_SETTING_NUM_SETTINGS)) ?
       CONTENT_SETTING_DEFAULT : static_cast<ContentSetting>(content_setting);
 }
 
 struct HistogramValue {
   ContentSettingsType type;
   int value;
@@ skipping 48 matching lines @@
   }
 
   DCHECK(base::ContainsKey(kMap, content_setting));
   *num_values = arraysize(kHistogramValue);
   return kMap[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) {
+  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(
-    const ContentSettingPatternSource& other) = default;
+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() {}
 
 RendererContentSettingRules::~RendererContentSettingRules() {}