Remove emtpy lists and empty dictionaries from Preferences and

Local State when writing to disk.

BUG=28836

Review URL: http://codereview.chromium.org/449074

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

3 files changed, 181 insertions, 42 deletions

diff --git a/base/values.cc b/base/values.cc
index 1f21d93..6071df8 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -2,10 +2,62 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/values.h"
+
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
-#include "base/values.h"
+
+namespace {
+
+// Make a deep copy of |node|, but don't include empty lists or dictionaries
+// in the copy. It's possible for this function to return NULL and it
+// expects |node| to always be non-NULL.
+Value* CopyWithoutEmptyChildren(Value* node) {
+  DCHECK(node);
+  switch (node->GetType()) {
+    case Value::TYPE_LIST: {
+      ListValue* list = static_cast<ListValue*>(node);
+      ListValue* copy = new ListValue;
+      for (ListValue::const_iterator it = list->begin(); it != list->end();
+           ++it) {
+        Value* child_copy = CopyWithoutEmptyChildren(*it);
+        if (child_copy)
+          copy->Append(child_copy);
+      }
+      if (!copy->empty())
+        return copy;
+
+      delete copy;
+      return NULL;
+    }
+
+    case Value::TYPE_DICTIONARY: {
+      DictionaryValue* dict = static_cast<DictionaryValue*>(node);
+      DictionaryValue* copy = new DictionaryValue;
+      for (DictionaryValue::key_iterator it = dict->begin_keys();
+           it != dict->end_keys(); ++it) {
+        Value* child = NULL;
+        bool rv = dict->GetWithoutPathExpansion(*it, &child);
+        DCHECK(rv);
+        Value* child_copy = CopyWithoutEmptyChildren(child);
+        if (child_copy)
+          copy->SetWithoutPathExpansion(*it, child_copy);
+      }
+      if (!copy->empty())
+        return copy;
+
+      delete copy;
+      return NULL;
+    }
+
+    default:
+      // For everything else, just make a copy.
+      return node->DeepCopy();
+  }
+}
+
+}  // namespace
 
 ///////////////////// Value ////////////////////
 
@@ -237,14 +289,41 @@ DictionaryValue::~DictionaryValue() {
   Clear();
 }
 
