Add a CachingValueStore class, which acts as a frontend for LeveldbValueStore,

for use as an extension value store.

This class does not implement the ValueStore interface, because I want to avoid
the extra copying of Values that interface requires. This is a simple key/value
store.

I also modified LeveldbValueStore and its interface a bit to make optional some of the extra copying done on Write operations.

BUG=123366
TEST=no
TBR=akalin

Review URL: https://chromiumcodereview.appspot.com/10512024

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

11 files changed, 485 insertions, 133 deletions

diff --git a/chrome/browser/value_store/caching_value_store.cc b/chrome/browser/value_store/caching_value_store.cc
new file mode 100644
index 0000000..d0d5899
--- /dev/null
+++ b/chrome/browser/value_store/caching_value_store.cc
@@ -0,0 +1,141 @@
+// Copyright (c) 2012 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.
+
+#include "chrome/browser/value_store/caching_value_store.h"
+
+#include "chrome/browser/value_store/leveldb_value_store.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
+
+class CachingValueStore::Backend : public base::RefCountedThreadSafe<Backend> {
+ public:
+  typedef base::Callback<void(scoped_ptr<base::DictionaryValue>)> ReadyCallback;
+  Backend() : storage_(NULL) {}
+
+  void Init(const FilePath& db_path, ReadyCallback callback) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CachingValueStore::Backend::InitOnFileThread,
+                   this, db_path, callback));
+  }
+
+  void Set(const std::string& key, scoped_ptr<base::Value> value) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    // We've already checked that the value has changed, so don't bother
+    // checking again. Also, skip changes to avoid wasted copies.
+    storage_->Set(ValueStore::IGNORE_QUOTA |
+                      ValueStore::NO_GENERATE_CHANGES |
+                      ValueStore::NO_CHECK_OLD_VALUE,
+                  key, *value.get());
+  }
+
+  void Remove(const std::string& key) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    storage_->Remove(key);
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<Backend>;
+
+  virtual ~Backend() {
+    if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+      delete storage_;
+    } else {
+      BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, storage_);
+    }
+  }
+
+  void InitOnFileThread(const FilePath& db_path, ReadyCallback callback) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    DCHECK(!storage_);
+    storage_ = LeveldbValueStore::Create(db_path);
+
+    ValueStore::ReadResult result = storage_->Get();
+    if (result->HasError()) {
+      callback.Run(scoped_ptr<base::DictionaryValue>(NULL));
+    } else {
+      callback.Run(result->settings().Pass());
+    }
+  }
+
+  // The actual ValueStore that handles persisting the data to disk. Used
+  // exclusively on the FILE thread.
+  LeveldbValueStore* storage_;
+
+  DISALLOW_COPY_AND_ASSIGN(Backend);
+};
+
+CachingValueStore::CachingValueStore(const FilePath& db_path)
+    : backend_(new Backend()) {
+  backend_->Init(db_path,
+                 base::Bind(&CachingValueStore::OnBackendReady, AsWeakPtr()));
+}
+
+CachingValueStore::~CachingValueStore() {
+}
+
+bool CachingValueStore::Get(const std::string& key,
+                            const base::Value** result) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsInitialized());
+
+  base::Value* value = NULL;
+  if (cache_->GetWithoutPathExpansion(key, &value)) {
+    *result = value;
+    return true;
+  }
+  return false;
+}
+
+void CachingValueStore::Set(const std::string& key, base::Value* value) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsInitialized());
+
+  scoped_ptr<base::Value> new_value(value);
+  base::Value* old_value = NULL;
+  if (!cache_->GetWithoutPathExpansion(key, &old_value) ||
+      !value->Equals(old_value)) {
+    cache_->SetWithoutPathExpansion(key, new_value.release());
+
+    scoped_ptr<base::Value> passed_value(value->DeepCopy());
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CachingValueStore::Backend::Set,
+                   backend_, key, base::Passed(passed_value.Pass())));
+  }
+}
+
+void CachingValueStore::Remove(const std::string& key) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsInitialized());
+
+  if (cache_->RemoveWithoutPathExpansion(key, NULL)) {
+    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CachingValueStore::Backend::Remove,
+                   backend_, key));
+  }
+}
+
+bool CachingValueStore::IsInitialized() {
+  DCHECK(CalledOnValidThread());
+  return !!cache_.get();
+}
+
+void CachingValueStore::AddObserver(Observer* observer) {
+  DCHECK(CalledOnValidThread());
+  observers_.AddObserver(observer);
+}
+
+void CachingValueStore::RemoveObserver(Observer* observer) {
+  DCHECK(CalledOnValidThread());
+  observers_.RemoveObserver(observer);
+}
+
+void CachingValueStore::OnBackendReady(
+    scoped_ptr<base::DictionaryValue> values) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!cache_.get());
+  cache_.swap(values);
+
+  FOR_EACH_OBSERVER(Observer, observers_, OnInitializationComplete());
+}
diff --git a/chrome/browser/value_store/caching_value_store.h b/chrome/browser/value_store/caching_value_store.h
new file mode 100644
index 0000000..8b3662b4
--- /dev/null
+++ b/chrome/browser/value_store/caching_value_store.h
@@ -0,0 +1,74 @@
+// Copyright (c) 2012 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.
+
+#ifndef CHROME_BROWSER_VALUE_STORE_CACHING_VALUE_STORE_H_
+#define CHROME_BROWSER_VALUE_STORE_CACHING_VALUE_STORE_H_
+#pragma once
+
+#include <string>
+
+#include "base/file_path.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/values.h"
+
+// Value store area with caching, backed by a LeveldbValueStore on the FILE
+// thread. The key/value store is flat with no path expansion, meaning "foo"
+// and "foo.bar" are completely separate keys.
+class CachingValueStore
+    : public base::SupportsWeakPtr<CachingValueStore>,
+      public base::NonThreadSafe {
+ public:
+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Notification that the Store is ready to use.
+    virtual void OnInitializationComplete() = 0;
+  };
+
+  explicit CachingValueStore(const FilePath& db_path);
+  ~CachingValueStore();
+
+  // Retrieves a value from the cache, returning true if value with the given
+  // key exists. The returned value is a reference to the value in the cache,
+  // i.e. no copies are made.
+  bool Get(const std::string& key, const base::Value** result);
+
+  // Sets a value with the given key. Ownership of |value| is transferred to
+  // the store.
+  void Set(const std::string& key, base::Value* value);
+
+  // Removes the value with the given key.
+  void Remove(const std::string& key);
+
+  // Returns true if the store has finished initializing.
+  bool IsInitialized();
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+ private:
+  class Backend;
+
+  // Called when our backend finishes reading the database.
+  void OnBackendReady(scoped_ptr<base::DictionaryValue> values);
+
+  // A cache of the value store. This is always up-to-date, with changes
+  // persisted to disk as they are made.
+  scoped_ptr<base::DictionaryValue> cache_;
+
+  // A helper class to manage lifetime of the backing ValueStore, which lives
+  // on the FILE thread.
+  scoped_refptr<Backend> backend_;
+
+  ObserverList<Observer, true> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(CachingValueStore);
+};
+
+#endif  // CHROME_BROWSER_VALUE_STORE_CACHING_VALUE_STORE_H_
diff --git a/chrome/browser/value_store/caching_value_store_unittest.cc b/chrome/browser/value_store/caching_value_store_unittest.cc
new file mode 100644
index 0000000..eaa8093
--- /dev/null
+++ b/chrome/browser/value_store/caching_value_store_unittest.cc
@@ -0,0 +1,113 @@
+// Copyright (c) 2012 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.
+
+#include "base/file_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/path_service.h"
+#include "base/scoped_temp_dir.h"
+#include "chrome/browser/value_store/caching_value_store.h"
+#include "chrome/common/chrome_paths.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::BrowserThread;
+
+// Test suite for CachingValueStore, using a test database with a few simple
+// entries.
+class CachingValueStoreTest
+    : public testing::Test,
+      public CachingValueStore::Observer {
+ public:
+  CachingValueStoreTest()
+      : ui_thread_(BrowserThread::UI, MessageLoop::current()),
+        file_thread_(BrowserThread::FILE, MessageLoop::current()) {
+  }
+
+  virtual void SetUp() {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+
+    FilePath test_data_dir;
+    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir));
+    FilePath src_db(test_data_dir.AppendASCII("value_store_db"));
+    db_path_ = temp_dir_.path().AppendASCII("temp_db");
+    file_util::CopyDirectory(src_db, db_path_, true);
+
+    ResetStorage();
+  }
+
+  virtual void TearDown() {
+    MessageLoop::current()->RunAllPending();  // wait for storage to delete
+    storage_->RemoveObserver(this);
+    storage_.reset();
+  }
+
+  // CachingValueStore::Observer
+  virtual void OnInitializationComplete() {
+    MessageLoop::current()->Quit();
+  }
+
+  // Reset the value store, reloading the DB from disk.
+  void ResetStorage() {
+    if (storage_.get())
+      storage_->RemoveObserver(this);
+    storage_.reset(new CachingValueStore(db_path_));
+    storage_->AddObserver(this);
+    MessageLoop::current()->Run();  // wait for OnInitializationComplete
+  }
+
+ protected:
+  scoped_ptr<CachingValueStore> storage_;
+  ScopedTempDir temp_dir_;
+  FilePath db_path_;
+  MessageLoop message_loop_;
+  content::TestBrowserThread ui_thread_;
+  content::TestBrowserThread file_thread_;
+};
+
+TEST_F(CachingValueStoreTest, GetExistingData) {
+  const base::Value* value = NULL;
+  ASSERT_FALSE(storage_->Get("key0", &value));
+
+  // Test existing keys in the DB.
+  {
+    ASSERT_TRUE(storage_->Get("key1", &value));
+    std::string result;
+    ASSERT_TRUE(value->GetAsString(&result));
+    EXPECT_EQ("value1", result);
+  }
+
+  {
+    ASSERT_TRUE(storage_->Get("key2", &value));
+    int result;
+    ASSERT_TRUE(value->GetAsInteger(&result));
+    EXPECT_EQ(2, result);
+  }
+}
+
+TEST_F(CachingValueStoreTest, ChangesPersistAfterReload) {
+  storage_->Set("key0", base::Value::CreateIntegerValue(0));
+  storage_->Set("key1", base::Value::CreateStringValue("new1"));
+  storage_->Remove("key2");
+
+  // Reload the DB and test our changes.
+  ResetStorage();
+
+  const base::Value* value = NULL;
+  {
+    ASSERT_TRUE(storage_->Get("key0", &value));
+    int result;
+    ASSERT_TRUE(value->GetAsInteger(&result));
+    EXPECT_EQ(0, result);
+  }
+
+  {
+    ASSERT_TRUE(storage_->Get("key1", &value));
+    std::string result;
+    ASSERT_TRUE(value->GetAsString(&result));
+    EXPECT_EQ("new1", result);
+  }
+
+  ASSERT_FALSE(storage_->Get("key2", &value));
+}
diff --git a/chrome/browser/value_store/failing_value_store.cc b/chrome/browser/value_store/failing_value_store.cc
index 332ceeb..8b6826a 100644
--- a/chrome/browser/value_store/failing_value_store.cc
+++ b/chrome/browser/value_store/failing_value_store.cc
@@ -11,11 +11,11 @@ namespace {
 const char* kGenericErrorMessage = "Failed to initialize settings";
 
 ValueStore::ReadResult ReadResultError() {
-  return ValueStore::ReadResult(kGenericErrorMessage);
+  return ValueStore::MakeReadResult(kGenericErrorMessage);
 }
 
 ValueStore::WriteResult WriteResultError() {
-  return ValueStore::WriteResult(kGenericErrorMessage);
+  return ValueStore::MakeWriteResult(kGenericErrorMessage);
 }
 
 }  // namespace
