Write last_modified date to Content Settings in the PrefProvider

components/content_settings/core/browser/content_settings_origin_identifier_value_map.h

Index: components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
diff --git a/components/content_settings/core/browser/content_settings_origin_identifier_value_map.h b/components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
index 1ac2fe56bed118f216876804e563e97c8d9a14a4..0ba4c20912ce443709a578c6e0898c389f9cd162 100644
--- a/components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
+++ b/components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
@@ -13,6 +13,7 @@
 
 #include "base/macros.h"
 #include "base/memory/linked_ptr.h"
+#include "base/time/time.h"
 #include "components/content_settings/core/common/content_settings.h"
 
 class GURL;
@@ -44,7 +45,14 @@ class OriginIdentifierValueMap {
     bool operator<(const OriginIdentifierValueMap::PatternPair& other) const;
   };
 
-  typedef std::map<PatternPair, linked_ptr<base::Value> > Rules;
+  struct ValueEntry {
+    base::Time last_modified;
+    linked_ptr<base::Value> value;
+    ValueEntry();
+    ~ValueEntry();
+  };
+
+  typedef std::map<PatternPair, ValueEntry> Rules;
   typedef std::map<EntryMapKey, Rules> EntryMap;
 
   EntryMap::iterator begin() {
@@ -92,15 +100,21 @@ class OriginIdentifierValueMap {
       ContentSettingsType content_type,
       const ResourceIdentifier& resource_identifier) const;
 
-  // Sets the |value| for the given |primary_pattern|, |secondary_pattern|,
-  // |content_type|, |resource_identifier| tuple. The method takes the ownership
-  // of the passed |value|.
-  void SetValue(
+  base::Time GetLastModified(
       const ContentSettingsPattern& primary_pattern,
       const ContentSettingsPattern& secondary_pattern,
       ContentSettingsType content_type,
-      const ResourceIdentifier& resource_identifier,
-      base::Value* value);
+      const ResourceIdentifier& resource_identifier) const;

[raymes, 2017/04/24 03:20:22]

Cool, that makes more sense to me.

I was thinking that we might be able to get away with not modifying
OriginIdentifierValueMap since some callsites don't use the last modified
functionality and instead write a wrapper that stores the time:

class OriginIdentifierValueMapWithLastModified { // naming is hard!
  std::unique_ptr<RuleIterator> GetRuleIterator(...);
  void SetValue(...);
  base::Time GetLastModified(...);
  size_t size();
  void Clear();  // Would clear both underlying maps

  std::map<PatternPair, base::Time> times_;
  OriginIdentifierValueMap map_;
};

It would be cleaner, but if you feel that the number of callsites is low (I
think it's ~5-6) and it's not worth it, I will leave it up to you and Martin to
decide.

[dullweber, 2017/04/25 10:50:44]

that sounds like a good idea and I tried to implement it but I noticed that I
have to duplicate basically all the code from set and get methods because I have
to create the keys, check if the map is empty...
The number of callsites that are passing base::Time() is not very high, so I
would suggest to leave it this way or add another method that passes
base::Time() by default.

[raymes, 2017/04/26 01:54:17]

Ah that makes sense. Thanks for looking at it. Perhaps adding another function
is slightly preferable, but I don't feel particularly strongly so I'll leave it
up to you.

+
+  // Sets the |value| for the given |primary_pattern|, |secondary_pattern|,
+  // |content_type|, |resource_identifier| tuple. The method takes the ownership
+  // of the passed |value|.
+  void SetValue(const ContentSettingsPattern& primary_pattern,
+                const ContentSettingsPattern& secondary_pattern,
+                ContentSettingsType content_type,
+                const ResourceIdentifier& resource_identifier,
+                base::Time last_modified,

[raymes, 2017/04/24 03:20:22]

const base::Time&

[dullweber, 2017/04/25 10:50:44]

I thought that base::Time and other very small classes like StringPiece can be
passed by value because that is as efficient or even more efficient than passing
them by reference? base::Time is just an int64_t. The value can be passed in a
register while a reference needs to dereferenced from memory.

The current code seems to have about 300 functions with pass by value and 257
with pass by const ref for base::Time:
https://cs.chromium.org/search/?q=%22(base::time+%22&type=cs
https://cs.chromium.org/search/?q=%22,+base::time+%22&type=cs

https://cs.chromium.org/search/?q=%22(const+base::time%26+%22&type=cs
https://cs.chromium.org/search/?q=%22,+const+base::time%26+%22&type=cs

base::TimeDelta is passed more often by value than reference (723 vs 264):
https://cs.chromium.org/search/?q=%22(base::timedelta+%22&type=cs
https://cs.chromium.org/search/?q=%22,+base::timedelta+%22&type=cs

https://cs.chromium.org/search/?q=%22(const+base::timedelta%26+%22&type=cs
https://cs.chromium.org/search/?q=%22,+const+base::timedelta%26+%22&type=cs

[raymes, 2017/04/26 01:54:17]

Thanks for looking at it. I think it's fine to pass by value in that case. In my
mind I just default to suggesting const& for all user-defined data types because
they are almost always bigger than a reference. But in this case it probably
doesn't matter.

+                base::Value* value);
 
   // Deletes the map entry for the given |primary_pattern|,
   // |secondary_pattern|, |content_type|, |resource_identifier| tuple.