Migrate Obsolete NotificationsSettings and remove content_settings::NotificationsProvider.

chrome/browser/content_settings/content_settings_pref_provider.cc

Index: chrome/browser/content_settings/content_settings_pref_provider.cc
diff --git a/chrome/browser/content_settings/content_settings_pref_provider.cc b/chrome/browser/content_settings/content_settings_pref_provider.cc
index 4a896b367ac45a964777941e661e1237e7eb6575..bee8a934fd6d9ad8ff4be9dd50bb8479ddb0cfbd 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider.cc
@@ -43,8 +43,8 @@ const char* kResourceTypeNames[] = {
   "per_plugin",
   NULL,
   NULL,
-  NULL,  // Not used for Notifications
-  NULL,  // Not used for Intents.
+  NULL,
+  NULL,
 };
 COMPILE_ASSERT(arraysize(kResourceTypeNames) == CONTENT_SETTINGS_NUM_TYPES,
                resource_type_names_incorrect_size);
@@ -71,9 +71,7 @@ const char* kTypeNames[] = {
   "plugins",
   "popups",
   "geolocation",
-  // TODO(markusheintz): Refactoring in progress. Content settings exceptions
-  // for notifications added next.
-  "notifications",  // Only used for default Notifications settings.
+  "notifications",
   "intents",
 };
 COMPILE_ASSERT(arraysize(kTypeNames) == CONTENT_SETTINGS_NUM_TYPES,
@@ -120,6 +118,25 @@ ContentSetting FixObsoleteCookiePromptMode(ContentSettingsType content_type,
   return setting;
 }
 
+// Clears all settings for the given |type| in the given |pattern_pairs|
+// dictionary.
+void ClearSettings(ContentSettingsType type,
+                   DictionaryValue* pattern_pairs) {
+  std::string type_name(kTypeNames[type]);
+  for (DictionaryValue::key_iterator i = pattern_pairs->begin_keys();
+       i != pattern_pairs->end_keys();
+       ++i) {
+    const std::string& pattern_pair(*i);
+
+    DictionaryValue* settings = NULL;
+    bool found = pattern_pairs->GetDictionaryWithoutPathExpansion(
+          pattern_pair, &settings);

[battre, 2011/08/22 15:00:57]

nit: indentation

[markusheintz_, 2011/08/24 00:47:12]

Done.

+    DCHECK(found);

[battre, 2011/08/22 15:00:57]

I thought this would cause a compiler error on the build bots because |found| is
never used if DCHECK is evaluated to nothing in release mode.

[markusheintz_, 2011/08/24 00:47:12]

You might be right. I removed the DCHECK since it is not really useful anyway.

+
+    settings->RemoveWithoutPathExpansion(type_name, NULL);
+  }
+}
+
 }  // namespace
 
 namespace content_settings {
@@ -368,6 +385,10 @@ void PrefProvider::RegisterUserPrefs(PrefService* prefs) {
                                 PrefService::SYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kContentSettingsPatterns,
                                 PrefService::SYNCABLE_PREF);
+  prefs->RegisterListPref(prefs::kDesktopNotificationAllowedOrigins,
+                          PrefService::SYNCABLE_PREF);

[battre, 2011/08/22 15:00:57]

please update pref_names.* to indicate that these preferences are obsolete and
what they have been replaced with. I would even consider renaming the constant
names.

[markusheintz_, 2011/08/24 00:47:12]

I marked the pref as obsolete in pref_names.h.

Why should I rename the constants?

+  prefs->RegisterListPref(prefs::kDesktopNotificationDeniedOrigins,
+                          PrefService::SYNCABLE_PREF);
   prefs->RegisterListPref(prefs::kPopupWhitelistedHosts,
                           PrefService::UNSYNCABLE_PREF);
   prefs->RegisterDictionaryPref(prefs::kPerHostContentSettings,
@@ -386,6 +407,7 @@ PrefProvider::PrefProvider(PrefService* prefs,
     MigrateObsoletePopupsPref();
     MigrateObsoleteContentSettingsPatternPref();
     MigrateObsoleteGeolocationPref();
+    MigrateObsoleteNotificationsPrefs();
   }
 
   // Verify preferences version.
@@ -410,6 +432,8 @@ PrefProvider::PrefProvider(PrefService* prefs,
   pref_change_registrar_.Add(prefs::kContentSettingsPatterns, this);
   pref_change_registrar_.Add(prefs::kContentSettingsPatternPairs, this);
   pref_change_registrar_.Add(prefs::kGeolocationContentSettings, this);
+  pref_change_registrar_.Add(prefs::kDesktopNotificationAllowedOrigins, this);
+  pref_change_registrar_.Add(prefs::kDesktopNotificationDeniedOrigins, this);
 }
 
 ContentSetting PrefProvider::GetContentSetting(
@@ -508,6 +532,7 @@ void PrefProvider::SetContentSetting(
                content_type,
                resource_identifier,
                setting);
+    prefs_->ScheduleSavePersistentPrefs();
   }
 
   NotifyObservers(
@@ -542,7 +567,7 @@ void PrefProvider::ClearAllContentSettingsRules(
       }
     }
   }
-
+  prefs_->ScheduleSavePersistentPrefs();
   NotifyObservers(ContentSettingsPattern(),
                   ContentSettingsPattern(),
                   content_type,
@@ -564,15 +589,19 @@ void PrefProvider::Observe(
     std::string* name = Details<std::string>(details).ptr();
     if (*name == prefs::kContentSettingsPatternPairs) {
       SyncObsoletePatternPref();
-      SyncObsoleteGeolocationPref();
+      SyncObsoletePrefs();
     } else if (*name == prefs::kContentSettingsPatterns) {
       MigrateObsoleteContentSettingsPatternPref();
     } else if (*name == prefs::kGeolocationContentSettings) {
       MigrateObsoleteGeolocationPref();
+    } else if (*name == prefs::kDesktopNotificationAllowedOrigins ||
+               *name == prefs::kDesktopNotificationDeniedOrigins) {
+      MigrateObsoleteNotificationsPrefs();
     } else {
       NOTREACHED() << "Unexpected preference observed";
       return;
     }
+    prefs_->ScheduleSavePersistentPrefs();
     ReadContentSettingsFromPref(true);
 
     NotifyObservers(ContentSettingsPattern(),
@@ -598,11 +627,17 @@ void PrefProvider::UpdatePref(
     const ResourceIdentifier& resource_identifier,
     ContentSetting setting) {
   AutoReset<bool> auto_reset(&updating_preferences_, true);
-  UpdatePatternPairsPref(primary_pattern,
-                         secondary_pattern,
-                         content_type,
-                         resource_identifier,
-                         setting);
+  {
+    DictionaryPrefUpdate update(prefs_,
+                                prefs::kContentSettingsPatternPairs);
+    DictionaryValue* pattern_pairs_settings = update.Get();
+    UpdatePatternPairsSettings(primary_pattern,
+                               secondary_pattern,
+                               content_type,
+                               resource_identifier,
+                               setting,
+                               pattern_pairs_settings);
+  }
   if (content_type != CONTENT_SETTINGS_TYPE_GEOLOCATION &&
       content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
     UpdateObsoletePatternsPref(primary_pattern,
@@ -610,9 +645,18 @@ void PrefProvider::UpdatePref(
                                content_type,
                                resource_identifier,
                                setting);
-  }
-  if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
+  } else if (content_type == CONTENT_SETTINGS_TYPE_GEOLOCATION) {
     UpdateObsoleteGeolocationPref(primary_pattern, secondary_pattern, setting);
+  } else if (content_type == CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
+    ListPrefUpdate update_allowed_sites(
+        prefs_, prefs::kDesktopNotificationAllowedOrigins);
+    ListPrefUpdate update_denied_sites(
+        prefs_, prefs::kDesktopNotificationDeniedOrigins);
+    UpdateObsoleteNotificationsSettings(primary_pattern,
+                                        secondary_pattern,
+                                        setting,
+                                        update_allowed_sites.Get(),
+                                        update_denied_sites.Get());
   }
 }
 
@@ -774,26 +818,23 @@ void PrefProvider::UpdateObsoletePatternsPref(
   }
 }
 
-void PrefProvider::UpdatePatternPairsPref(
+void PrefProvider::UpdatePatternPairsSettings(
       const ContentSettingsPattern& primary_pattern,
       const ContentSettingsPattern& secondary_pattern,
       ContentSettingsType content_type,
       const ResourceIdentifier& resource_identifier,
-      ContentSetting setting) {
-  DictionaryPrefUpdate update(prefs_,
-                              prefs::kContentSettingsPatternPairs);
-  DictionaryValue* all_settings_dictionary = update.Get();
-
+      ContentSetting setting,
+      DictionaryValue* pattern_pairs_settings) {
   // Get settings dictionary for the given patterns.
   std::string pattern_str(CreatePatternString(primary_pattern,
                                               secondary_pattern));
   DictionaryValue* settings_dictionary = NULL;
-  bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+  bool found = pattern_pairs_settings->GetDictionaryWithoutPathExpansion(
       pattern_str, &settings_dictionary);
 
   if (!found && (setting != CONTENT_SETTING_DEFAULT)) {
     settings_dictionary = new DictionaryValue;
-    all_settings_dictionary->SetWithoutPathExpansion(
+    pattern_pairs_settings->SetWithoutPathExpansion(
         pattern_str, settings_dictionary);
   }
 
@@ -835,7 +876,7 @@ void PrefProvider::UpdatePatternPairsPref(
     }
     // Remove the settings dictionary if it is empty.
     if (settings_dictionary->empty()) {
-      all_settings_dictionary->RemoveWithoutPathExpansion(
+      pattern_pairs_settings->RemoveWithoutPathExpansion(
           pattern_str, NULL);
     }
   }
@@ -878,6 +919,31 @@ void PrefProvider::UpdateObsoleteGeolocationPref(
   }
 }
 
+void PrefProvider::UpdateObsoleteNotificationsSettings(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSetting setting,
+    ListValue* allowed_sites,
+    ListValue* denied_sites) {
+  DCHECK_EQ(secondary_pattern, ContentSettingsPattern::Wildcard());
+  GURL origin(primary_pattern.ToString());
+  DCHECK(origin.is_valid());
+  StringValue* value = new StringValue(origin.spec());
+  if (setting == CONTENT_SETTING_ALLOW) {
+    denied_sites->Remove(*value, NULL);
+    allowed_sites->AppendIfNotPresent(value);
+  } else if (setting == CONTENT_SETTING_BLOCK) {
+    allowed_sites->Remove(*value, NULL);
+    denied_sites->AppendIfNotPresent(value);
+  } else if (setting == CONTENT_SETTING_DEFAULT) {
+    denied_sites->Remove(*value, NULL);
+    allowed_sites->Remove(*value, NULL);

[battre, 2011/08/22 15:00:57]

here you leak |value|

[markusheintz_, 2011/08/24 00:47:12]

Changed value to a scoped_ptr<StringValue>

+  } else {
+    NOTREACHED() << "Setting value: " << setting
+                 << " is not supported for notifications";
+  }
+}
+
 // static
 void PrefProvider::CanonicalizeContentSettingsExceptions(
     DictionaryValue* all_settings_dictionary) {
@@ -1155,6 +1221,10 @@ void PrefProvider::MigrateObsoleteGeolocationPref() {
   if (!prefs_->HasPrefPath(prefs::kGeolocationContentSettings))
     return;
 
+  DictionaryPrefUpdate update(prefs_,
+                              prefs::kContentSettingsPatternPairs);
+  DictionaryValue* pattern_pairs_settings = update.Get();
+
   const DictionaryValue* geolocation_settings =
       prefs_->GetDictionary(prefs::kGeolocationContentSettings);
   for (DictionaryValue::key_iterator i =
@@ -1189,22 +1259,78 @@ void PrefProvider::MigrateObsoleteGeolocationPref() {
           ContentSettingsPattern::FromURLNoWildcard(secondary_url);
       DCHECK(primary_pattern.IsValid() && secondary_pattern.IsValid());
 
-      UpdatePatternPairsPref(primary_pattern,
-                             secondary_pattern,
-                             CONTENT_SETTINGS_TYPE_GEOLOCATION,
-                             std::string(),
-                             IntToContentSetting(setting_value));
+      UpdatePatternPairsSettings(primary_pattern,
+                                 secondary_pattern,
+                                 CONTENT_SETTINGS_TYPE_GEOLOCATION,
+                                 std::string(),
+                                 IntToContentSetting(setting_value),
+                                 pattern_pairs_settings);
     }
   }
 }
 
-void PrefProvider::SyncObsoleteGeolocationPref() {
+void PrefProvider::MigrateObsoleteNotificationsPrefs() {
+  // The notifications settings in the preferences
+  // prefs::kContentSettingsPatternPairs do not contain the latest
+  // notifications settings. So all notification settings are cleared and
+  // migrated from the obsolete preferences for notifications settings that
+  // contain the lattest settings.

[battre, 2011/08/22 15:00:57]

nit: typo "latest"

[markusheintz_, 2011/08/24 00:47:12]

Done.

+  DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatternPairs);
+  DictionaryValue* pattern_pairs_settings = update.Get();
+  ClearSettings(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, pattern_pairs_settings);
+
+  const ListValue* allow_sites =
+      prefs_->GetList(prefs::kDesktopNotificationAllowedOrigins);
+  for (size_t i = 0; i < allow_sites->GetSize(); ++i) {
+    std::string url_string;
+    bool status = allow_sites->GetString(i, &url_string);
+    DCHECK(status);
+    ContentSettingsPattern primary_pattern =
+        ContentSettingsPattern::FromURLNoWildcard(GURL(url_string));
+    DCHECK(primary_pattern.IsValid());
+    UpdatePatternPairsSettings(primary_pattern,
+                               ContentSettingsPattern::Wildcard(),
+                               CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                               std::string(),
+                               CONTENT_SETTING_ALLOW,
+                               pattern_pairs_settings);
+  }
+
+  const ListValue* denied_sites =
+      prefs_->GetList(prefs::kDesktopNotificationDeniedOrigins);
+  for (size_t i = 0; i < denied_sites->GetSize(); ++i) {
+    std::string url_string;
+    bool status = denied_sites->GetString(i, &url_string);
+    DCHECK(status);
+    ContentSettingsPattern primary_pattern =
+        ContentSettingsPattern::FromURLNoWildcard(GURL(url_string));
+    DCHECK(primary_pattern.IsValid());
+    UpdatePatternPairsSettings(primary_pattern,
+                               ContentSettingsPattern::Wildcard(),
+                               CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                               std::string(),
+                               CONTENT_SETTING_BLOCK,
+                               pattern_pairs_settings);
+  }
+}
+
+void PrefProvider::SyncObsoletePrefs() {
   DCHECK(prefs_);
   DCHECK(prefs_->HasPrefPath(prefs::kContentSettingsPatternPairs));
 
-  // Clear the obsolete preference for geolocation settings. Then copy all
-  // geolocation settings from the new preference to the obsolete one.
+  // Clear obsolete preferences first. Then copy the settings from the new
+  // preference to the obsolete ones.
   prefs_->ClearPref(prefs::kGeolocationContentSettings);
+  prefs_->ClearPref(prefs::kDesktopNotificationAllowedOrigins);
+  prefs_->ClearPref(prefs::kDesktopNotificationDeniedOrigins);
+
+  ListPrefUpdate update_allowed_sites(
+        prefs_, prefs::kDesktopNotificationAllowedOrigins);

[battre, 2011/08/22 15:00:57]

nit: indentation -2

[markusheintz_, 2011/08/24 00:47:12]

Done.

+  ListPrefUpdate update_denied_sites(
+        prefs_, prefs::kDesktopNotificationDeniedOrigins);
+  ListValue* allowed_sites = update_allowed_sites.Get();
+  ListValue* denied_sites = update_denied_sites.Get();
+
   const DictionaryValue* pattern_pairs_dictionary =
       prefs_->GetDictionary(prefs::kContentSettingsPatternPairs);
   for (DictionaryValue::key_iterator i =
@@ -1222,11 +1348,22 @@ void PrefProvider::SyncObsoleteGeolocationPref() {
     DCHECK(found);
 
     if (settings_dictionary->HasKey(
+            kTypeNames[CONTENT_SETTINGS_TYPE_NOTIFICATIONS])) {
+      int setting_value;

[battre, 2011/08/22 15:00:57]

nit: initialize

[markusheintz_, 2011/08/24 00:47:12]

Done.

+      settings_dictionary->GetInteger(
+          kTypeNames[CONTENT_SETTINGS_TYPE_NOTIFICATIONS], &setting_value);

[battre, 2011/08/22 15:00:57]

Return value is not checked. I propose to simplify this, by replacing the HasKey
call above with the following.

int setting_value = 0;
if (settings_dictionary->GetInteger(..., &setting_value)) {
  UpdateObsolete...
}

(no previous check whether the key exists)

[markusheintz_, 2011/08/24 00:47:12]

Done.

+      UpdateObsoleteNotificationsSettings(pattern_pair.first,
+                                          pattern_pair.second,
+                                          ContentSetting(setting_value),
+                                          allowed_sites,
+                                          denied_sites);
+    }
+
+    if (settings_dictionary->HasKey(
             kTypeNames[CONTENT_SETTINGS_TYPE_GEOLOCATION])) {

[battre, 2011/08/22 15:00:57]

same here

[markusheintz_, 2011/08/24 00:47:12]

Done.

       int setting_value;
       settings_dictionary->GetInteger(
           kTypeNames[CONTENT_SETTINGS_TYPE_GEOLOCATION], &setting_value);
-
       UpdateObsoleteGeolocationPref(pattern_pair.first,
                                     pattern_pair.second,
                                     ContentSetting(setting_value));