Remove JsonPrefStore pruning of empty values on write.

Written from https://codereview.chromium.org/12092021

The entire source code was surveyed for existing list/dict prefs for which this change could be problematic, it doesn't look like anything is relying on this internal detail of the API, see this for details: https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AtwXJ4IPPZBAdG9rX3RTc3k5Z1pyN3U4b3d4Tkota3c#gid=0

TBR=joth (for minor android_webview side-effects)
BUG=323346

Review URL: https://codereview.chromium.org/81183005

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

23 files changed, 83 insertions, 279 deletions

diff --git a/android_webview/browser/aw_pref_store.cc b/android_webview/browser/aw_pref_store.cc
index 0720ee9..40a41a0 100644
--- a/android_webview/browser/aw_pref_store.cc
+++ b/android_webview/browser/aw_pref_store.cc
@@ -52,9 +52,6 @@ void AwPrefStore::RemoveValue(const std::string& key) {
     ReportValueChanged(key);
 }
 
-void AwPrefStore::MarkNeedsEmptyValue(const std::string& key) {
-}
-
 bool AwPrefStore::ReadOnly() const {
   return false;
 }
diff --git a/android_webview/browser/aw_pref_store.h b/android_webview/browser/aw_pref_store.h
index 8581a03..1504d93 100644
--- a/android_webview/browser/aw_pref_store.h
+++ b/android_webview/browser/aw_pref_store.h
@@ -37,7 +37,6 @@ class AwPrefStore : public PersistentPrefStore {
   virtual void SetValueSilently(const std::string& key,
                                 base::Value* value) OVERRIDE;
   virtual void RemoveValue(const std::string& key) OVERRIDE;
-  virtual void MarkNeedsEmptyValue(const std::string& key) OVERRIDE;
   virtual bool ReadOnly() const OVERRIDE;
   virtual PrefReadError GetReadError() const OVERRIDE;
   virtual PersistentPrefStore::PrefReadError ReadPrefs() OVERRIDE;
diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index e230407..ad97b84 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -217,14 +217,10 @@ void JsonPrefStore::SetValueSilently(const std::string& key,
 }
 
 void JsonPrefStore::RemoveValue(const std::string& key) {
-  if (prefs_->Remove(key, NULL))
+  if (prefs_->RemovePath(key, NULL))
     ReportValueChanged(key);
 }
 
-void JsonPrefStore::MarkNeedsEmptyValue(const std::string& key) {
-  keys_need_empty_value_.insert(key);
-}
-
 bool JsonPrefStore::ReadOnly() const {
   return read_only_;
 }
@@ -324,35 +320,7 @@ JsonPrefStore::~JsonPrefStore() {
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {
-  // TODO(tc): Do we want to prune webkit preferences that match the default
-  // value?
   JSONStringValueSerializer serializer(output);
   serializer.set_pretty_print(true);
-  scoped_ptr<base::DictionaryValue> copy(
-      prefs_->DeepCopyWithoutEmptyChildren());
-
-  // Iterates |keys_need_empty_value_| and if the key exists in |prefs_|,
-  // ensure its empty ListValue or DictonaryValue is preserved.
-  for (std::set<std::string>::const_iterator
-       it = keys_need_empty_value_.begin();
-       it != keys_need_empty_value_.end();
-       ++it) {
-    const std::string& key = *it;
-
-    base::Value* value = NULL;
-    if (!prefs_->Get(key, &value))
-      continue;
-
-    if (value->IsType(base::Value::TYPE_LIST)) {
-      const base::ListValue* list = NULL;
-      if (value->GetAsList(&list) && list->empty())
-        copy->Set(key, new base::ListValue);
-    } else if (value->IsType(base::Value::TYPE_DICTIONARY)) {
-      const base::DictionaryValue* dict = NULL;
-      if (value->GetAsDictionary(&dict) && dict->empty())
-        copy->Set(key, new base::DictionaryValue);
-    }
-  }
-
-  return serializer.Serialize(*(copy.get()));
+  return serializer.Serialize(*prefs_);
 }
diff --git a/base/prefs/json_pref_store.h b/base/prefs/json_pref_store.h
index 9e6c182..21fc8f9 100644
--- a/base/prefs/json_pref_store.h
+++ b/base/prefs/json_pref_store.h
@@ -21,8 +21,8 @@
 namespace base {
 class DictionaryValue;
 class FilePath;
-class SequencedWorkerPool;
 class SequencedTaskRunner;
+class SequencedWorkerPool;
 class Value;
 }
 
@@ -58,7 +58,6 @@ class BASE_PREFS_EXPORT JsonPrefStore
   virtual void SetValueSilently(const std::string& key,
                                 base::Value* value) OVERRIDE;
   virtual void RemoveValue(const std::string& key) OVERRIDE;
-  virtual void MarkNeedsEmptyValue(const std::string& key) OVERRIDE;
   virtual bool ReadOnly() const OVERRIDE;
   virtual PrefReadError GetReadError() const OVERRIDE;
   virtual PrefReadError ReadPrefs() OVERRIDE;
diff --git a/base/prefs/json_pref_store_unittest.cc b/base/prefs/json_pref_store_unittest.cc
index 34e1b8a..a26afd7 100644
--- a/base/prefs/json_pref_store_unittest.cc
+++ b/base/prefs/json_pref_store_unittest.cc
@@ -216,6 +216,33 @@ TEST_F(JsonPrefStoreTest, BasicAsync) {
       pref_store.get(), input_file, data_dir_.AppendASCII("write.golden.json"));
 }
 
+TEST_F(JsonPrefStoreTest, PreserveEmptyValues) {
+  FilePath pref_file = temp_dir_.path().AppendASCII("empty_values.json");
+
+  scoped_refptr<JsonPrefStore> pref_store =
+      new JsonPrefStore(pref_file, message_loop_.message_loop_proxy());
+
+  // Set some keys with empty values.
+  pref_store->SetValue("list", new base::ListValue);
+  pref_store->SetValue("dict", new base::DictionaryValue);
+
+  // Write to file.
+  pref_store->CommitPendingWrite();
+  MessageLoop::current()->RunUntilIdle();
+
+  // Reload.
+  pref_store = new JsonPrefStore(pref_file, message_loop_.message_loop_proxy());
+  ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
+  ASSERT_FALSE(pref_store->ReadOnly());
+
+  // Check values.
+  const Value* result = NULL;
+  EXPECT_TRUE(pref_store->GetValue("list", &result));
+  EXPECT_TRUE(ListValue().Equals(result));
+  EXPECT_TRUE(pref_store->GetValue("dict", &result));
+  EXPECT_TRUE(DictionaryValue().Equals(result));
+}
+
 // Tests asynchronous reading of the file when there is no file.
 TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
   base::FilePath bogus_input_file = data_dir_.AppendASCII("read.txt");
@@ -237,51 +264,4 @@ TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) {
   EXPECT_FALSE(pref_store->ReadOnly());
 }
 
