Write last_modified date to Content Settings in the PrefProvider

chrome/browser/content_settings/content_settings_pref_provider_unittest.cc

Index: chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
index a59763fa7bc83107b88ab7ddd24bd217f7ed07a7..cea2c0e10726b6b77accef9a3db0e6e040362e48 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
@@ -114,7 +114,7 @@ class PrefProviderTest : public testing::Test {
 
 TEST_F(PrefProviderTest, Observer) {
   TestingProfile profile;
-  PrefProvider pref_content_settings_provider(profile.GetPrefs(), false);
+  PrefProvider pref_content_settings_provider(profile.GetPrefs(), false, true);

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

nit (here and below): please document these boolean parameters, e.g.  
, true /* param_name */);

If you could document the existing param it would be great as well!

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

Done.

 
   ContentSettingsPattern pattern =
       ContentSettingsPattern::FromString("[*.]example.com");
@@ -162,7 +162,7 @@ TEST_F(PrefProviderTest, DiscardObsoleteFullscreenAndMouselockPreferences) {
 
   // Instantiate a new PrefProvider here, because we want to test the
   // constructor's behavior after setting the above.
-  PrefProvider provider(prefs, false);
+  PrefProvider provider(prefs, false, true);
   provider.ShutdownOnUIThread();
 
   // Check that fullscreen and mouselock have been deleted.
@@ -220,7 +220,7 @@ TEST_F(PrefProviderTest, DiscardObsoleteLastUsagePreferences) {
 
   // Instantiate a new PrefProvider here, because we want to test the
   // constructor's behavior after setting the above.
-  PrefProvider provider(prefs, false);
+  PrefProvider provider(prefs, false, true);
 
   // Check that last_used data has been deleted.
   EXPECT_TRUE(prefs->GetDictionary(kGeolocationPrefPath)->empty());
@@ -273,8 +273,8 @@ TEST_F(PrefProviderTest, Incognito) {
   otr_profile_builder.SetPrefService(base::WrapUnique(otr_prefs));
   otr_profile_builder.BuildIncognito(profile.get());
 
-  PrefProvider pref_content_settings_provider(regular_prefs, false);
-  PrefProvider pref_content_settings_provider_incognito(otr_prefs, true);
+  PrefProvider pref_content_settings_provider(regular_prefs, false, true);
+  PrefProvider pref_content_settings_provider_incognito(otr_prefs, true, true);
   ContentSettingsPattern pattern =
       ContentSettingsPattern::FromString("[*.]example.com");
   pref_content_settings_provider.SetWebsiteSetting(
@@ -303,7 +303,7 @@ TEST_F(PrefProviderTest, Incognito) {
 
 TEST_F(PrefProviderTest, GetContentSettingsValue) {
   TestingProfile testing_profile;
-  PrefProvider provider(testing_profile.GetPrefs(), false);
+  PrefProvider provider(testing_profile.GetPrefs(), false, true);
 
   GURL primary_url("http://example.com/");
   ContentSettingsPattern primary_pattern =
@@ -343,8 +343,8 @@ TEST_F(PrefProviderTest, GetContentSettingsValue) {
 
 TEST_F(PrefProviderTest, Patterns) {
   TestingProfile testing_profile;
-  PrefProvider pref_content_settings_provider(testing_profile.GetPrefs(),
-                                              false);
+  PrefProvider pref_content_settings_provider(testing_profile.GetPrefs(), false,
+                                              true);
 
   GURL host1("http://example.com/");
   GURL host2("http://www.example.com/");
@@ -403,8 +403,8 @@ TEST_F(PrefProviderTest, Patterns) {
 #if BUILDFLAG(ENABLE_PLUGINS)
 TEST_F(PrefProviderTest, ResourceIdentifier) {
   TestingProfile testing_profile;
-  PrefProvider pref_content_settings_provider(testing_profile.GetPrefs(),
-                                              false);
+  PrefProvider pref_content_settings_provider(testing_profile.GetPrefs(), false,
+                                              true);
 
   GURL host("http://example.com/");
   ContentSettingsPattern pattern =
@@ -444,7 +444,7 @@ TEST_F(PrefProviderTest, Deadlock) {
 
   const WebsiteSettingsInfo* info = WebsiteSettingsRegistry::GetInstance()->Get(
       CONTENT_SETTINGS_TYPE_COOKIES);
-  PrefProvider provider(&prefs, false);
+  PrefProvider provider(&prefs, false, true);
   DeadlockCheckerObserver observer(&prefs, &provider);
   {
     DictionaryPrefUpdate update(&prefs, info->pref_name());
@@ -470,7 +470,7 @@ TEST_F(PrefProviderTest, IncognitoInheritsValueMap) {
   std::unique_ptr<base::Value> value(new base::Value(CONTENT_SETTING_ALLOW));
 
   // Create a normal provider and set a setting.
-  PrefProvider normal_provider(&prefs, false);
+  PrefProvider normal_provider(&prefs, false, true);
   normal_provider.SetWebsiteSetting(pattern_1, wildcard,
                                     CONTENT_SETTINGS_TYPE_COOKIES,
                                     std::string(), value->DeepCopy());
@@ -492,7 +492,7 @@ TEST_F(PrefProviderTest, IncognitoInheritsValueMap) {
   }
 
   // Create an incognito provider and set a setting.
-  PrefProvider incognito_provider(&prefs, true);
+  PrefProvider incognito_provider(&prefs, true, true);
   incognito_provider.SetWebsiteSetting(pattern_2, wildcard,
                                        CONTENT_SETTINGS_TYPE_COOKIES,
                                        std::string(), value->DeepCopy());
@@ -530,7 +530,7 @@ TEST_F(PrefProviderTest, ClearAllContentSettingsRules) {
   std::unique_ptr<base::Value> value(new base::Value(CONTENT_SETTING_ALLOW));
   ResourceIdentifier res_id("abcde");
 
-  PrefProvider provider(&prefs, false);
+  PrefProvider provider(&prefs, false, true);
 
   // Non-empty pattern, syncable, empty resource identifier.
   provider.SetWebsiteSetting(pattern, wildcard,
@@ -596,4 +596,63 @@ TEST_F(PrefProviderTest, ClearAllContentSettingsRules) {
   provider.ShutdownOnUIThread();
 }
 
+TEST_F(PrefProviderTest, DeleteWebsiteSetting) {
+  sync_preferences::TestingPrefServiceSyncable prefs;
+  PrefProvider::RegisterProfilePrefs(prefs.registry());
+
+  ContentSettingsPattern pattern_1 =
+      ContentSettingsPattern::FromString("google.com");
+  ContentSettingsPattern pattern_2 =
+      ContentSettingsPattern::FromString("www.google.com");
+  std::unique_ptr<base::Value> value(new base::Value(CONTENT_SETTING_ALLOW));
+
+  // Create a  provider and set a setting.
+  PrefProvider provider(&prefs, false, /*store_last_modified=*/true);

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

The usual way to do this is:
true /* store_last_modified */);

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

I changed it but do you know if this style is documented somewhere? 
The chromium style guide doesn't mention parameter comments all and the google
style guide suggests the /*comment=*/ style. 

https://google.github.io/styleguide/cppguide.html#Implementation_Comments -->
Function Argument Comments

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

Ah I actually just found out today that the style you had is now considered
preferable. The reason is because clang will check it. I'm sorry about that, my
experience on Chrome has always been to use the style I suggested.

Don't feel you need to change this now, it's already a style that's used in many
places.

+  provider.SetWebsiteSetting(pattern_1, ContentSettingsPattern::Wildcard(),
+                             CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
+                             value->DeepCopy());
+  provider.SetWebsiteSetting(pattern_2, ContentSettingsPattern::Wildcard(),
+                             CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
+                             value->DeepCopy());
+  base::Time t = base::Time::Now();
+  // These clear calls shouldn't do anything because both rules were created
+  // before |t|.
+  provider.DeleteWebsiteSetting(pattern_1, ContentSettingsPattern::Wildcard(),
+                                CONTENT_SETTINGS_TYPE_COOKIES, t);
+  provider.DeleteWebsiteSetting(pattern_2, ContentSettingsPattern::Wildcard(),
+                                CONTENT_SETTINGS_TYPE_COOKIES, t);
+  {
+    std::unique_ptr<RuleIterator> it(provider.GetRuleIterator(
+        CONTENT_SETTINGS_TYPE_COOKIES, std::string(), false));
+    EXPECT_TRUE(it->HasNext());
+    Rule rule = it->Next();
+    EXPECT_EQ(pattern_1, rule.primary_pattern);
+    EXPECT_TRUE(it->HasNext());
+    rule = it->Next();
+    EXPECT_EQ(pattern_2, rule.primary_pattern);
+    EXPECT_FALSE(it->HasNext());
+  }
+
+  // Update pattern_1 and check that the timestamp changes.
+  std::unique_ptr<base::Value> value2(new base::Value(CONTENT_SETTING_BLOCK));
+  provider.SetWebsiteSetting(pattern_1, ContentSettingsPattern::Wildcard(),
+                             CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
+                             value2->DeepCopy());
+  // This time pattern_1 should be deleted because it was edited after |t|.
+  provider.DeleteWebsiteSetting(pattern_1, ContentSettingsPattern::Wildcard(),
+                                CONTENT_SETTINGS_TYPE_COOKIES, t);
+  provider.DeleteWebsiteSetting(pattern_2, ContentSettingsPattern::Wildcard(),
+                                CONTENT_SETTINGS_TYPE_COOKIES, t);
+  {
+    std::unique_ptr<RuleIterator> it(provider.GetRuleIterator(
+        CONTENT_SETTINGS_TYPE_COOKIES, std::string(), false));
+    EXPECT_TRUE(it->HasNext());
+    Rule rule = it->Next();
+    EXPECT_EQ(pattern_2, rule.primary_pattern);
+    EXPECT_FALSE(it->HasNext());
+  }
+
+  provider.ShutdownOnUIThread();
+}
+
 }  // namespace content_settings