diff --git a/chrome/browser/value_store/leveldb_value_store.cc b/chrome/browser/value_store/leveldb_value_store.cc
index 1e0f25d..ad78e05 100644
--- a/chrome/browser/value_store/leveldb_value_store.cc
+++ b/chrome/browser/value_store/leveldb_value_store.cc
@@ -82,7 +82,7 @@ LeveldbValueStore::~LeveldbValueStore() {
     // Close |db_| now to release any lock on the directory.
     db_.reset();
     if (!file_util::Delete(db_path_, true)) {
-      LOG(WARNING) << "Failed to delete extension settings directory " <<
+      LOG(WARNING) << "Failed to delete LeveldbValueStore database " <<
           db_path_.value();
     }
   }
@@ -112,13 +112,13 @@ ValueStore::ReadResult LeveldbValueStore::Get(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<Value> setting;
   if (!ReadFromDb(leveldb::ReadOptions(), key, &setting)) {
-    return ReadResult(kGenericOnFailureMessage);
+    return MakeReadResult(kGenericOnFailureMessage);
   }
   DictionaryValue* settings = new DictionaryValue();
   if (setting.get()) {
     settings->SetWithoutPathExpansion(key, setting.release());
   }
-  return ReadResult(settings);
+  return MakeReadResult(settings);
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get(
@@ -135,14 +135,14 @@ ValueStore::ReadResult LeveldbValueStore::Get(
       it != keys.end(); ++it) {
     scoped_ptr<Value> setting;
     if (!ReadFromDb(options, *it, &setting)) {
-      return ReadResult(kGenericOnFailureMessage);
+      return MakeReadResult(kGenericOnFailureMessage);
     }
     if (setting.get()) {
       settings->SetWithoutPathExpansion(*it, setting.release());
     }
   }
 
-  return ReadResult(settings.release());
+  return MakeReadResult(settings.release());
 }
 
 ValueStore::ReadResult LeveldbValueStore::Get() {
@@ -168,50 +168,35 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
 
   if (!it->status().ok()) {
     LOG(ERROR) << "DB iteration failed: " << it->status().ToString();
-    return ReadResult(kGenericOnFailureMessage);
+    return MakeReadResult(kGenericOnFailureMessage);
   }
 
-  return ReadResult(settings.release());
+  return MakeReadResult(settings.release());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const std::string& key, const Value& value) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  DictionaryValue settings;
-  settings.SetWithoutPathExpansion(key, value.DeepCopy());
-  return Set(options, settings);
+
+  leveldb::WriteBatch batch;
+  scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
+  AddToBatch(options, key, value, &batch, changes.get());
+  return WriteToDb(&batch, changes.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Set(
     WriteOptions options, const DictionaryValue& settings) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  std::string value_as_json;
   leveldb::WriteBatch batch;
   scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
 
   for (DictionaryValue::Iterator it(settings); it.HasNext(); it.Advance()) {
-    scoped_ptr<Value> old_value;
-    if (!ReadFromDb(leveldb::ReadOptions(), it.key(), &old_value)) {
-      return WriteResult(kGenericOnFailureMessage);
-    }
-
-    if (!old_value.get() || !old_value->Equals(&it.value())) {
-      changes->push_back(
-          ValueStoreChange(
-              it.key(), old_value.release(), it.value().DeepCopy()));
-      base::JSONWriter::Write(&it.value(), &value_as_json);
-      batch.Put(it.key(), value_as_json);
-    }
-  }
-
-  leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
-  if (!status.ok()) {
-    LOG(WARNING) << "DB batch write failed: " << status.ToString();
-    return WriteResult(kGenericOnFailureMessage);
+    if (!AddToBatch(options, it.key(), it.value(), &batch, changes.get()))
+      return MakeWriteResult(kGenericOnFailureMessage);
   }
 
-  return WriteResult(changes.release());
+  return WriteToDb(&batch, changes.Pass());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Remove(
@@ -227,14 +212,13 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   leveldb::WriteBatch batch;
-  scoped_ptr<ValueStoreChangeList> changes(
-      new ValueStoreChangeList());
+  scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
 
   for (std::vector<std::string>::const_iterator it = keys.begin();
       it != keys.end(); ++it) {
     scoped_ptr<Value> old_value;
     if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) {
-      return WriteResult(kGenericOnFailureMessage);
+      return MakeWriteResult(kGenericOnFailureMessage);
     }
 
     if (old_value.get()) {
@@ -247,10 +231,10 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
   if (!status.ok() && !status.IsNotFound()) {
     LOG(WARNING) << "DB batch delete failed: " << status.ToString();
-    return WriteResult(kGenericOnFailureMessage);
+    return MakeWriteResult(kGenericOnFailureMessage);
   }
 
-  return WriteResult(changes.release());
+  return MakeWriteResult(changes.release());
 }
 
 ValueStore::WriteResult LeveldbValueStore::Clear() {
@@ -279,16 +263,16 @@ ValueStore::WriteResult LeveldbValueStore::Clear() {
 
   if (!it->status().ok()) {
     LOG(WARNING) << "Clear iteration failed: " << it->status().ToString();
-    return WriteResult(kGenericOnFailureMessage);
+    return MakeWriteResult(kGenericOnFailureMessage);
   }
 
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
   if (!status.ok() && !status.IsNotFound()) {
     LOG(WARNING) << "Clear failed: " << status.ToString();
-    return WriteResult(kGenericOnFailureMessage);
+    return MakeWriteResult(kGenericOnFailureMessage);
   }
 
-  return WriteResult(changes.release());
+  return MakeWriteResult(changes.release());
 }
 
 bool LeveldbValueStore::ReadFromDb(
@@ -321,6 +305,43 @@ bool LeveldbValueStore::ReadFromDb(
   return true;
 }
 
+bool LeveldbValueStore::AddToBatch(
+    ValueStore::WriteOptions options,
+    const std::string& key,
+    const base::Value& value,
+    leveldb::WriteBatch* batch,
+    ValueStoreChangeList* changes) {
+  scoped_ptr<Value> old_value;
+  if (!(options & NO_CHECK_OLD_VALUE)) {
+    if (!ReadFromDb(leveldb::ReadOptions(), key, &old_value))
+      return false;
+  }
+
+  if (!old_value.get() || !old_value->Equals(&value)) {
+    if (!(options & NO_GENERATE_CHANGES)) {
+      changes->push_back(
+          ValueStoreChange(key, old_value.release(), value.DeepCopy()));
+    }
+    std::string value_as_json;
+    base::JSONWriter::Write(&value, &value_as_json);
+    batch->Put(key, value_as_json);
+  }
+
+  return true;
+}
+
+ValueStore::WriteResult LeveldbValueStore::WriteToDb(
+    leveldb::WriteBatch* batch,
+    scoped_ptr<ValueStoreChangeList> changes) {
+  leveldb::Status status = db_->Write(leveldb::WriteOptions(), batch);
+  if (!status.ok()) {
+    LOG(WARNING) << "DB batch write failed: " << status.ToString();
+    return MakeWriteResult(kGenericOnFailureMessage);
+  }
+
+  return MakeWriteResult(changes.release());
+}
+
 bool LeveldbValueStore::IsEmpty() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   scoped_ptr<leveldb::Iterator> it(db_->NewIterator(leveldb::ReadOptions()));
diff --git a/chrome/browser/value_store/leveldb_value_store.h b/chrome/browser/value_store/leveldb_value_store.h
index 25713f7..cf4f7fa 100644
--- a/chrome/browser/value_store/leveldb_value_store.h
+++ b/chrome/browser/value_store/leveldb_value_store.h
@@ -55,6 +55,21 @@ class LeveldbValueStore : public ValueStore {
       // Will be reset() with the result, if any.
       scoped_ptr<Value>* setting);
 
+  // Adds a setting to a WriteBatch, and logs the change in |changes|. For
+  // use with WriteToDb.
+  bool AddToBatch(
+      ValueStore::WriteOptions options,
+      const std::string& key,
+      const base::Value& value,
+      leveldb::WriteBatch* batch,
+      ValueStoreChangeList* changes);
+
+  // Commits the changes in |batch| to the database, and returns a WriteResult
+  // with the changes.
+  WriteResult WriteToDb(
+      leveldb::WriteBatch* batch,
+      scoped_ptr<ValueStoreChangeList> changes);
+
   // Returns whether the database is empty.
   bool IsEmpty();
 
diff --git a/chrome/browser/value_store/leveldb_value_store_unittest.cc b/chrome/browser/value_store/leveldb_value_store_unittest.cc
index ac4d7ef..525d240 100644
--- a/chrome/browser/value_store/leveldb_value_store_unittest.cc
+++ b/chrome/browser/value_store/leveldb_value_store_unittest.cc
@@ -7,8 +7,6 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/value_store/leveldb_value_store.h"
 
-namespace extensions {
-
 namespace {
 
 ValueStore* Param(const FilePath& file_path) {
@@ -21,5 +19,3 @@ INSTANTIATE_TEST_CASE_P(
     LeveldbValueStore,
     ValueStoreTest,
     testing::Values(&Param));
-
-}  // namespace extensions
diff --git a/chrome/browser/value_store/testing_value_store.cc b/chrome/browser/value_store/testing_value_store.cc
index d863b3fb..337d999 100644
--- a/chrome/browser/value_store/testing_value_store.cc
+++ b/chrome/browser/value_store/testing_value_store.cc
@@ -11,11 +11,11 @@ namespace {
 const char* kGenericErrorMessage = "TestingSettingsStorage configured to error";
 
 ValueStore::ReadResult ReadResultError() {
-  return ValueStore::ReadResult(kGenericErrorMessage);
+  return ValueStore::MakeReadResult(kGenericErrorMessage);
 }
 
 ValueStore::WriteResult WriteResultError() {
-  return ValueStore::WriteResult(kGenericErrorMessage);
+  return ValueStore::MakeWriteResult(kGenericErrorMessage);
 }
 
 std::vector<std::string> CreateVector(const std::string& string) {
@@ -73,14 +73,14 @@ ValueStore::ReadResult TestingSettingsStorage::Get(
       settings->SetWithoutPathExpansion(*it, value->DeepCopy());
     }
   }
-  return ReadResult(settings);
+  return MakeReadResult(settings);
 }
 
 ValueStore::ReadResult TestingSettingsStorage::Get() {
   if (fail_all_requests_) {
     return ReadResultError();
   }
-  return ReadResult(storage_.DeepCopy());
+  return MakeReadResult(storage_.DeepCopy());
 }
 
 ValueStore::WriteResult TestingSettingsStorage::Set(
@@ -109,7 +109,7 @@ ValueStore::WriteResult TestingSettingsStorage::Set(
       storage_.SetWithoutPathExpansion(it.key(), it.value().DeepCopy());
     }
   }
-  return WriteResult(changes.release());
+  return MakeWriteResult(changes.release());
 }
 
 ValueStore::WriteResult TestingSettingsStorage::Remove(
@@ -132,7 +132,7 @@ ValueStore::WriteResult TestingSettingsStorage::Remove(
       changes->push_back(ValueStoreChange(*it, old_value, NULL));
     }
   }
-  return WriteResult(changes.release());
+  return MakeWriteResult(changes.release());
 }
 
 ValueStore::WriteResult TestingSettingsStorage::Clear() {
diff --git a/chrome/browser/value_store/value_store.cc b/chrome/browser/value_store/value_store.cc
index 4ab0430..9d7c796 100644
--- a/chrome/browser/value_store/value_store.cc
+++ b/chrome/browser/value_store/value_store.cc
@@ -6,71 +6,58 @@
 
 #include "base/logging.h"
 
-// Implementation of ReadResult.
+// Implementation of ReadResultType.
 
-ValueStore::ReadResult::ReadResult(DictionaryValue* settings)
-    : inner_(new Inner(settings, "")) {
+ValueStore::ReadResultType::ReadResultType(DictionaryValue* settings)
+    : settings_(settings) {
   DCHECK(settings);
 }
 
-ValueStore::ReadResult::ReadResult(const std::string& error)
-    : inner_(new Inner(NULL, error)) {
+ValueStore::ReadResultType::ReadResultType(const std::string& error)
+    : error_(error) {
   DCHECK(!error.empty());
 }
 
-ValueStore::ReadResult::~ReadResult() {}
+ValueStore::ReadResultType::~ReadResultType() {}
 
-bool ValueStore::ReadResult::HasError() const {
-  return !inner_->error_.empty();
+bool ValueStore::ReadResultType::HasError() const {
+  return !error_.empty();
 }
 
-const DictionaryValue& ValueStore::ReadResult::settings() const {
+scoped_ptr<DictionaryValue>& ValueStore::ReadResultType::settings() {
   DCHECK(!HasError());
-  return *inner_->settings_;
+  return settings_;
 }
 
-const std::string& ValueStore::ReadResult::error() const {
+const std::string& ValueStore::ReadResultType::error() const {
   DCHECK(HasError());
-  return inner_->error_;
+  return error_;
 }
 
-ValueStore::ReadResult::Inner::Inner(
-    DictionaryValue* settings, const std::string& error)
-    : settings_(settings), error_(error) {}
+// Implementation of WriteResultType.
 
-ValueStore::ReadResult::Inner::~Inner() {}
-
-// Implementation of WriteResult.
-
-ValueStore::WriteResult::WriteResult(
-    ValueStoreChangeList* changes) : inner_(new Inner(changes, "")) {
+ValueStore::WriteResultType::WriteResultType(ValueStoreChangeList* changes)
+    : changes_(changes) {
   DCHECK(changes);
 }
 
-ValueStore::WriteResult::WriteResult(const std::string& error)
-    : inner_(new Inner(NULL, error)) {
+ValueStore::WriteResultType::WriteResultType(const std::string& error)
+    : error_(error) {
   DCHECK(!error.empty());
 }
 
-ValueStore::WriteResult::~WriteResult() {}
+ValueStore::WriteResultType::~WriteResultType() {}
 
-bool ValueStore::WriteResult::HasError() const {
-  return !inner_->error_.empty();
+bool ValueStore::WriteResultType::HasError() const {
+  return !error_.empty();
 }
 
-const ValueStoreChangeList&
-ValueStore::WriteResult::changes() const {
+const ValueStoreChangeList& ValueStore::WriteResultType::changes() const {
   DCHECK(!HasError());
-  return *inner_->changes_;
+  return *changes_;
 }
 
-const std::string& ValueStore::WriteResult::error() const {
+const std::string& ValueStore::WriteResultType::error() const {
   DCHECK(HasError());
-  return inner_->error_;
+  return error_;
 }
-
-ValueStore::WriteResult::Inner::Inner(
-    ValueStoreChangeList* changes, const std::string& error)
-    : changes_(changes), error_(error) {}
-
-ValueStore::WriteResult::Inner::~Inner() {}
diff --git a/chrome/browser/value_store/value_store.h b/chrome/browser/value_store/value_store.h
index 56fb40d..cdc2da4 100644
--- a/chrome/browser/value_store/value_store.h
+++ b/chrome/browser/value_store/value_store.h
@@ -19,19 +19,19 @@
 // destruction.
 class ValueStore {
  public:
-  // The result of a read operation (Get). Safe/efficient to copy.
-  class ReadResult {
+  // The result of a read operation (Get).
+  class ReadResultType {
    public:
     // Ownership of |settings| taken.
-    explicit ReadResult(DictionaryValue* settings);
-    explicit ReadResult(const std::string& error);
-    ~ReadResult();
+    explicit ReadResultType(DictionaryValue* settings);
+    explicit ReadResultType(const std::string& error);
+    ~ReadResultType();
 
     // Gets the settings read from the storage. Note that this represents
     // the root object. If you request the value for key "foo", that value will
     // be in |settings.foo|.
     // Must only be called if HasError() is false.
-    const DictionaryValue& settings() const;
+    scoped_ptr<DictionaryValue>& settings();
 
     // Gets whether the operation failed.
     bool HasError() const;
@@ -41,27 +41,20 @@ class ValueStore {
     const std::string& error() const;
 
    private:
-    class Inner : public base::RefCountedThreadSafe<Inner> {
-     public:
-      Inner(DictionaryValue* settings, const std::string& error);
-      const scoped_ptr<DictionaryValue> settings_;
-      const std::string error_;
-
-     private:
-      friend class base::RefCountedThreadSafe<Inner>;
-      virtual ~Inner();
-    };
-
-    scoped_refptr<Inner> inner_;
+    scoped_ptr<DictionaryValue> settings_;
+    const std::string error_;
+
+    DISALLOW_COPY_AND_ASSIGN(ReadResultType);
   };
+  typedef scoped_ptr<ReadResultType> ReadResult;
 
-  // The result of a write operation (Set/Remove/Clear). Safe/efficient to copy.
-  class WriteResult {
+  // The result of a write operation (Set/Remove/Clear).
+  class WriteResultType {
    public:
     // Ownership of |changes| taken.
-    explicit WriteResult(ValueStoreChangeList* changes);
-    explicit WriteResult(const std::string& error);
-    ~WriteResult();
+    explicit WriteResultType(ValueStoreChangeList* changes);
+    explicit WriteResultType(const std::string& error);
+    ~WriteResultType();
 
     // Gets the list of changes to the settings which resulted from the write.
     // Must only be called if HasError() is false.
@@ -75,31 +68,43 @@ class ValueStore {
     const std::string& error() const;
 
    private:
-    class Inner : public base::RefCountedThreadSafe<Inner> {
-     public:
-      Inner(ValueStoreChangeList* changes, const std::string& error);
-      const scoped_ptr<ValueStoreChangeList> changes_;
-      const std::string error_;
-
-     private:
-      friend class base::RefCountedThreadSafe<Inner>;
-      virtual ~Inner();
-    };
-
-    scoped_refptr<Inner> inner_;
+    const scoped_ptr<ValueStoreChangeList> changes_;
+    const std::string error_;
+
+    DISALLOW_COPY_AND_ASSIGN(WriteResultType);
   };
+  typedef scoped_ptr<WriteResultType> WriteResult;
 
   // Options for write operations.
-  enum WriteOptions {
+  enum WriteOptionsValues {
     // Callers should usually use this.
-    DEFAULTS,
+    DEFAULTS = 0,
 
     // Ignore any quota restrictions.
-    IGNORE_QUOTA,
+    IGNORE_QUOTA = 1<<1,
+
+    // Don't generate the changes for a WriteResult.
+    NO_GENERATE_CHANGES = 1<<2,
+
+    // Don't check the old value before writing a new value. This will also
+    // result in an empty |old_value| in the WriteResult::changes list.
+    NO_CHECK_OLD_VALUE = 1<<3
   };
+  typedef int WriteOptions;
 
   virtual ~ValueStore() {}
 
+  // Helpers for making a Read/WriteResult.
+  template<typename T>
+  static ReadResult MakeReadResult(T arg) {
+    return ReadResult(new ReadResultType(arg));
+  }
+
+  template<typename T>
+  static WriteResult MakeWriteResult(T arg) {
+    return WriteResult(new WriteResultType(arg));
+  }
+
   // Gets the amount of space being used by a single value, in bytes.
   // Note: The GetBytesInUse methods are only used by extension settings at the
   // moment. If these become more generally useful, the
diff --git a/chrome/browser/value_store/value_store_unittest.cc b/chrome/browser/value_store/value_store_unittest.cc
index b719052..980a557 100644
--- a/chrome/browser/value_store/value_store_unittest.cc
+++ b/chrome/browser/value_store/value_store_unittest.cc
@@ -68,14 +68,14 @@ bool ValuesEqual(
 testing::AssertionResult SettingsEq(
     const char* _1, const char* _2,
     const DictionaryValue& expected,
-    const ValueStore::ReadResult& actual_result) {
-  if (actual_result.HasError()) {
+    ValueStore::ReadResult actual_result) {
+  if (actual_result->HasError()) {
     return testing::AssertionFailure() <<
-        "Result has error: " << actual_result.error();
+        "Result has error: " << actual_result->error();
   }
 
   std::string error;
-  if (!ValuesEqual(&expected, &actual_result.settings(), &error)) {
+  if (!ValuesEqual(&expected, actual_result->settings().get(), &error)) {
     return testing::AssertionFailure() << error;
   }
 
@@ -87,13 +87,13 @@ testing::AssertionResult SettingsEq(
 testing::AssertionResult ChangesEq(
     const char* _1, const char* _2,
     const ValueStoreChangeList& expected,
-    const ValueStore::WriteResult& actual_result) {
-  if (actual_result.HasError()) {
+    ValueStore::WriteResult actual_result) {
+  if (actual_result->HasError()) {
     return testing::AssertionFailure() <<
-        "Result has error: " << actual_result.error();
+        "Result has error: " << actual_result->error();
   }
 
-  const ValueStoreChangeList& actual = actual_result.changes();
+  const ValueStoreChangeList& actual = actual_result->changes();
   if (expected.size() != actual.size()) {
     return testing::AssertionFailure() <<
         "Actual has wrong size, expecting " << expected.size() <<