-void DictionaryValue::Clear() {
-  ValueMap::iterator dict_iterator = dictionary_.begin();
-  while (dict_iterator != dictionary_.end()) {
-    delete dict_iterator->second;
-    ++dict_iterator;
+Value* DictionaryValue::DeepCopy() const {
+  DictionaryValue* result = new DictionaryValue;
+
+  for (ValueMap::const_iterator current_entry(dictionary_.begin());
+       current_entry != dictionary_.end(); ++current_entry) {
+    result->SetWithoutPathExpansion(current_entry->first,
+                                    current_entry->second->DeepCopy());
   }
 
-  dictionary_.clear();
+  return result;
+}
+
+bool DictionaryValue::Equals(const Value* other) const {
+  if (other->GetType() != GetType())
+    return false;
+
+  const DictionaryValue* other_dict =
+      static_cast<const DictionaryValue*>(other);
+  key_iterator lhs_it(begin_keys());
+  key_iterator rhs_it(other_dict->begin_keys());
+  while (lhs_it != end_keys() && rhs_it != other_dict->end_keys()) {
+    Value* lhs;
+    Value* rhs;
+    if (!GetWithoutPathExpansion(*lhs_it, &lhs) ||
+        !other_dict->GetWithoutPathExpansion(*rhs_it, &rhs) ||
+        !lhs->Equals(rhs)) {
+      return false;
+    }
+    ++lhs_it;
+    ++rhs_it;
+  }
+  if (lhs_it != end_keys() || rhs_it != other_dict->end_keys())
+    return false;
+
+  return true;
 }
 
 bool DictionaryValue::HasKey(const std::wstring& key) const {
@@ -253,6 +332,16 @@ bool DictionaryValue::HasKey(const std::wstring& key) const {
   return current_entry != dictionary_.end();
 }
 
+void DictionaryValue::Clear() {
+  ValueMap::iterator dict_iterator = dictionary_.begin();
+  while (dict_iterator != dictionary_.end()) {
+    delete dict_iterator->second;
+    ++dict_iterator;
+  }
+
+  dictionary_.clear();
+}
+
 void DictionaryValue::Set(const std::wstring& path, Value* in_value) {
   DCHECK(in_value);
 
@@ -510,41 +599,9 @@ bool DictionaryValue::RemoveWithoutPathExpansion(const std::wstring& key,
   return true;
 }
 
-Value* DictionaryValue::DeepCopy() const {
-  DictionaryValue* result = new DictionaryValue;
-
-  for (ValueMap::const_iterator current_entry(dictionary_.begin());
-       current_entry != dictionary_.end(); ++current_entry) {
-    result->SetWithoutPathExpansion(current_entry->first,
-                                    current_entry->second->DeepCopy());
-  }
-
-  return result;
-}
-
-bool DictionaryValue::Equals(const Value* other) const {
-  if (other->GetType() != GetType())
-    return false;
-
-  const DictionaryValue* other_dict =
-      static_cast<const DictionaryValue*>(other);
-  key_iterator lhs_it(begin_keys());
-  key_iterator rhs_it(other_dict->begin_keys());
-  while (lhs_it != end_keys() && rhs_it != other_dict->end_keys()) {
-    Value* lhs;
-    Value* rhs;
-    if (!GetWithoutPathExpansion(*lhs_it, &lhs) ||
-        !other_dict->GetWithoutPathExpansion(*rhs_it, &rhs) ||
-        !lhs->Equals(rhs)) {
-      return false;
-    }
-    ++lhs_it;
-    ++rhs_it;
-  }
-  if (lhs_it != end_keys() || rhs_it != other_dict->end_keys())
-    return false;
-
-  return true;
+DictionaryValue* DictionaryValue::DeepCopyWithoutEmptyChildren() {
+  Value* copy = CopyWithoutEmptyChildren(this);
+  return copy ? static_cast<DictionaryValue*>(copy) : new DictionaryValue;
 }
 
 ///////////////////// ListValue ////////////////////
diff --git a/base/values.h b/base/values.h
index 82a29fd..1bbc581 100644
--- a/base/values.h
+++ b/base/values.h
@@ -287,6 +287,10 @@ class DictionaryValue : public Value {
   // to be used as paths.
   bool RemoveWithoutPathExpansion(const std::wstring& key, Value** out_value);
 
+  // Makes a copy of |this| but doesn't include empty dictionaries and lists in
+  // the copy.  This never returns NULL, even if |this| itself is empty.
+  DictionaryValue* DeepCopyWithoutEmptyChildren();
+
   // This class provides an iterator for the keys in the dictionary.
   // It can't be used to modify the dictionary.
   //
@@ -331,6 +335,9 @@ class ListValue : public Value {
   // Returns the number of Values in this list.
   size_t GetSize() const { return list_.size(); }
 
+  // Returns whether the list is empty.
+  bool empty() const { return list_.empty(); }
+
   // Sets the list item at the given index to be the Value specified by
   // the value given.  If the index beyond the current end of the list, null
   // Values will be used to pad out the list.
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 5835e2b..912b380 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -478,3 +478,78 @@ TEST(ValuesTest, Equals) {
   EXPECT_FALSE(dv.Equals(copy));
   delete copy;
 }
+
+TEST(ValuesTest, RemoveEmptyChildren) {
+  scoped_ptr<DictionaryValue> root(new DictionaryValue);
+  // Remove empty lists and dictionaries.
+  root->Set(L"empty_dict", new DictionaryValue);
+  root->Set(L"empty_list", new ListValue);
+  root->SetWithoutPathExpansion(L"a.b.c.d.e", new DictionaryValue);
+  root.reset(root->DeepCopyWithoutEmptyChildren());
+  EXPECT_TRUE(root->empty());
+
+  // Make sure we don't prune too much.
+  root->SetBoolean(L"bool", true);
+  root->Set(L"empty_dict", new DictionaryValue);
+  root->SetString(L"empty_string", "");
+  root.reset(root->DeepCopyWithoutEmptyChildren());
+  EXPECT_EQ(2U, root->size());
+
+  // Should do nothing.
+  root.reset(root->DeepCopyWithoutEmptyChildren());
+  EXPECT_EQ(2U, root->size());
+
+  // Nested test cases.  These should all reduce back to the bool and string
+  // set above.
+  {
+    root->Set(L"a.b.c.d.e", new DictionaryValue);
+    root.reset(root->DeepCopyWithoutEmptyChildren());
+    EXPECT_EQ(2U, root->size());
+  }
+  {
+    DictionaryValue* inner = new DictionaryValue;
+    root->Set(L"dict_with_emtpy_children", inner);
+    inner->Set(L"empty_dict", new DictionaryValue);
+    inner->Set(L"empty_list", new ListValue);
+    root.reset(root->DeepCopyWithoutEmptyChildren());
+    EXPECT_EQ(2U, root->size());
+  }
+  {
+    ListValue* inner = new ListValue;
+    root->Set(L"list_with_empty_children", inner);
+    inner->Append(new DictionaryValue);
+    inner->Append(new ListValue);
+    root.reset(root->DeepCopyWithoutEmptyChildren());
+    EXPECT_EQ(2U, root->size());
+  }
+
+  // Nested with siblings.
+  {
+    ListValue* inner = new ListValue;
+    root->Set(L"list_with_empty_children", inner);
+    inner->Append(new DictionaryValue);
+    inner->Append(new ListValue);
+    DictionaryValue* inner2 = new DictionaryValue;
+    root->Set(L"dict_with_empty_children", inner2);
+    inner2->Set(L"empty_dict", new DictionaryValue);
+    inner2->Set(L"empty_list", new ListValue);
+    root.reset(root->DeepCopyWithoutEmptyChildren());
+    EXPECT_EQ(2U, root->size());
+  }
+
+  // Make sure nested values don't get pruned.
+  {
+    ListValue* inner = new ListValue;
+    root->Set(L"list_with_empty_children", inner);
+    ListValue* inner2 = new ListValue;
+    inner->Append(new DictionaryValue);
+    inner->Append(inner2);
+    inner2->Append(Value::CreateStringValue("hello"));
+    root.reset(root->DeepCopyWithoutEmptyChildren());
+    EXPECT_EQ(3U, root->size());
+    EXPECT_TRUE(root->GetList(L"list_with_empty_children", &inner));
+    EXPECT_EQ(1U, inner->GetSize());  // Dictionary was pruned.
+    EXPECT_TRUE(inner->GetList(0, &inner2));
+    EXPECT_EQ(1U, inner2->GetSize());
+  }
+}