-TEST_F(JsonPrefStoreTest, NeedsEmptyValue) {
-  base::FilePath pref_file = temp_dir_.path().AppendASCII("write.json");
-
-  ASSERT_TRUE(base::CopyFile(
-      data_dir_.AppendASCII("read.need_empty_value.json"),
-      pref_file));
-
-  // Test that the persistent value can be loaded.
-  ASSERT_TRUE(PathExists(pref_file));
-  scoped_refptr<JsonPrefStore> pref_store =
-      new JsonPrefStore(pref_file, message_loop_.message_loop_proxy().get());
-  ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs());
-  ASSERT_FALSE(pref_store->ReadOnly());
-
-  // The JSON file looks like this:
-  // {
-  //   "list": [ 1 ],
-  //   "list_needs_empty_value": [ 2 ],
-  //   "dict": {
-  //     "dummy": true,
-  //   },
-  //   "dict_needs_empty_value": {
-  //     "dummy": true,
-  //   },
-  // }
-
-  // Set flag to preserve empty values for the following keys.
-  pref_store->MarkNeedsEmptyValue("list_needs_empty_value");
-  pref_store->MarkNeedsEmptyValue("dict_needs_empty_value");
-
-  // Set all keys to empty values.
-  pref_store->SetValue("list", new base::ListValue);
-  pref_store->SetValue("list_needs_empty_value", new base::ListValue);
-  pref_store->SetValue("dict", new base::DictionaryValue);
-  pref_store->SetValue("dict_needs_empty_value", new base::DictionaryValue);
-
-  // Write to file.
-  pref_store->CommitPendingWrite();
-  RunLoop().RunUntilIdle();
-
-  // Compare to expected output.
-  base::FilePath golden_output_file =
-      data_dir_.AppendASCII("write.golden.need_empty_value.json");
-  ASSERT_TRUE(PathExists(golden_output_file));
-  EXPECT_TRUE(TextContentsEqual(golden_output_file, pref_file));
-}
-
 }  // namespace base
