Store base::Value in ContentSettingPatternSource instead of an enum

components/content_settings/core/common/content_settings.h

Index: components/content_settings/core/common/content_settings.h
diff --git a/components/content_settings/core/common/content_settings.h b/components/content_settings/core/common/content_settings.h
index c441e280eaa5f42cf6c21b1bdbf04315ed740357..e28e51eed34e8527a072345871cc4672b7b04cdf 100644
--- a/components/content_settings/core/common/content_settings.h
+++ b/components/content_settings/core/common/content_settings.h
@@ -7,9 +7,12 @@
 
 #include <stddef.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
+#include "base/memory/ptr_util.h"
+#include "base/values.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 
@@ -43,14 +46,19 @@ int ContentSettingTypeToHistogramValue(ContentSettingsType content_setting,
 struct ContentSettingPatternSource {
   ContentSettingPatternSource(const ContentSettingsPattern& primary_pattern,
                               const ContentSettingsPattern& secondary_patttern,
-                              ContentSetting setting,
+                              std::unique_ptr<base::Value> setting_value,
                               const std::string& source,
                               bool incognito);
   ContentSettingPatternSource(const ContentSettingPatternSource& other);
   ContentSettingPatternSource();
+  ContentSettingPatternSource& operator=(
+      const ContentSettingPatternSource& other);
+  ~ContentSettingPatternSource();
+  ContentSetting GetContentSetting() const;
+
   ContentSettingsPattern primary_pattern;
   ContentSettingsPattern secondary_pattern;
-  ContentSetting setting;
+  std::unique_ptr<base::Value> setting_value;

[raymes, 2017/06/27 03:49:33]

I was just chatting with mgiuca@ about this. It looks like it's now safe to
store base::Value's directly as members. This would allow us to avoid defining
the copy constructor and assignment operator. That means that we can change the
constructor back to pass a base::Value by reference (sorry for the churn) and
just set up the member in the constructor. I realized we're not getting any
benefit from it right now anyway because we're only ever passing in int's as the
base::Value (and we're doing a deep copy in the constructor anyway).

[tbansal1, 2017/06/27 07:51:37]

qq: With unique_ptr, it seems that the ctor requires only one deep copy (because
the created ContentSettingPatternSource object simply takes the ownership of the
already constructed base::Value unique_ptr). When passing the ref, and having
the base::Value as the member, it seems two deep copies would be required. Is my
understanding correct? Is the extra overhead associated with not using
unique_ptr a concern?

[raymes, 2017/06/27 08:59:54]

My understanding is that with the current patchset, a copy will be created in
the MakeUnique call inside the constructor. That's the only copy at present. We
could actually eliminate that copy since the ContentSettingsPatternSource takes
ownership of the pointer.

If we passed by ref, the only copy would be in the initialization of the member
variable (which invokes the copy constructor). This is one more copy than is
strictly necessary but I think worth the tradeoff in simplicity.

The other alternative would be to keep the unique_ptr but not define a copy
constructor/assignment operator and only allow the struct to be moved. This
would remove all copies. But I suspect this may require additional changes in
the code and I'm not sure how hard it would be depending on how the struct is
used.

[tbansal1, 2017/06/27 09:56:34]

I see. Storing the base::value directly as a member (instead of unique_ptr) may
make the mojo interface less unreadable. mojo/common/values.typemap maps
mojo.common.mojom.Value to a std::unique_ptr<base::Value>. That makes it
possible for components/content_settings/core/common/content_settings.typemap to
directly map the mojo CSPS (ContentSettingPatternSOurce) to native CSPS.

IIUC, changing the native CSPS to store base::value directly would require
defining a new way to serialize/unserialize base::value. 

I would look at the possibility of avoiding the extra copy in the
copy/assignment ctors which seems like the best approach anyways.

[tbansal1, 2017/06/27 10:29:53]

Another straight-forward option is to keep the current patch, but add a DCHECK
somewhere that base::Value is of type int. For the new content setting, I only
need to store a single 64 bit int (a timestamp to be precise). With int's, the
overhead of copies is not going to be prohibitive. WDYT?

   std::string source;
   bool incognito;
 };