fix partially broken patterns, delete totally brocken patterns

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 c7fe7fdcdc13715573fbf2f236926aa08a0cee62..c52baad2b95283a0ba8ce9bf82845a8d52c16255 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider.cc
@@ -30,6 +30,8 @@
 
 namespace {
 
+typedef std::pair<std::string, std::string> StringPair;
+
 // The preference keys where resource identifiers are stored for
 // ContentSettingsType values that support resource identifiers.
 const char* kResourceTypeNames[] = {
@@ -967,39 +969,88 @@ void PrefProvider::MigrateObsoleteContentSettingsPatternPref() {
     const DictionaryValue* all_settings_dictionary =
       prefs_->GetDictionary(prefs::kContentSettingsPatterns);
 
-    DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatternPairs);
-    DictionaryValue* exceptions_dictionary;
-    exceptions_dictionary = update.Get();
-    for (DictionaryValue::key_iterator i(all_settings_dictionary->begin_keys());
-         i != all_settings_dictionary->end_keys();
-         ++i) {
-      const std::string& key(*i);
-      if (key.empty())
-        continue;
+    // A list with invalid keys that will be removed.
+    std::list<std::string> keys_to_remove;
+    std::list<StringPair> keys_to_swap;
 
-      // Validate pattern string and skip it if it is invalid.
-      std::pair<ContentSettingsPattern, ContentSettingsPattern> pattern_pair =
-          ParsePatternString(key);
-      const ContentSettingsPattern& primary_pattern = pattern_pair.first;
-      if (!primary_pattern.IsValid()) {
-        LOG(DFATAL) << "Invalid pattern strings: " << key;
-        continue;
-      }
+    {
+      DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatternPairs);
+      DictionaryValue* exceptions_dictionary;
+      exceptions_dictionary = update.Get();

[Bernhard Bauer, 2011/08/02 11:32:32]

Declare |exceptions_dictionary| in this line?

[markusheintz_, 2011/08/02 12:24:53]

Absolutely.

+      for (DictionaryValue::key_iterator i(
+               all_settings_dictionary->begin_keys());
+           i != all_settings_dictionary->end_keys();
+           ++i) {
+        const std::string& key(*i);
+        // Validate pattern string and skip it if it is invalid.
+        std::pair<ContentSettingsPattern, ContentSettingsPattern> pattern_pair =
+            ParsePatternString(key);
+
+        // Broken patterns will be removed
+        if (!pattern_pair.first.IsValid()) {
+          keys_to_remove.push_back(key);
+          continue;
+        }
 
-      // Copy dictionary value.
-      // Get old settings.
-      DictionaryValue* dictionary = NULL;
-      bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
-          key, &dictionary);
-      DCHECK(found);
+        // Fix key with pattern pairs.
+        // TODO(markusheintz): Delete the "!pattern_pair.second.IsValid()".

[Bernhard Bauer, 2011/08/02 11:32:32]

?

[markusheintz_, 2011/08/02 12:24:53]

Removed

+        if (pattern_pair.first.IsValid() &&
+            (!pattern_pair.second.IsValid() ||
+             !(pattern_pair.second == ContentSettingsPattern::Wildcard()))) {

[Bernhard Bauer, 2011/08/02 11:32:32]

|pattern_pair.second != ContentSettingsPattern::Wildcard()|?

[markusheintz_, 2011/08/02 12:24:53]

ContentSettingsPattern does not have a != operator. If I recall that correctly
you didn't like the != operator. Sorry if my memory is blurry here.

+          // If the dictionary has a non corrupted key that equals the primary
+          // key of a corrupted pattern pair key, don't fix the key but remove
+          // it.
+          if (all_settings_dictionary->HasKey(

[Bernhard Bauer, 2011/08/02 11:32:32]

Can you fit this on one line?

[markusheintz_, 2011/08/02 12:24:53]

 yep. Done.

+              pattern_pair.first.ToString())) {
+            keys_to_remove.push_back(key);
+            continue;
+          }
 
-      // Create new dictionary key.
-      std::string new_pattern_str = CreatePatternString(
-          primary_pattern, ContentSettingsPattern::Wildcard());
+          // If there is more than one key with a pattern pair the has the same
+          // valid primary key, then the value of the last key will overwrite
+          // the valule of previous keys.

[Bernhard Bauer, 2011/08/02 11:32:32]

Nit: "value"

[markusheintz_, 2011/08/02 12:24:53]

Done.

+          keys_to_swap.push_back(std::make_pair(
+              key, pattern_pair.first.ToString()));
+        }
 
-      // Existing values are overwritten.
-      exceptions_dictionary->SetWithoutPathExpansion(
-          new_pattern_str, dictionary->DeepCopy());
+        // Copy dictionary value.
+        DictionaryValue* dictionary = NULL;
+        bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+            key, &dictionary);
+        DCHECK(found);
+        std::string new_key = CreatePatternString(
+            pattern_pair.first, ContentSettingsPattern::Wildcard());
+        // Existing values are overwritten.
+        exceptions_dictionary->SetWithoutPathExpansion(
+            new_key, dictionary->DeepCopy());
+      }
+    }
+
+    {
+      DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatterns);
+      DictionaryValue* mutable_patterns_dictionary = update.Get();
+      // Fix broken pattern strings.
+      for (std::list<StringPair>::iterator i(keys_to_swap.begin());

[Bernhard Bauer, 2011/08/02 11:32:32]

Why can't you do this directly in the loop above?

[markusheintz_, 2011/08/02 12:24:53]

Last time I tried that the iterator didn't like it if I remember correctly.

[Bernhard Bauer, 2011/08/02 12:36:40]

Ah, right.

I think you could still copy the new value to |exceptions_dictionary| (if a
value doesn't already exist for the key), and just keep a set of keys to remove.

[Bernhard Bauer, 2011/08/03 08:33:33]

What about this one?

[markusheintz_, 2011/08/03 11:23:41]

I shouldn't add new keys while I'm iterating over it.

+           i != keys_to_swap.end();
+           ++i) {
+        const StringPair& pattern_str_pair(*i);
+        Value* dict;
+        bool found = mutable_patterns_dictionary->RemoveWithoutPathExpansion(
+                pattern_str_pair.first, &dict);

[Bernhard Bauer, 2011/08/02 11:32:32]

Nit: indent

[markusheintz_, 2011/08/02 12:24:53]

Done.

+        DCHECK(found);
+        mutable_patterns_dictionary->SetWithoutPathExpansion(
+            pattern_str_pair.second, dict);
+      }
+
+      // Remove settings for invalid pattern strings (keys).
+      for (std::list<std::string>::iterator i(keys_to_remove.begin());
+           i != keys_to_remove.end();
+           ++i) {
+        const std::string& key(*i);
+        bool found =
+            mutable_patterns_dictionary->RemoveWithoutPathExpansion(key, NULL);
+        DCHECK(found);
+      }
     }
   }
 }