diff --git a/base/prefs/overlay_user_pref_store.cc b/base/prefs/overlay_user_pref_store.cc
index 47668cc..a708bb6 100644
--- a/base/prefs/overlay_user_pref_store.cc
+++ b/base/prefs/overlay_user_pref_store.cc
@@ -93,11 +93,6 @@ void OverlayUserPrefStore::RemoveValue(const std::string& key) {
     ReportValueChanged(key);
 }
 
-void OverlayUserPrefStore::MarkNeedsEmptyValue(const std::string& key) {
-  if (!ShallBeStoredInOverlay(key))
-    underlay_->MarkNeedsEmptyValue(key);
-}
-
 bool OverlayUserPrefStore::ReadOnly() const {
   return false;
 }
diff --git a/base/prefs/overlay_user_pref_store.h b/base/prefs/overlay_user_pref_store.h
index 1895ac0..c9993b9 100644
--- a/base/prefs/overlay_user_pref_store.h
+++ b/base/prefs/overlay_user_pref_store.h
@@ -44,7 +44,6 @@ class BASE_PREFS_EXPORT OverlayUserPrefStore : public PersistentPrefStore,
   virtual void SetValueSilently(const std::string& key,
                                 base::Value* value) OVERRIDE;
   virtual void RemoveValue(const std::string& key) OVERRIDE;
-  virtual void MarkNeedsEmptyValue(const std::string& key) OVERRIDE;
   virtual bool ReadOnly() const OVERRIDE;
   virtual PrefReadError GetReadError() const OVERRIDE;
   virtual PrefReadError ReadPrefs() OVERRIDE;
