Many changes to DictionaryValues:

* Add support for keys with "." in them via new XXXWithoutPathExpansion() APIs.
* Use these APIs with all key iterator usage.
* SetXXX() calls cannot fail, so change them from bool to void.
* Change GetSize() to size() since it's cheap, and add empty().

Other:
* Use standard for loop format in more places (e.g. instead of while loops when they're really doing a for loop).
* Shorten a few bits of code.

BUG=567
TEST=none
Review URL: http://codereview.chromium.org/441008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@33109 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 59 insertions, 85 deletions

diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 9b69756..94d6de2 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -480,12 +480,10 @@ bool Extension::ContainsNonThemeKeys(const DictionaryValue& source) {
   // Go through all the root level keys and verify that they're in the map
   // of keys allowable by themes. If they're not, then make a not of it for
   // later.
-  DictionaryValue::key_iterator iter = source.begin_keys();
-  while (iter != source.end_keys()) {
-    std::wstring key = (*iter);
-    if (theme_keys.find(key) == theme_keys.end())
+  for (DictionaryValue::key_iterator iter = source.begin_keys();
+       iter != source.end_keys(); ++iter) {
+    if (theme_keys.find(*iter) == theme_keys.end())
       return true;
-    ++iter;
   }
   return false;
 }
@@ -770,14 +768,13 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     DictionaryValue* images_value;
     if (theme_value->GetDictionary(keys::kThemeImages, &images_value)) {
       // Validate that the images are all strings
-      DictionaryValue::key_iterator iter = images_value->begin_keys();
-      while (iter != images_value->end_keys()) {
+      for (DictionaryValue::key_iterator iter = images_value->begin_keys();
+           iter != images_value->end_keys(); ++iter) {
         std::string val;
         if (!images_value->GetString(*iter, &val)) {
           *error = errors::kInvalidThemeImages;
           return false;
         }
-        ++iter;
       }
       theme_images_.reset(
           static_cast<DictionaryValue*>(images_value->DeepCopy()));
@@ -785,35 +782,28 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
 
     DictionaryValue* colors_value;
     if (theme_value->GetDictionary(keys::kThemeColors, &colors_value)) {
-      // Validate that the colors are all three-item lists
-      DictionaryValue::key_iterator iter = colors_value->begin_keys();
-      while (iter != colors_value->end_keys()) {
-        std::string val;
-        int color = 0;
+      // Validate that the colors are RGB or RGBA lists
+      for (DictionaryValue::key_iterator iter = colors_value->begin_keys();
+           iter != colors_value->end_keys(); ++iter) {
         ListValue* color_list;
-        if (colors_value->GetList(*iter, &color_list)) {
-          if (color_list->GetSize() == 3 ||
-              color_list->GetSize() == 4) {
-            if (color_list->GetInteger(0, &color) &&
-                color_list->GetInteger(1, &color) &&
-                color_list->GetInteger(2, &color)) {
-              if (color_list->GetSize() == 4) {
-                double alpha;
-                int alpha_int;
-                if (color_list->GetReal(3, &alpha) ||
-                    color_list->GetInteger(3, &alpha_int)) {
-                  ++iter;
-                  continue;
-                }
-              } else {
-                ++iter;
-                continue;
-              }
-            }
-          }
+        double alpha;
+        int alpha_int;
+        int color;
+        // The color must be a list
+        if (!colors_value->GetListWithoutPathExpansion(*iter, &color_list) ||
+            // And either 3 items (RGB) or 4 (RGBA)
+            ((color_list->GetSize() != 3) &&
+             ((color_list->GetSize() != 4) ||
+              // For RGBA, the fourth item must be a real or int alpha value
+              (!color_list->GetReal(3, &alpha) &&
+               !color_list->GetInteger(3, &alpha_int)))) ||
+            // For both RGB and RGBA, the first three items must be ints (R,G,B)
+            !color_list->GetInteger(0, &color) ||
+            !color_list->GetInteger(1, &color) ||
+            !color_list->GetInteger(2, &color)) {
+          *error = errors::kInvalidThemeColors;
+          return false;
         }
-        *error = errors::kInvalidThemeColors;
-        return false;
       }
       theme_colors_.reset(
           static_cast<DictionaryValue*>(colors_value->DeepCopy()));
@@ -822,12 +812,12 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     DictionaryValue* tints_value;
     if (theme_value->GetDictionary(keys::kThemeTints, &tints_value)) {
       // Validate that the tints are all reals.
-      DictionaryValue::key_iterator iter = tints_value->begin_keys();
-      while (iter != tints_value->end_keys()) {
+      for (DictionaryValue::key_iterator iter = tints_value->begin_keys();
+           iter != tints_value->end_keys(); ++iter) {
         ListValue* tint_list;
-        double v = 0;
-        int vi = 0;
-        if (!tints_value->GetList(*iter, &tint_list) ||
+        double v;
+        int vi;
+        if (!tints_value->GetListWithoutPathExpansion(*iter, &tint_list) ||
             tint_list->GetSize() != 3 ||
             !(tint_list->GetReal(0, &v) || tint_list->GetInteger(0, &vi)) ||
             !(tint_list->GetReal(1, &v) || tint_list->GetInteger(1, &vi)) ||
@@ -835,7 +825,6 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
           *error = errors::kInvalidThemeTints;
           return false;
         }
-        ++iter;
       }
       theme_tints_.reset(
           static_cast<DictionaryValue*>(tints_value->DeepCopy()));
@@ -1132,24 +1121,20 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     }
 
     // Validate that the overrides are all strings
-    DictionaryValue::key_iterator iter = overrides->begin_keys();
-    while (iter != overrides->end_keys()) {
+    for (DictionaryValue::key_iterator iter = overrides->begin_keys();
+         iter != overrides->end_keys(); ++iter) {
       std::string page = WideToUTF8(*iter);
       // For now, only allow the new tab page.  Others will work when we remove
       // this check, but let's keep it simple for now.
       // TODO(erikkay) enable other pages as well
-      if (page != chrome::kChromeUINewTabHost) {
-        *error = errors::kInvalidChromeURLOverrides;
-        return false;
-      }
       std::string val;
-      if (!overrides->GetString(*iter, &val)) {
+      if ((page != chrome::kChromeUINewTabHost) ||
+          !overrides->GetStringWithoutPathExpansion(*iter, &val)) {
         *error = errors::kInvalidChromeURLOverrides;
         return false;
       }
       // Replace the entry with a fully qualified chrome-extension:// URL.
       chrome_url_overrides_[page] = GetResourceURL(val);
-      ++iter;
     }
   }
 
@@ -1178,9 +1163,8 @@ std::set<FilePath> Extension::GetBrowserImages() {
     for (DictionaryValue::key_iterator it = theme_images->begin_keys();
          it != theme_images->end_keys(); ++it) {
       std::wstring val;
-      if (theme_images->GetString(*it, &val)) {
+      if (theme_images->GetStringWithoutPathExpansion(*it, &val))
         image_paths.insert(FilePath::FromWStringHack(val));
-      }
     }
   }
 
diff --git a/chrome/common/extensions/extension_message_bundle.cc b/chrome/common/extensions/extension_message_bundle.cc
index 9e3a7df..a076d44 100644
--- a/chrome/common/extensions/extension_message_bundle.cc
+++ b/chrome/common/extensions/extension_message_bundle.cc
@@ -50,11 +50,11 @@ bool ExtensionMessageBundle::Init(const CatalogVector& locale_catalogs,
                                   std::string* error) {
   dictionary_.clear();
 
-  CatalogVector::const_reverse_iterator it = locale_catalogs.rbegin();
-  for (; it != locale_catalogs.rend(); ++it) {
+  for (CatalogVector::const_reverse_iterator it = locale_catalogs.rbegin();
+       it != locale_catalogs.rend(); ++it) {
     DictionaryValue* catalog = (*it).get();
-    DictionaryValue::key_iterator key_it = catalog->begin_keys();
-    for (; key_it != catalog->end_keys(); ++key_it) {
+    for (DictionaryValue::key_iterator key_it = catalog->begin_keys();
+         key_it != catalog->end_keys(); ++key_it) {
       std::string key(StringToLowerASCII(WideToUTF8(*key_it)));
       if (!IsValidName(*key_it))
         return BadKeyMessage(key, error);
@@ -76,7 +76,7 @@ bool ExtensionMessageBundle::GetMessageValue(const std::wstring& wkey,
   std::string key(WideToUTF8(wkey));
   // Get the top level tree for given key (name part).
   DictionaryValue* name_tree;
-  if (!catalog.GetDictionary(wkey, &name_tree)) {
+  if (!catalog.GetDictionaryWithoutPathExpansion(wkey, &name_tree)) {
     *error = StringPrintf("Not a valid tree for key %s.", key.c_str());
     return false;
   }
@@ -117,13 +117,13 @@ bool ExtensionMessageBundle::GetPlaceholders(const DictionaryValue& name_tree,
   }
 
   for (DictionaryValue::key_iterator key_it = placeholders_tree->begin_keys();
-       key_it != placeholders_tree->end_keys();
-       ++key_it) {
+       key_it != placeholders_tree->end_keys(); ++key_it) {
     DictionaryValue* placeholder;
     std::string content_key = WideToUTF8(*key_it);
     if (!IsValidName(*key_it))
       return BadKeyMessage(content_key, error);
-    if (!placeholders_tree->GetDictionary(*key_it, &placeholder)) {
+    if (!placeholders_tree->GetDictionaryWithoutPathExpansion(*key_it,
+                                                              &placeholder)) {
       *error = StringPrintf("Invalid placeholder %s for key %s",
                             content_key.c_str(),
                             name_key.c_str());
diff --git a/chrome/common/extensions/extension_unpacker_unittest.cc b/chrome/common/extensions/extension_unpacker_unittest.cc
index fe3272e..747827d 100644
--- a/chrome/common/extensions/extension_unpacker_unittest.cc
+++ b/chrome/common/extensions/extension_unpacker_unittest.cc
@@ -107,12 +107,12 @@ TEST_F(ExtensionUnpackerTest, GoodL10n) {
   SetupUnpacker("good_l10n.crx");
   EXPECT_TRUE(unpacker_->Run());
   EXPECT_TRUE(unpacker_->error_message().empty());
-  ASSERT_EQ(2U, unpacker_->parsed_catalogs()->GetSize());
+  ASSERT_EQ(2U, unpacker_->parsed_catalogs()->size());
 }
 
 TEST_F(ExtensionUnpackerTest, NoL10n) {
   SetupUnpacker("no_l10n.crx");
   EXPECT_TRUE(unpacker_->Run());
   EXPECT_TRUE(unpacker_->error_message().empty());
-  EXPECT_EQ(0U, unpacker_->parsed_catalogs()->GetSize());
+  EXPECT_EQ(0U, unpacker_->parsed_catalogs()->size());
 }
diff --git a/chrome/common/pref_service.cc b/chrome/common/pref_service.cc
index 691fb77..ac44166 100644
--- a/chrome/common/pref_service.cc
+++ b/chrome/common/pref_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -435,8 +435,7 @@ void PrefService::SetBoolean(const wchar_t* path, bool value) {
   }
 
   scoped_ptr<Value> old_value(GetPrefCopy(path));
-  bool rv = persistent_->SetBoolean(path, value);
-  DCHECK(rv);
+  persistent_->SetBoolean(path, value);
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -455,8 +454,7 @@ void PrefService::SetInteger(const wchar_t* path, int value) {
   }
 
   scoped_ptr<Value> old_value(GetPrefCopy(path));
-  bool rv = persistent_->SetInteger(path, value);
-  DCHECK(rv);
+  persistent_->SetInteger(path, value);
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -475,8 +473,7 @@ void PrefService::SetReal(const wchar_t* path, double value) {
   }
 
   scoped_ptr<Value> old_value(GetPrefCopy(path));
-  bool rv = persistent_->SetReal(path, value);
-  DCHECK(rv);
+  persistent_->SetReal(path, value);
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -495,8 +492,7 @@ void PrefService::SetString(const wchar_t* path, const std::wstring& value) {
   }
 
   scoped_ptr<Value> old_value(GetPrefCopy(path));
-  bool rv = persistent_->SetString(path, value);
-  DCHECK(rv);
+  persistent_->SetString(path, value);
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -519,11 +515,10 @@ void PrefService::SetFilePath(const wchar_t* path, const FilePath& value) {
   // Value::SetString only knows about UTF8 strings, so convert the path from
   // the system native value to UTF8.
   std::string path_utf8 = WideToUTF8(base::SysNativeMBToWide(value.value()));
-  bool rv = persistent_->SetString(path, path_utf8);
+  persistent_->SetString(path, path_utf8);
 #else
-  bool rv = persistent_->SetString(path, value.value());
+  persistent_->SetString(path, value.value());
 #endif
-  DCHECK(rv);
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -542,8 +537,7 @@ void PrefService::SetInt64(const wchar_t* path, int64 value) {
   }
 
   scoped_ptr<Value> old_value(GetPrefCopy(path));
-  bool rv = persistent_->SetString(path, Int64ToWString(value));
-  DCHECK(rv);
+  persistent_->SetString(path, Int64ToWString(value));
 
   FireObserversIfChanged(path, old_value.get());
 }
@@ -585,11 +579,9 @@ DictionaryValue* PrefService::GetMutableDictionary(const wchar_t* path) {
   }
 
   DictionaryValue* dict = NULL;
-  bool rv = persistent_->GetDictionary(path, &dict);
-  if (!rv) {
+  if (!persistent_->GetDictionary(path, &dict)) {
     dict = new DictionaryValue;
-    rv = persistent_->Set(path, dict);
-    DCHECK(rv);
+    persistent_->Set(path, dict);
   }
   return dict;
 }
@@ -608,11 +600,9 @@ ListValue* PrefService::GetMutableList(const wchar_t* path) {
   }
 
   ListValue* list = NULL;
-  bool rv = persistent_->GetList(path, &list);
-  if (!rv) {
+  if (!persistent_->GetList(path, &list)) {
     list = new ListValue;
-    rv = persistent_->Set(path, list);
-    DCHECK(rv);
+    persistent_->Set(path, list);
   }
   return list;
 }
diff --git a/chrome/common/pref_service_unittest.cc b/chrome/common/pref_service_unittest.cc
index 29b19c3..bfb96b0 100644
--- a/chrome/common/pref_service_unittest.cc
+++ b/chrome/common/pref_service_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -117,7 +117,7 @@ TEST_F(PrefServiceTest, Basic) {
 
   // Now test that the transient value overrides the persistent value,
   // without actually altering the persistent store.
-  EXPECT_TRUE(prefs.transient()->SetString(prefs::kHomePage, microsoft));
+  prefs.transient()->SetString(prefs::kHomePage, microsoft);
   EXPECT_TRUE(prefs.transient()->GetString(prefs::kHomePage, &homepage));
   EXPECT_EQ(microsoft, homepage);