diff --git a/base/prefs/persistent_pref_store.h b/base/prefs/persistent_pref_store.h
index 0baf02a..811ebff 100644
--- a/base/prefs/persistent_pref_store.h
+++ b/base/prefs/persistent_pref_store.h
@@ -63,10 +63,6 @@ class BASE_PREFS_EXPORT PersistentPrefStore : public PrefStore {
   // Removes the value for |key|.
   virtual void RemoveValue(const std::string& key) = 0;
 
-  // Marks that the |key| with empty ListValue/DictionaryValue needs to be
-  // persisted.
-  virtual void MarkNeedsEmptyValue(const std::string& key) = 0;
-
   // Whether the store is in a pseudo-read-only mode where changes are not
   // actually persisted to disk.  This happens in some cases when there are
   // read errors during startup.
diff --git a/base/prefs/pref_registry.cc b/base/prefs/pref_registry.cc
index 31d788d..9d6b05c 100644
--- a/base/prefs/pref_registry.cc
+++ b/base/prefs/pref_registry.cc
@@ -42,11 +42,6 @@ void PrefRegistry::SetDefaultPrefValue(const char* pref_name,
   defaults_->ReplaceDefaultValue(pref_name, make_scoped_ptr(value));
 }
 
-void PrefRegistry::SetRegistrationCallback(
-    const RegistrationCallback& callback) {
-  registration_callback_ = callback;
-}
-
 void PrefRegistry::RegisterPreference(const char* path,
                                       base::Value* default_value) {
   base::Value::Type orig_type = default_value->GetType();
@@ -57,7 +52,4 @@ void PrefRegistry::RegisterPreference(const char* path,
       "Trying to register a previously registered pref: " << path;
 
   defaults_->SetDefaultValue(path, make_scoped_ptr(default_value));
-
-  if (!registration_callback_.is_null())
-    registration_callback_.Run(path, default_value);
 }
diff --git a/base/prefs/pref_registry.h b/base/prefs/pref_registry.h
index 6c7eac9f..896db3f 100644
--- a/base/prefs/pref_registry.h
+++ b/base/prefs/pref_registry.h
@@ -5,7 +5,6 @@
 #ifndef BASE_PREFS_PREF_REGISTRY_H_
 #define BASE_PREFS_PREF_REGISTRY_H_
 
-#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/prefs/base_prefs_export.h"
 #include "base/prefs/pref_value_map.h"
@@ -29,7 +28,6 @@ class PrefStore;
 class BASE_PREFS_EXPORT PrefRegistry : public base::RefCounted<PrefRegistry> {
  public:
   typedef PrefValueMap::const_iterator const_iterator;
-  typedef base::Callback<void(const char*, base::Value*)> RegistrationCallback;
 
   PrefRegistry();
 
@@ -45,15 +43,6 @@ class BASE_PREFS_EXPORT PrefRegistry : public base::RefCounted<PrefRegistry> {
   // |pref_name| must be a previously registered preference.
   void SetDefaultPrefValue(const char* pref_name, base::Value* value);
 
-  // Exactly one callback can be set for registration. The callback
-  // will be invoked each time registration has been performed on this
-  // object.
-  //
-  // Calling this method after a callback has already been set will
-  // make the object forget the previous callback and use the new one
-  // instead.
-  void SetRegistrationCallback(const RegistrationCallback& callback);
-
  protected:
   friend class base::RefCounted<PrefRegistry>;
   virtual ~PrefRegistry();
@@ -64,8 +53,6 @@ class BASE_PREFS_EXPORT PrefRegistry : public base::RefCounted<PrefRegistry> {
   scoped_refptr<DefaultPrefStore> defaults_;
 
  private:
-  RegistrationCallback registration_callback_;
-
   DISALLOW_COPY_AND_ASSIGN(PrefRegistry);
 };
 
diff --git a/base/prefs/pref_service.cc b/base/prefs/pref_service.cc
index 4c707a5..576043b 100644
--- a/base/prefs/pref_service.cc
+++ b/base/prefs/pref_service.cc
@@ -53,20 +53,12 @@ PrefService::PrefService(
       read_error_callback_(read_error_callback) {
   pref_notifier_->SetPrefService(this);
 
-  pref_registry_->SetRegistrationCallback(
-      base::Bind(&PrefService::AddRegisteredPreference,
-                 base::Unretained(this)));
-  AddInitialPreferences();
-
   InitFromStorage(async);
 }
 
 PrefService::~PrefService() {
   DCHECK(CalledOnValidThread());
 
-  // Remove our callback, setting a NULL one.
-  pref_registry_->SetRegistrationCallback(PrefRegistry::RegistrationCallback());
-
   // Reset pointers so accesses after destruction reliably crash.
   pref_value_store_.reset();
   pref_registry_ = NULL;
@@ -297,10 +289,6 @@ const base::Value* PrefService::GetDefaultPrefValue(const char* path) const {
   return value;
 }
 
-void PrefService::MarkUserStoreNeedsEmptyValue(const std::string& key) const {
-  user_pref_store_->MarkNeedsEmptyValue(key);
-}
-
 const base::ListValue* PrefService::GetList(const char* path) const {
   DCHECK(CalledOnValidThread());
 
@@ -332,42 +320,6 @@ PrefRegistry* PrefService::DeprecatedGetPrefRegistry() {
   return pref_registry_.get();
 }
 
-void PrefService::AddInitialPreferences() {
-  for (PrefRegistry::const_iterator it = pref_registry_->begin();
-       it != pref_registry_->end();
-       ++it) {
-    AddRegisteredPreference(it->first.c_str(), it->second);
-  }
-}
-
-// TODO(joi): Once MarkNeedsEmptyValue is gone, we can probably
-// completely get rid of this method. There will be one difference in
-// semantics; currently all registered preferences are stored right
-// away in the prefs_map_, if we remove this they would be stored only
-// opportunistically.
-void PrefService::AddRegisteredPreference(const char* path,
-                                          base::Value* default_value) {
-  DCHECK(CalledOnValidThread());
-
-  // For ListValue and DictionaryValue with non empty default, empty value
-  // for |path| needs to be persisted in |user_pref_store_|. So that
-  // non empty default is not used when user sets an empty ListValue or
-  // DictionaryValue.
-  bool needs_empty_value = false;
-  base::Value::Type orig_type = default_value->GetType();
-  if (orig_type == base::Value::TYPE_LIST) {
-    const base::ListValue* list = NULL;
-    if (default_value->GetAsList(&list) && !list->empty())
-      needs_empty_value = true;
-  } else if (orig_type == base::Value::TYPE_DICTIONARY) {
-    const base::DictionaryValue* dict = NULL;
-    if (default_value->GetAsDictionary(&dict) && !dict->empty())
-      needs_empty_value = true;
-  }
-  if (needs_empty_value)
-    user_pref_store_->MarkNeedsEmptyValue(path);
-}
-
 void PrefService::ClearPref(const char* path) {
   DCHECK(CalledOnValidThread());
 
diff --git a/base/prefs/pref_service.h b/base/prefs/pref_service.h
index 9f4dc7c..176594a 100644
--- a/base/prefs/pref_service.h
+++ b/base/prefs/pref_service.h
@@ -221,13 +221,6 @@ class BASE_PREFS_EXPORT PrefService : public base::NonThreadSafe {
   // registered preference. In that case, will never return NULL.
   const base::Value* GetDefaultPrefValue(const char* path) const;
 
-  // Deprecated. Do not add calls to this method.
-  // Marks that the user store should not prune out empty values for |key| when
-  // writting to disk.
-  // TODO(gab): Enforce this at a lower level for all values and remove this
-  // method.
-  void MarkUserStoreNeedsEmptyValue(const std::string& key) const;
-
   // Returns true if a value has been set for the specified path.
   // NOTE: this is NOT the same as FindPreference. In particular
   // FindPreference returns whether RegisterXXX has been invoked, where as
@@ -268,17 +261,6 @@ class BASE_PREFS_EXPORT PrefService : public base::NonThreadSafe {
   PrefRegistry* DeprecatedGetPrefRegistry();
 
  protected:
-  // Adds the registered preferences from the PrefRegistry instance
-  // passed to us at construction time.
-  void AddInitialPreferences();
-
-  // Updates local caches for a preference registered at |path|. The
-  // |default_value| must not be NULL as it determines the preference
-  // value's type.  AddRegisteredPreference must not be called twice
-  // for the same path.
-  void AddRegisteredPreference(const char* path,
-                               base::Value* default_value);
-
   // The PrefNotifier handles registering and notifying preference observers.
   // It is created and owned by this PrefService. Subclasses may access it for
   // unit testing.
diff --git a/base/prefs/testing_pref_store.cc b/base/prefs/testing_pref_store.cc
index b420969..2f429c9 100644
--- a/base/prefs/testing_pref_store.cc
+++ b/base/prefs/testing_pref_store.cc
@@ -53,9 +53,6 @@ void TestingPrefStore::RemoveValue(const std::string& key) {
     NotifyPrefValueChanged(key);
 }
 
-void TestingPrefStore::MarkNeedsEmptyValue(const std::string& key) {
-}
-
 bool TestingPrefStore::ReadOnly() const {
   return read_only_;
 }
diff --git a/base/prefs/testing_pref_store.h b/base/prefs/testing_pref_store.h
index 08d7125..c6a2b83 100644
--- a/base/prefs/testing_pref_store.h
+++ b/base/prefs/testing_pref_store.h
@@ -36,7 +36,6 @@ class TestingPrefStore : public PersistentPrefStore {
   virtual void SetValueSilently(const std::string& key,
                                 base::Value* value) OVERRIDE;
   virtual void RemoveValue(const std::string& key) OVERRIDE;
-  virtual void MarkNeedsEmptyValue(const std::string& key) OVERRIDE;
   virtual bool ReadOnly() const OVERRIDE;
   virtual PrefReadError GetReadError() const OVERRIDE;
   virtual PersistentPrefStore::PrefReadError ReadPrefs() OVERRIDE;
diff --git a/base/test/data/prefs/read.need_empty_value.json b/base/test/data/prefs/read.need_empty_value.json
deleted file mode 100644
index 48e1749..0000000
--- a/base/test/data/prefs/read.need_empty_value.json
+++ /dev/null
@@ -1,10 +0,0 @@
-{
-  "list": [ 1 ],
-  "list_needs_empty_value": [ 2 ],
-  "dict": {
-    "dummy": true
-  },
-  "dict_needs_empty_value": {
-    "dummy": true
-  }
-}
diff --git a/base/test/data/prefs/write.golden.need_empty_value.json b/base/test/data/prefs/write.golden.need_empty_value.json
deleted file mode 100644
index fa79590..0000000
--- a/base/test/data/prefs/write.golden.need_empty_value.json
+++ /dev/null
@@ -1,6 +0,0 @@
-{
-   "dict_needs_empty_value": {
-
-   },
-   "list_needs_empty_value": [  ]
-}
diff --git a/base/values.cc b/base/values.cc
index 5196cec..f2a7743 100644
--- a/base/values.cc
+++ b/base/values.cc
@@ -462,8 +462,8 @@ void DictionaryValue::SetStringWithoutPathExpansion(
   SetWithoutPathExpansion(path, CreateStringValue(in_value));
 }
 
-bool DictionaryValue::Get(
-    const std::string& path, const Value** out_value) const {
+bool DictionaryValue::Get(const std::string& path,
+                          const Value** out_value) const {
   DCHECK(IsStringUTF8(path));
   std::string current_path(path);
   const DictionaryValue* current_dictionary = this;
@@ -752,6 +752,26 @@ bool DictionaryValue::RemoveWithoutPathExpansion(const std::string& key,
   return true;
 }
 
+bool DictionaryValue::RemovePath(const std::string& path,
+                                 scoped_ptr<Value>* out_value) {
+  bool result = false;
+  size_t delimiter_position = path.find('.');
+
+  if (delimiter_position == std::string::npos)
+    return RemoveWithoutPathExpansion(path, out_value);
+
+  const std::string subdict_path = path.substr(0, delimiter_position);
+  DictionaryValue* subdict = NULL;
+  if (!GetDictionary(subdict_path, &subdict))
+    return false;
+  result = subdict->RemovePath(path.substr(delimiter_position + 1),
+                               out_value);
+  if (result && subdict->empty())
+    RemoveWithoutPathExpansion(subdict_path, NULL);
+
+  return result;
+}
+
 DictionaryValue* DictionaryValue::DeepCopyWithoutEmptyChildren() const {
   Value* copy = CopyWithoutEmptyChildren(this);
   return copy ? static_cast<DictionaryValue*>(copy) : new DictionaryValue;
diff --git a/base/values.h b/base/values.h
index 3bc1f8b..bffdbc7 100644
--- a/base/values.h
+++ b/base/values.h
@@ -324,6 +324,11 @@ class BASE_EXPORT DictionaryValue : public Value {
   virtual bool RemoveWithoutPathExpansion(const std::string& key,
                                           scoped_ptr<Value>* out_value);
 
+  // Removes a path, clearing out all dictionaries on |path| that remain empty
+  // after removing the value at |path|.
+  virtual bool RemovePath(const std::string& path,
+                          scoped_ptr<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() const;
diff --git a/base/values_unittest.cc b/base/values_unittest.cc
index 733c485..70acdfd 100644
--- a/base/values_unittest.cc
+++ b/base/values_unittest.cc
@@ -323,6 +323,31 @@ TEST(ValuesTest, DictionaryWithoutPathExpansion) {
   EXPECT_EQ(Value::TYPE_NULL, value4->GetType());
 }
 
+TEST(ValuesTest, DictionaryRemovePath) {
+  DictionaryValue dict;
+  dict.Set("a.long.way.down", Value::CreateIntegerValue(1));
+  dict.Set("a.long.key.path", Value::CreateBooleanValue(true));
+
+  scoped_ptr<Value> removed_item;
+  EXPECT_TRUE(dict.RemovePath("a.long.way.down", &removed_item));
+  ASSERT_TRUE(removed_item);
+  EXPECT_TRUE(removed_item->IsType(base::Value::TYPE_INTEGER));
+  EXPECT_FALSE(dict.HasKey("a.long.way.down"));
+  EXPECT_FALSE(dict.HasKey("a.long.way"));
+  EXPECT_TRUE(dict.Get("a.long.key.path", NULL));
+
+  removed_item.reset();
+  EXPECT_FALSE(dict.RemovePath("a.long.way.down", &removed_item));
+  EXPECT_FALSE(removed_item);
+  EXPECT_TRUE(dict.Get("a.long.key.path", NULL));
+
+  removed_item.reset();
+  EXPECT_TRUE(dict.RemovePath("a.long.key.path", &removed_item));
+  ASSERT_TRUE(removed_item);
+  EXPECT_TRUE(removed_item->IsType(base::Value::TYPE_BOOLEAN));
+  EXPECT_TRUE(dict.empty());
+}
+
 TEST(ValuesTest, DeepCopy) {
   DictionaryValue original_dict;
   Value* original_null = Value::CreateNullValue();
diff --git a/chrome/browser/prefs/chrome_pref_service_unittest.cc b/chrome/browser/prefs/chrome_pref_service_unittest.cc
index 4a453fd4..0d27501 100644
--- a/chrome/browser/prefs/chrome_pref_service_unittest.cc
+++ b/chrome/browser/prefs/chrome_pref_service_unittest.cc
@@ -88,59 +88,6 @@ class ChromePrefServiceUserFilePrefsTest : public testing::Test {
   base::MessageLoop message_loop_;
 };
 
-// Verifies that ListValue and DictionaryValue pref with non emtpy default
-// preserves its empty value.
-TEST_F(ChromePrefServiceUserFilePrefsTest, PreserveEmptyValue) {
-  base::FilePath pref_file = temp_dir_.path().AppendASCII("write.json");
-
-  ASSERT_TRUE(base::CopyFile(
-      data_dir_.AppendASCII("read.need_empty_value.json"),
-      pref_file));
-
-  PrefServiceMockFactory factory;
-  factory.SetUserPrefsFile(pref_file,
-                          message_loop_.message_loop_proxy().get());
-  scoped_refptr<user_prefs::PrefRegistrySyncable> registry(
-      new user_prefs::PrefRegistrySyncable);
-  scoped_ptr<PrefServiceSyncable> prefs(factory.CreateSyncable(registry.get()));
-
-  // Register testing prefs.
-  registry->RegisterListPref("list",
-                             user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-  registry->RegisterDictionaryPref(
-      "dict",
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-
-  base::ListValue* non_empty_list = new base::ListValue;
-  non_empty_list->Append(base::Value::CreateStringValue("test"));
-  registry->RegisterListPref("list_needs_empty_value",
-                             non_empty_list,
-                             user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-
-  base::DictionaryValue* non_empty_dict = new base::DictionaryValue;
-  non_empty_dict->SetString("dummy", "whatever");
-  registry->RegisterDictionaryPref(
-      "dict_needs_empty_value",
-      non_empty_dict,
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
-
-  // Set all testing prefs to empty.
-  ClearListValue(prefs.get(), "list");
-  ClearListValue(prefs.get(), "list_needs_empty_value");
-  ClearDictionaryValue(prefs.get(), "dict");
-  ClearDictionaryValue(prefs.get(), "dict_needs_empty_value");
-
-  // Write to file.
-  prefs->CommitPendingWrite();
-  message_loop_.RunUntilIdle();
-
-  // Compare to expected output.
-  base::FilePath golden_output_file =
-      data_dir_.AppendASCII("write.golden.need_empty_value.json");
-  ASSERT_TRUE(base::PathExists(golden_output_file));
-  EXPECT_TRUE(base::TextContentsEqual(golden_output_file, pref_file));
-}
-
 class ChromePrefServiceWebKitPrefs : public ChromeRenderViewHostTestHarness {
  protected:
   virtual void SetUp() {
diff --git a/chrome/browser/prefs/pref_metrics_service.cc b/chrome/browser/prefs/pref_metrics_service.cc
index 6bbfa4a..7c3f920 100644
--- a/chrome/browser/prefs/pref_metrics_service.cc
+++ b/chrome/browser/prefs/pref_metrics_service.cc
@@ -92,8 +92,6 @@ PrefMetricsService::PrefMetricsService(Profile* profile)
 
   RegisterSyncedPrefObservers();
 
-  MarkNeedsEmptyValueForTrackedPreferences();
-
   // The following code might cause callbacks into this instance before we exit
   // the constructor. This instance should be initialized at this point.
 #if defined(OS_WIN) || defined(OS_MACOSX)
@@ -122,7 +120,6 @@ PrefMetricsService::PrefMetricsService(Profile* profile,
       tracked_pref_path_count_(tracked_pref_path_count),
       checked_tracked_prefs_(false),
       weak_factory_(this) {
-  MarkNeedsEmptyValueForTrackedPreferences();
   CheckTrackedPreferences();
 }
 
@@ -273,19 +270,6 @@ void PrefMetricsService::GetDeviceIdCallback(const std::string& device_id) {
   CheckTrackedPreferences();
 }
 
-void PrefMetricsService::MarkNeedsEmptyValueForTrackedPreferences() {
-  for (int i = 0; i < tracked_pref_path_count_; ++i) {
-    // Skip prefs that haven't been registered.
-    if (!prefs_->FindPreference(tracked_pref_paths_[i]))
-      continue;
-
-    // Make sure tracked prefs are saved to disk even if empty.
-    // TODO(gab): Guarantee this for all prefs at a lower level and remove this
-    // hack.
-    prefs_->MarkUserStoreNeedsEmptyValue(tracked_pref_paths_[i]);
-  }
-}
-
 // To detect changes to Preferences that happen outside of Chrome, we hash
 // selected pref values and save them in local state. CheckTrackedPreferences
 // compares the saved values to the values observed in the profile's prefs. A
diff --git a/chrome/browser/prefs/pref_metrics_service.h b/chrome/browser/prefs/pref_metrics_service.h
index c776b07..d38a006 100644
--- a/chrome/browser/prefs/pref_metrics_service.h
+++ b/chrome/browser/prefs/pref_metrics_service.h
@@ -95,10 +95,6 @@ class PrefMetricsService : public BrowserContextKeyedService {
   // Callback to receive a unique device_id.
   void GetDeviceIdCallback(const std::string& device_id);
 
-  // Marks all tracked preferences as required to save their values even if
-  // empty.
-  void MarkNeedsEmptyValueForTrackedPreferences();
-
   // Checks the tracked preferences against their last known values and reports
   // any discrepancies. This must be called after |device_id| has been set.
   void CheckTrackedPreferences();
diff --git a/components/user_prefs/pref_registry_syncable.h b/components/user_prefs/pref_registry_syncable.h
index 9134d0b..64de0a3 100644
--- a/components/user_prefs/pref_registry_syncable.h
+++ b/components/user_prefs/pref_registry_syncable.h
@@ -8,6 +8,7 @@
 #include <set>
 #include <string>
 
+#include "base/callback.h"
 #include "base/prefs/pref_registry.h"
 #include "components/user_prefs/user_prefs_export.h"