Skip to content

GitLab

T
tangled
Commit c571dd4b authored by kalman@chromium.org's avatar kalman@chromium.org
Browse files
Options

Propagate more information about ValueStore errors to callers, notably an 

error code in addition to the error message. This turned out to be a
surprisingly invasive yak shave and is accompanied by a few very small fixes
to inefficient and overly complex code I noticed along the way.

This is the first stage in making the chrome.storage extension/app API able to
gracefully recover from leveldb corruption.

BUG=261623
R=mpcomplete@chromium.org
TBR=tim@chromium.org
NOTRY=true

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@222178 0039d316-1c4b-4281-b951-d872f2087c98
parent ddfb7dce
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 467 additions and 444 deletions
chrome/browser/extensions/api/storage/policy_value_store.cc
View file @ c571dd4b
...@@ -10,18 +10,20 @@ ...@@ -10,18 +10,20 @@
#include "chrome/browser/policy/policy_map.h" #include "chrome/browser/policy/policy_map.h"
#include "chrome/browser/policy/policy_types.h" #include "chrome/browser/policy/policy_types.h"
#include "chrome/browser/value_store/value_store_change.h" #include "chrome/browser/value_store/value_store_change.h"
#include "chrome/browser/value_store/value_store_util.h"
#include "content/public/browser/browser_thread.h" #include "content/public/browser/browser_thread.h"
using content::BrowserThread; using content::BrowserThread;
namespace util = value_store_util;
namespace extensions { namespace extensions {
namespace { namespace {
const char kReadOnlyStoreErrorMessage[] = "This is a read-only store."; scoped_ptr<ValueStore::Error> ReadOnlyError(scoped_ptr<std::string> key) {
return make_scoped_ptr(new ValueStore::Error(
ValueStore::WriteResult WriteResultError() { ValueStore::READ_ONLY, "This is a read-only store.", key.Pass()));
return ValueStore::MakeWriteResult(kReadOnlyStoreErrorMessage);
} }
} // namespace } // namespace
...@@ -58,11 +60,11 @@ void PolicyValueStore::SetCurrentPolicy(const policy::PolicyMap& policy, ...@@ -58,11 +60,11 @@ void PolicyValueStore::SetCurrentPolicy(const policy::PolicyMap& policy,
ValueStore::ReadResult read_result = delegate_->Get(); ValueStore::ReadResult read_result = delegate_->Get();
if (read_result->HasError()) { if (read_result->HasError()) {
LOG(WARNING) << "Failed to read managed settings for extension " LOG(WARNING) << "Failed to read managed settings for extension "
<< extension_id_ << ": " << read_result->error(); << extension_id_ << ": " << read_result->error().message;
// Leave |previous_policy| empty, so that events are generated for every // Leave |previous_policy| empty, so that events are generated for every
// policy in |current_policy|. // policy in |current_policy|.
} else { } else {
read_result->settings()->Swap(&previous_policy); read_result->settings().Swap(&previous_policy);
} }
// Now get two lists of changes: changes after setting the current policies, // Now get two lists of changes: changes after setting the current policies,
...@@ -139,25 +141,25 @@ ValueStore::ReadResult PolicyValueStore::Get() { ...@@ -139,25 +141,25 @@ ValueStore::ReadResult PolicyValueStore::Get() {
ValueStore::WriteResult PolicyValueStore::Set( ValueStore::WriteResult PolicyValueStore::Set(
WriteOptions options, const std::string& key, const base::Value& value) { WriteOptions options, const std::string& key, const base::Value& value) {
return WriteResultError(); return MakeWriteResult(ReadOnlyError(util::NewKey(key)));
} }
ValueStore::WriteResult PolicyValueStore::Set( ValueStore::WriteResult PolicyValueStore::Set(
WriteOptions options, const base::DictionaryValue& settings) { WriteOptions options, const base::DictionaryValue& settings) {
return WriteResultError(); return MakeWriteResult(ReadOnlyError(util::NoKey()));
} }
ValueStore::WriteResult PolicyValueStore::Remove(const std::string& key) { ValueStore::WriteResult PolicyValueStore::Remove(const std::string& key) {
return WriteResultError(); return MakeWriteResult(ReadOnlyError(util::NewKey(key)));
} }
ValueStore::WriteResult PolicyValueStore::Remove( ValueStore::WriteResult PolicyValueStore::Remove(
const std::vector<std::string>& keys) { const std::vector<std::string>& keys) {
return WriteResultError(); return MakeWriteResult(ReadOnlyError(util::NoKey()));
} }
ValueStore::WriteResult PolicyValueStore::Clear() { ValueStore::WriteResult PolicyValueStore::Clear() {
return WriteResultError(); return MakeWriteResult(ReadOnlyError(util::NoKey()));
} }
} // namespace extensions } // namespace extensions
chrome/browser/extensions/api/storage/policy_value_store_unittest.cc
View file @ c571dd4b
...@@ -128,10 +128,10 @@ TEST_F(PolicyValueStoreTest, DontProvideRecommendedPolicies) { ...@@ -128,10 +128,10 @@ TEST_F(PolicyValueStoreTest, DontProvideRecommendedPolicies) {
store_->SetCurrentPolicy(policies, false); store_->SetCurrentPolicy(policies, false);
ValueStore::ReadResult result = store_->Get(); ValueStore::ReadResult result = store_->Get();
ASSERT_FALSE(result->HasError()); ASSERT_FALSE(result->HasError());
EXPECT_EQ(1u, result->settings()->size()); EXPECT_EQ(1u, result->settings().size());
base::Value* value = NULL; base::Value* value = NULL;
EXPECT_FALSE(result->settings()->Get("may", &value)); EXPECT_FALSE(result->settings().Get("may", &value));
EXPECT_TRUE(result->settings()->Get("must", &value)); EXPECT_TRUE(result->settings().Get("must", &value));
EXPECT_TRUE(base::Value::Equals(&expected, value)); EXPECT_TRUE(base::Value::Equals(&expected, value));
} }
......
chrome/browser/extensions/api/storage/settings_backend.cc
View file @ c571dd4b
...@@ -160,11 +160,10 @@ syncer::SyncDataList SettingsBackend::GetAllSyncData( ...@@ -160,11 +160,10 @@ syncer::SyncDataList SettingsBackend::GetAllSyncData(
ValueStore::ReadResult maybe_settings = GetStorage(*it)->Get(); ValueStore::ReadResult maybe_settings = GetStorage(*it)->Get();
if (maybe_settings->HasError()) { if (maybe_settings->HasError()) {
LOG(WARNING) << "Failed to get settings for " << *it << ": " << LOG(WARNING) << "Failed to get settings for " << *it << ": " <<
maybe_settings->error(); maybe_settings->error().message;
continue; continue;
} }
AddAllSyncData(*it, *maybe_settings->settings().get(), AddAllSyncData(*it, maybe_settings->settings(), type, &all_sync_data);
type, &all_sync_data);
} }
return all_sync_data; return all_sync_data;
......
chrome/browser/extensions/api/storage/settings_frontend_unittest.cc
View file @ c571dd4b
...@@ -111,7 +111,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) { ...@@ -111,7 +111,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) {
{ {
ValueStore::ReadResult result = storage->Get(); ValueStore::ReadResult result = storage->Get();
ASSERT_FALSE(result->HasError()); ASSERT_FALSE(result->HasError());
EXPECT_FALSE(result->settings()->empty()); EXPECT_FALSE(result->settings().empty());
} }
ResetFrontend(); ResetFrontend();
...@@ -120,7 +120,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) { ...@@ -120,7 +120,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) {
{ {
ValueStore::ReadResult result = storage->Get(); ValueStore::ReadResult result = storage->Get();
ASSERT_FALSE(result->HasError()); ASSERT_FALSE(result->HasError());
EXPECT_FALSE(result->settings()->empty()); EXPECT_FALSE(result->settings().empty());
} }
} }
...@@ -148,7 +148,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsClearedOnUninstall) { ...@@ -148,7 +148,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsClearedOnUninstall) {
{ {
ValueStore::ReadResult result = storage->Get(); ValueStore::ReadResult result = storage->Get();
ASSERT_FALSE(result->HasError()); ASSERT_FALSE(result->HasError());
EXPECT_TRUE(result->settings()->empty()); EXPECT_TRUE(result->settings().empty());
} }
} }
......
chrome/browser/extensions/api/storage/settings_quota_unittest.cc
View file @ c571dd4b
...@@ -62,8 +62,8 @@ class ExtensionSettingsQuotaTest : public testing::Test { ...@@ -62,8 +62,8 @@ class ExtensionSettingsQuotaTest : public testing::Test {
// Returns whether the settings in |storage_| and |delegate_| are the same as // Returns whether the settings in |storage_| and |delegate_| are the same as
// |settings|. // |settings|.
bool SettingsEqual(const DictionaryValue& settings) { bool SettingsEqual(const DictionaryValue& settings) {
return settings.Equals(storage_->Get()->settings().get()) && return settings.Equals(&storage_->Get()->settings()) &&
settings.Equals(delegate_->Get()->settings().get()); settings.Equals(&delegate_->Get()->settings());
} }
// Values with different serialized sizes. // Values with different serialized sizes.
......
chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
View file @ c571dd4b
...@@ -9,15 +9,16 @@ ...@@ -9,15 +9,16 @@
#include "base/memory/scoped_ptr.h" #include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h" #include "base/message_loop/message_loop.h"
#include "base/metrics/histogram.h" #include "base/metrics/histogram.h"
#include "base/strings/stringprintf.h"
#include "chrome/browser/value_store/value_store_util.h"
#include "chrome/common/extensions/api/extension_api.h" #include "chrome/common/extensions/api/extension_api.h"
#include "extensions/common/error_utils.h"
namespace util = value_store_util;
namespace extensions { namespace extensions {
namespace { namespace {
const char* kQuotaExceededError = "* quota exceeded.";
// Resources there are a quota for. // Resources there are a quota for.
enum Resource { enum Resource {
QUOTA_BYTES, QUOTA_BYTES,
...@@ -53,9 +54,10 @@ void Free( ...@@ -53,9 +54,10 @@ void Free(
used_per_setting->erase(key); used_per_setting->erase(key);
} }
// Returns an error result and logs the quota exceeded to UMA. scoped_ptr<ValueStore::Error> QuotaExceededError(Resource resource,
ValueStore::WriteResult QuotaExceededFor(Resource resource) { scoped_ptr<std::string> key) {
std::string name; const char* name = NULL;
// TODO(kalman): These hisograms are both silly and untracked. Fix.
switch (resource) { switch (resource) {
case QUOTA_BYTES: case QUOTA_BYTES:
name = "QUOTA_BYTES"; name = "QUOTA_BYTES";
...@@ -72,11 +74,12 @@ ValueStore::WriteResult QuotaExceededFor(Resource resource) { ...@@ -72,11 +74,12 @@ ValueStore::WriteResult QuotaExceededFor(Resource resource) {
UMA_HISTOGRAM_COUNTS_100( UMA_HISTOGRAM_COUNTS_100(
"Extensions.SettingsQuotaExceeded.KeyCount", 1); "Extensions.SettingsQuotaExceeded.KeyCount", 1);
break; break;
default:
NOTREACHED();
} }
return ValueStore::MakeWriteResult( CHECK(name);
ErrorUtils::FormatErrorMessage(kQuotaExceededError, name)); return make_scoped_ptr(new ValueStore::Error(
ValueStore::QUOTA_EXCEEDED,
base::StringPrintf("%s quota exceeded", name),
key.Pass()));
} }
} // namespace } // namespace
...@@ -87,11 +90,11 @@ SettingsStorageQuotaEnforcer::SettingsStorageQuotaEnforcer( ...@@ -87,11 +90,11 @@ SettingsStorageQuotaEnforcer::SettingsStorageQuotaEnforcer(
ReadResult maybe_settings = delegate_->Get(); ReadResult maybe_settings = delegate_->Get();
if (maybe_settings->HasError()) { if (maybe_settings->HasError()) {
LOG(WARNING) << "Failed to get initial settings for quota: " << LOG(WARNING) << "Failed to get initial settings for quota: " <<
maybe_settings->error(); maybe_settings->error().message;
return; return;
} }
for (base::DictionaryValue::Iterator it(*maybe_settings->settings().get()); for (base::DictionaryValue::Iterator it(maybe_settings->settings());
!it.IsAtEnd(); it.Advance()) { !it.IsAtEnd(); it.Advance()) {
Allocate(it.key(), it.value(), &used_total_, &used_per_setting_); Allocate(it.key(), it.value(), &used_total_, &used_per_setting_);
} }
...@@ -143,14 +146,15 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( ...@@ -143,14 +146,15 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set(
if (!(options & IGNORE_QUOTA)) { if (!(options & IGNORE_QUOTA)) {
if (new_used_total > limits_.quota_bytes) { if (new_used_total > limits_.quota_bytes) {
return QuotaExceededFor(QUOTA_BYTES); return MakeWriteResult(
QuotaExceededError(QUOTA_BYTES, util::NewKey(key)));
} }
if (new_used_per_setting[key] > limits_.quota_bytes_per_item) { if (new_used_per_setting[key] > limits_.quota_bytes_per_item) {
return QuotaExceededFor(QUOTA_BYTES_PER_ITEM); return MakeWriteResult(
} QuotaExceededError(QUOTA_BYTES_PER_ITEM, util::NewKey(key)));
if (new_used_per_setting.size() > limits_.max_items) {
return QuotaExceededFor(MAX_ITEMS);
} }
if (new_used_per_setting.size() > limits_.max_items)
return MakeWriteResult(QuotaExceededError(MAX_ITEMS, util::NewKey(key)));
} }
WriteResult result = delegate_->Set(options, key, value); WriteResult result = delegate_->Set(options, key, value);
...@@ -173,17 +177,16 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set( ...@@ -173,17 +177,16 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Set(
if (!(options & IGNORE_QUOTA) && if (!(options & IGNORE_QUOTA) &&
new_used_per_setting[it.key()] > limits_.quota_bytes_per_item) { new_used_per_setting[it.key()] > limits_.quota_bytes_per_item) {
return QuotaExceededFor(QUOTA_BYTES_PER_ITEM); return MakeWriteResult(
QuotaExceededError(QUOTA_BYTES_PER_ITEM, util::NewKey(it.key())));
} }
} }
if (!(options & IGNORE_QUOTA)) { if (!(options & IGNORE_QUOTA)) {
if (new_used_total > limits_.quota_bytes) { if (new_used_total > limits_.quota_bytes)
return QuotaExceededFor(QUOTA_BYTES); return MakeWriteResult(QuotaExceededError(QUOTA_BYTES, util::NoKey()));
} if (new_used_per_setting.size() > limits_.max_items)
if (new_used_per_setting.size() > limits_.max_items) { return MakeWriteResult(QuotaExceededError(MAX_ITEMS, util::NoKey()));
return QuotaExceededFor(MAX_ITEMS);
}
} }
WriteResult result = delegate_->Set(options, values); WriteResult result = delegate_->Set(options, values);
......
chrome/browser/extensions/api/storage/settings_sync_unittest.cc
View file @ c571dd4b
...@@ -78,10 +78,10 @@ testing::AssertionResult SettingsEq( ...@@ -78,10 +78,10 @@ testing::AssertionResult SettingsEq(
ValueStore::ReadResult actual) { ValueStore::ReadResult actual) {
if (actual->HasError()) { if (actual->HasError()) {
return testing::AssertionFailure() << return testing::AssertionFailure() <<
"Expected: " << GetJson(expected) << "Expected: " << expected <<
", actual has error: " << actual->error(); ", actual has error: " << actual->error().message;
} }
return ValuesEq(_1, _2, &expected, actual->settings().get()); return ValuesEq(_1, _2, &expected, &actual->settings());
} }
// SyncChangeProcessor which just records the changes made, accessed after // SyncChangeProcessor which just records the changes made, accessed after
...@@ -121,7 +121,7 @@ class MockSyncChangeProcessor : public syncer::SyncChangeProcessor { ...@@ -121,7 +121,7 @@ class MockSyncChangeProcessor : public syncer::SyncChangeProcessor {
changes_.clear(); changes_.clear();
} }
void SetFailAllRequests(bool fail_all_requests) { void set_fail_all_requests(bool fail_all_requests) {
fail_all_requests_ = fail_all_requests; fail_all_requests_ = fail_all_requests;
} }
...@@ -721,7 +721,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { ...@@ -721,7 +721,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) {
ValueStore* bad = AddExtensionAndGetStorage("bad", type); ValueStore* bad = AddExtensionAndGetStorage("bad", type);
// Make bad fail for incoming sync changes. // Make bad fail for incoming sync changes.
testing_factory->GetExisting("bad")->SetFailAllRequests(true); testing_factory->GetExisting("bad")->set_error_code(ValueStore::CORRUPTION);
{ {
syncer::SyncDataList sync_data; syncer::SyncDataList sync_data;
sync_data.push_back(settings_sync_util::CreateData( sync_data.push_back(settings_sync_util::CreateData(
...@@ -735,7 +735,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { ...@@ -735,7 +735,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) {
scoped_ptr<syncer::SyncErrorFactory>( scoped_ptr<syncer::SyncErrorFactory>(
new syncer::SyncErrorFactoryMock())); new syncer::SyncErrorFactoryMock()));
} }
testing_factory->GetExisting("bad")->SetFailAllRequests(false); testing_factory->GetExisting("bad")->set_error_code(ValueStore::OK);
{ {
DictionaryValue dict; DictionaryValue dict;
...@@ -818,7 +818,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { ...@@ -818,7 +818,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) {
} }
// Failing ProcessSyncChanges shouldn't go to the storage. // Failing ProcessSyncChanges shouldn't go to the storage.
testing_factory->GetExisting("bad")->SetFailAllRequests(true); testing_factory->GetExisting("bad")->set_error_code(ValueStore::CORRUPTION);
{ {
syncer::SyncChangeList change_list; syncer::SyncChangeList change_list;
change_list.push_back(settings_sync_util::CreateUpdate( change_list.push_back(settings_sync_util::CreateUpdate(
...@@ -828,7 +828,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) { ...@@ -828,7 +828,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingStartSyncingDisablesSync) {
"bad", "foo", fooValue, model_type)); "bad", "foo", fooValue, model_type));
GetSyncableService(model_type)->ProcessSyncChanges(FROM_HERE, change_list); GetSyncableService(model_type)->ProcessSyncChanges(FROM_HERE, change_list);
} }
testing_factory->GetExisting("bad")->SetFailAllRequests(false); testing_factory->GetExisting("bad")->set_error_code(ValueStore::OK);
{ {
DictionaryValue dict; DictionaryValue dict;
...@@ -946,7 +946,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) { ...@@ -946,7 +946,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) {
} }
// Now fail ProcessSyncChanges for bad. // Now fail ProcessSyncChanges for bad.
testing_factory->GetExisting("bad")->SetFailAllRequests(true); testing_factory->GetExisting("bad")->set_error_code(ValueStore::CORRUPTION);
{ {
syncer::SyncChangeList change_list; syncer::SyncChangeList change_list;
change_list.push_back(settings_sync_util::CreateAdd( change_list.push_back(settings_sync_util::CreateAdd(
...@@ -955,7 +955,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) { ...@@ -955,7 +955,7 @@ TEST_F(ExtensionSettingsSyncTest, FailingProcessChangesDisablesSync) {
"bad", "bar", barValue, model_type)); "bad", "bar", barValue, model_type));
GetSyncableService(model_type)->ProcessSyncChanges(FROM_HERE, change_list); GetSyncableService(model_type)->ProcessSyncChanges(FROM_HERE, change_list);
} }
testing_factory->GetExisting("bad")->SetFailAllRequests(false); testing_factory->GetExisting("bad")->set_error_code(ValueStore::OK);
{ {
DictionaryValue dict; DictionaryValue dict;
...@@ -1020,14 +1020,14 @@ TEST_F(ExtensionSettingsSyncTest, FailingGetAllSyncDataDoesntStopSync) { ...@@ -1020,14 +1020,14 @@ TEST_F(ExtensionSettingsSyncTest, FailingGetAllSyncDataDoesntStopSync) {
// Even though bad will fail to get all sync data, sync data should still // Even though bad will fail to get all sync data, sync data should still
// include that from good. // include that from good.
testing_factory->GetExisting("bad")->SetFailAllRequests(true); testing_factory->GetExisting("bad")->set_error_code(ValueStore::CORRUPTION);
{ {
syncer::SyncDataList all_sync_data = syncer::SyncDataList all_sync_data =
GetSyncableService(model_type)->GetAllSyncData(model_type); GetSyncableService(model_type)->GetAllSyncData(model_type);
EXPECT_EQ(1u, all_sync_data.size()); EXPECT_EQ(1u, all_sync_data.size());
EXPECT_EQ("good/foo", all_sync_data[0].GetTag()); EXPECT_EQ("good/foo", all_sync_data[0].GetTag());
} }
testing_factory->GetExisting("bad")->SetFailAllRequests(false); testing_factory->GetExisting("bad")->set_error_code(ValueStore::OK);
// Sync shouldn't be disabled for good (nor bad -- but this is unimportant). // Sync shouldn't be disabled for good (nor bad -- but this is unimportant).
GetSyncableService(model_type)->MergeDataAndStartSyncing( GetSyncableService(model_type)->MergeDataAndStartSyncing(
...@@ -1075,13 +1075,13 @@ TEST_F(ExtensionSettingsSyncTest, FailureToReadChangesToPushDisablesSync) { ...@@ -1075,13 +1075,13 @@ TEST_F(ExtensionSettingsSyncTest, FailureToReadChangesToPushDisablesSync) {
// good will successfully push foo:fooValue to sync, but bad will fail to // good will successfully push foo:fooValue to sync, but bad will fail to
// get them so won't. // get them so won't.
testing_factory->GetExisting("bad")->SetFailAllRequests(true); testing_factory->GetExisting("bad")->set_error_code(ValueStore::CORRUPTION);
GetSyncableService(model_type)->MergeDataAndStartSyncing( GetSyncableService(model_type)->MergeDataAndStartSyncing(
model_type, model_type,
syncer::SyncDataList(), syncer::SyncDataList(),
sync_processor_delegate_.PassAs<syncer::SyncChangeProcessor>(), sync_processor_delegate_.PassAs<syncer::SyncChangeProcessor>(),
scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock())); scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock()));
testing_factory->GetExisting("bad")->SetFailAllRequests(false); testing_factory->GetExisting("bad")->set_error_code(ValueStore::OK);
EXPECT_EQ( EXPECT_EQ(
syncer::SyncChange::ACTION_ADD, syncer::SyncChange::ACTION_ADD,
...@@ -1177,13 +1177,13 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalStateDisablesSync) { ...@@ -1177,13 +1177,13 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalStateDisablesSync) {
// Only set bad; setting good will cause it to fail below. // Only set bad; setting good will cause it to fail below.
bad->Set(DEFAULTS, "foo", fooValue); bad->Set(DEFAULTS, "foo", fooValue);
sync_processor_->SetFailAllRequests(true); sync_processor_->set_fail_all_requests(true);
GetSyncableService(model_type)->MergeDataAndStartSyncing( GetSyncableService(model_type)->MergeDataAndStartSyncing(
model_type, model_type,
syncer::SyncDataList(), syncer::SyncDataList(),
sync_processor_delegate_.PassAs<syncer::SyncChangeProcessor>(), sync_processor_delegate_.PassAs<syncer::SyncChangeProcessor>(),
scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock())); scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock()));
sync_processor_->SetFailAllRequests(false); sync_processor_->set_fail_all_requests(false);
// Changes from good will be send to sync, changes from bad won't. // Changes from good will be send to sync, changes from bad won't.
sync_processor_->ClearChanges(); sync_processor_->ClearChanges();
...@@ -1273,9 +1273,9 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalChangeDisablesSync) { ...@@ -1273,9 +1273,9 @@ TEST_F(ExtensionSettingsSyncTest, FailureToPushLocalChangeDisablesSync) {
// bad will fail to send changes. // bad will fail to send changes.
good->Set(DEFAULTS, "foo", fooValue); good->Set(DEFAULTS, "foo", fooValue);
sync_processor_->SetFailAllRequests(true); sync_processor_->set_fail_all_requests(true);
bad->Set(DEFAULTS, "foo", fooValue); bad->Set(DEFAULTS, "foo", fooValue);
sync_processor_->SetFailAllRequests(false); sync_processor_->set_fail_all_requests(false);
EXPECT_EQ( EXPECT_EQ(
syncer::SyncChange::ACTION_ADD, syncer::SyncChange::ACTION_ADD,
...@@ -1421,7 +1421,7 @@ TEST_F(ExtensionSettingsSyncTest, Dots) { ...@@ -1421,7 +1421,7 @@ TEST_F(ExtensionSettingsSyncTest, Dots) {
expected_data.SetWithoutPathExpansion( expected_data.SetWithoutPathExpansion(
"key.with.dot", "key.with.dot",
new base::StringValue("value")); new base::StringValue("value"));
EXPECT_TRUE(Value::Equals(&expected_data, data->settings().get())); EXPECT_TRUE(Value::Equals(&expected_data, &data->settings()));
} }
// Test dots in keys going to sync. // Test dots in keys going to sync.
......
chrome/browser/extensions/api/storage/storage_api.cc
View file @ c571dd4b
...@@ -21,15 +21,6 @@ namespace extensions { ...@@ -21,15 +21,6 @@ namespace extensions {
using content::BrowserThread; using content::BrowserThread;
namespace {
const char kUnsupportedArgumentType[] = "Unsupported argument type";
const char kInvalidNamespaceErrorMessage[] =
"\"%s\" is not available in this instance of Chrome";
const char kManagedNamespaceDisabledErrorMessage[] =
"\"managed\" is disabled. Use \"--%s\" to enable it.";
const char kStorageErrorMessage[] = "Storage error";
} // namespace
// SettingsFunction // SettingsFunction
SettingsFunction::SettingsFunction() SettingsFunction::SettingsFunction()
...@@ -41,8 +32,8 @@ bool SettingsFunction::ShouldSkipQuotaLimiting() const { ...@@ -41,8 +32,8 @@ bool SettingsFunction::ShouldSkipQuotaLimiting() const {
// Only apply quota if this is for sync storage. // Only apply quota if this is for sync storage.
std::string settings_namespace_string; std::string settings_namespace_string;
if (!args_->GetString(0, &settings_namespace_string)) { if (!args_->GetString(0, &settings_namespace_string)) {
// This is an error but it will be caught in RunImpl(), there is no // This should be EXTENSION_FUNCTION_VALIDATE(false) but there is no way
// mechanism to signify an error from this function. // to signify that from this function. It will be caught in RunImpl().
return false; return false;
} }
return settings_namespace_string != "sync"; return settings_namespace_string != "sync";
...@@ -60,8 +51,9 @@ bool SettingsFunction::RunImpl() { ...@@ -60,8 +51,9 @@ bool SettingsFunction::RunImpl() {
SettingsFrontend* frontend = SettingsFrontend* frontend =
profile()->GetExtensionService()->settings_frontend(); profile()->GetExtensionService()->settings_frontend();
if (!frontend->IsStorageEnabled(settings_namespace_)) { if (!frontend->IsStorageEnabled(settings_namespace_)) {
error_ = base::StringPrintf(kInvalidNamespaceErrorMessage, error_ = base::StringPrintf(
settings_namespace_string.c_str()); "\"%s\" is not available in this instance of Chrome",
settings_namespace_string.c_str());
return false; return false;
} }
...@@ -81,19 +73,21 @@ void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) { ...@@ -81,19 +73,21 @@ void SettingsFunction::AsyncRunWithStorage(ValueStore* storage) {
base::Bind(&SettingsFunction::SendResponse, this, success)); base::Bind(&SettingsFunction::SendResponse, this, success));
} }
bool SettingsFunction::UseReadResult(ValueStore::ReadResult result) { bool SettingsFunction::UseReadResult(ValueStore::ReadResult read_result) {
if (result->HasError()) { if (read_result->HasError()) {
error_ = result->error(); error_ = read_result->error().message;
return false; return false;
} }
SetResult(result->settings().release()); base::DictionaryValue* result = new base::DictionaryValue();
result->Swap(&read_result->settings());
SetResult(result);
return true; return true;
} }
bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result) { bool SettingsFunction::UseWriteResult(ValueStore::WriteResult result) {
if (result->HasError()) { if (result->HasError()) {
error_ = result->error(); error_ = result->error().message;
return false; return false;
} }
...@@ -192,14 +186,14 @@ bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) { ...@@ -192,14 +186,14 @@ bool StorageStorageAreaGetFunction::RunWithStorage(ValueStore* storage) {
} }
base::DictionaryValue* with_default_values = as_dict->DeepCopy(); base::DictionaryValue* with_default_values = as_dict->DeepCopy();
with_default_values->MergeDictionary(result->settings().get()); with_default_values->MergeDictionary(&result->settings());
return UseReadResult( return UseReadResult(
ValueStore::MakeReadResult(with_default_values)); ValueStore::MakeReadResult(make_scoped_ptr(with_default_values)));
} }
default: default:
return UseReadResult( EXTENSION_FUNCTION_VALIDATE(false);
ValueStore::MakeReadResult(kUnsupportedArgumentType)); return false;
} }
} }
...@@ -231,7 +225,7 @@ bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage( ...@@ -231,7 +225,7 @@ bool StorageStorageAreaGetBytesInUseFunction::RunWithStorage(
} }
default: default:
error_ = kUnsupportedArgumentType; EXTENSION_FUNCTION_VALIDATE(false);
return false; return false;
} }
...@@ -269,8 +263,8 @@ bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) { ...@@ -269,8 +263,8 @@ bool StorageStorageAreaRemoveFunction::RunWithStorage(ValueStore* storage) {
} }
default: default:
return UseWriteResult( EXTENSION_FUNCTION_VALIDATE(false);
ValueStore::MakeWriteResult(kUnsupportedArgumentType)); return false;
}; };
} }
......
chrome/browser/extensions/api/storage/syncable_settings_storage.cc
View file @ c571dd4b
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
#include "chrome/browser/extensions/api/storage/syncable_settings_storage.h" #include "chrome/browser/extensions/api/storage/syncable_settings_storage.h"
#include "base/strings/stringprintf.h"
#include "chrome/browser/extensions/api/storage/settings_namespace.h" #include "chrome/browser/extensions/api/storage/settings_namespace.h"
#include "chrome/browser/extensions/api/storage/settings_sync_processor.h" #include "chrome/browser/extensions/api/storage/settings_sync_processor.h"
#include "chrome/browser/extensions/api/storage/settings_sync_util.h" #include "chrome/browser/extensions/api/storage/settings_sync_util.h"
...@@ -142,15 +143,15 @@ syncer::SyncError SyncableSettingsStorage::StartSyncing( ...@@ -142,15 +143,15 @@ syncer::SyncError SyncableSettingsStorage::StartSyncing(
return syncer::SyncError( return syncer::SyncError(
FROM_HERE, FROM_HERE,
syncer::SyncError::DATATYPE_ERROR, syncer::SyncError::DATATYPE_ERROR,
std::string("Failed to get settings: ") + maybe_settings->error(), base::StringPrintf("Failed to get settings: %s",
maybe_settings->error().message.c_str()),
sync_processor_->type()); sync_processor_->type());
} }
const base::DictionaryValue& settings = *maybe_settings->settings().get(); const base::DictionaryValue& settings = maybe_settings->settings();
if (sync_state.empty()) return sync_state.empty() ?
return SendLocalSettingsToSync(settings); SendLocalSettingsToSync(settings) :
else OverwriteLocalSettingsWithSync(sync_state, settings);
return OverwriteLocalSettingsWithSync(sync_state, settings);
} }
syncer::SyncError SyncableSettingsStorage::SendLocalSettingsToSync( syncer::SyncError SyncableSettingsStorage::SendLocalSettingsToSync(
...@@ -261,15 +262,14 @@ syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges( ...@@ -261,15 +262,14 @@ syncer::SyncError SyncableSettingsStorage::ProcessSyncChanges(
errors.push_back(syncer::SyncError( errors.push_back(syncer::SyncError(
FROM_HERE, FROM_HERE,
syncer::SyncError::DATATYPE_ERROR, syncer::SyncError::DATATYPE_ERROR,
std::string("Error getting current sync state for ") + base::StringPrintf("Error getting current sync state for %s/%s: %s",
extension_id_ + "/" + key + ": " + maybe_settings->error(), extension_id_.c_str(), key.c_str(),
maybe_settings->error().message.c_str()),
sync_processor_->type())); sync_processor_->type()));
continue; continue;
} }
Value* value = NULL; maybe_settings->settings().RemoveWithoutPathExpansion(key,
if (maybe_settings->settings()->GetWithoutPathExpansion(key, &value)) { &current_value);
current_value.reset(value->DeepCopy());
}
} }
syncer::SyncError error; syncer::SyncError error;
...@@ -341,8 +341,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncAdd( ...@@ -341,8 +341,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncAdd(
return syncer::SyncError( return syncer::SyncError(
FROM_HERE, FROM_HERE,
syncer::SyncError::DATATYPE_ERROR, syncer::SyncError::DATATYPE_ERROR,
std::string("Error pushing sync add to local settings: ") + base::StringPrintf("Error pushing sync add to local settings: %s",
result->error(), result->error().message.c_str()),
sync_processor_->type()); sync_processor_->type());
} }
changes->push_back(ValueStoreChange(key, NULL, new_value)); changes->push_back(ValueStoreChange(key, NULL, new_value));
...@@ -361,8 +361,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncUpdate( ...@@ -361,8 +361,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncUpdate(
return syncer::SyncError( return syncer::SyncError(
FROM_HERE, FROM_HERE,
syncer::SyncError::DATATYPE_ERROR, syncer::SyncError::DATATYPE_ERROR,
std::string("Error pushing sync update to local settings: ") + base::StringPrintf("Error pushing sync update to local settings: %s",
result->error(), result->error().message.c_str()),
sync_processor_->type()); sync_processor_->type());
} }
changes->push_back(ValueStoreChange(key, old_value, new_value)); changes->push_back(ValueStoreChange(key, old_value, new_value));
...@@ -379,8 +379,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncDelete( ...@@ -379,8 +379,8 @@ syncer::SyncError SyncableSettingsStorage::OnSyncDelete(
return syncer::SyncError( return syncer::SyncError(
FROM_HERE, FROM_HERE,
syncer::SyncError::DATATYPE_ERROR, syncer::SyncError::DATATYPE_ERROR,
std::string("Error pushing sync remove to local settings: ") + base::StringPrintf("Error pushing sync remove to local settings: %s",
result->error(), result->error().message.c_str()),
sync_processor_->type()); sync_processor_->type());
} }
changes->push_back(ValueStoreChange(key, old_value, NULL)); changes->push_back(ValueStoreChange(key, old_value, NULL));
......
chrome/browser/sync/test/integration/extension_settings_helper.cc
View file @ c571dd4b
...@@ -36,23 +36,22 @@ std::string ToJson(const Value& value) { ...@@ -36,23 +36,22 @@ std::string ToJson(const Value& value) {
return json; return json;
} }
void GetAllSettingsOnFileThread( void GetAllSettingsOnFileThread(DictionaryValue* out,
scoped_ptr<DictionaryValue>* out, base::WaitableEvent* signal,
base::WaitableEvent* signal, ValueStore* storage) {
ValueStore* storage) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); CHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
out->swap(storage->Get()->settings()); out->Swap(&storage->Get()->settings());
signal->Signal(); signal->Signal();
} }
scoped_ptr<DictionaryValue> GetAllSettings( scoped_ptr<DictionaryValue> GetAllSettings(
Profile* profile, const std::string& id) { Profile* profile, const std::string& id) {
base::WaitableEvent signal(false, false); base::WaitableEvent signal(false, false);
scoped_ptr<DictionaryValue> settings; scoped_ptr<DictionaryValue> settings(new DictionaryValue());
profile->GetExtensionService()->settings_frontend()->RunWithStorage( profile->GetExtensionService()->settings_frontend()->RunWithStorage(
id, id,
extensions::settings_namespace::SYNC, extensions::settings_namespace::SYNC,
base::Bind(&GetAllSettingsOnFileThread, &settings, &signal)); base::Bind(&GetAllSettingsOnFileThread, settings.get(), &signal));
signal.Wait(); signal.Wait();
return settings.Pass(); return settings.Pass();
} }
......
chrome/browser/value_store/leveldb_value_store.h
View file @ c571dd4b
...@@ -47,32 +47,36 @@ class LeveldbValueStore : public ValueStore { ...@@ -47,32 +47,36 @@ class LeveldbValueStore : public ValueStore {
virtual WriteResult Clear() OVERRIDE; virtual WriteResult Clear() OVERRIDE;
private: private:
// Tries to open the database if it hasn't been opened already. Returns the // Tries to open the database if it hasn't been opened already.
// error message on failure, or "" on success (guaranteeding that |db_| is scoped_ptr<ValueStore::Error> EnsureDbIsOpen();
// non-NULL),
std::string EnsureDbIsOpen(); // Reads a setting from the database.
scoped_ptr<ValueStore::Error> ReadFromDb(
// Reads a setting from the database. Returns the error message on failure,
// or "" on success in which case |setting| will be reset to the Value read
// from the database. This value may be NULL.
std::string ReadFromDb(
leveldb::ReadOptions options, leveldb::ReadOptions options,
const std::string& key, const std::string& key,
// Will be reset() with the result, if any. // Will be reset() with the result, if any.
scoped_ptr<Value>* setting); scoped_ptr<Value>* setting);
// Adds a setting to a WriteBatch, and logs the change in |changes|. For use // Adds a setting to a WriteBatch, and logs the change in |changes|. For use
// with WriteToDb. Returns the error message on failure, or "" on success. // with WriteToDb.
std::string AddToBatch( scoped_ptr<ValueStore::Error> AddToBatch(ValueStore::WriteOptions options,
ValueStore::WriteOptions options, const std::string& key,
const std::string& key, const base::Value& value,
const base::Value& value, leveldb::WriteBatch* batch,
leveldb::WriteBatch* batch, ValueStoreChangeList* changes);
ValueStoreChangeList* changes);
// Commits the changes in |batch| to the database.
scoped_ptr<ValueStore::Error> WriteToDb(leveldb::WriteBatch* batch);
// Converts an error leveldb::Status to a ValueStore::Error. Returns a
// scoped_ptr for convenience; the result will always be non-empty.
scoped_ptr<ValueStore::Error> ToValueStoreError(
const leveldb::Status& status,
scoped_ptr<std::string> key);
// Commits the changes in |batch| to the database, returning the error message // Removes the on-disk database at |db_path_|. Any file system locks should
// on failure or "" on success. // be released before calling this method.
std::string WriteToDb(leveldb::WriteBatch* batch); void DeleteDbFile();
// Returns whether the database is empty. // Returns whether the database is empty.
bool IsEmpty(); bool IsEmpty();
......
chrome/browser/value_store/testing_value_store.cc
View file @ c571dd4b
...@@ -10,31 +10,13 @@ namespace { ...@@ -10,31 +10,13 @@ namespace {
const char kGenericErrorMessage[] = "TestingValueStore configured to error"; const char kGenericErrorMessage[] = "TestingValueStore configured to error";
ValueStore::ReadResult ReadResultError() {
return ValueStore::MakeReadResult(kGenericErrorMessage);
}
ValueStore::WriteResult WriteResultError() {
return ValueStore::MakeWriteResult(kGenericErrorMessage);
}
std::vector<std::string> CreateVector(const std::string& string) {
std::vector<std::string> strings;
strings.push_back(string);
return strings;
}
} // namespace } // namespace
TestingValueStore::TestingValueStore() TestingValueStore::TestingValueStore()
: read_count_(0), write_count_(0), fail_all_requests_(false) {} : read_count_(0), write_count_(0), error_code_(OK) {}
TestingValueStore::~TestingValueStore() {} TestingValueStore::~TestingValueStore() {}
void TestingValueStore::SetFailAllRequests(bool fail_all_requests) {
fail_all_requests_ = fail_all_requests;
}
size_t TestingValueStore::GetBytesInUse(const std::string& key) { size_t TestingValueStore::GetBytesInUse(const std::string& key) {
// Let SettingsStorageQuotaEnforcer implement this. // Let SettingsStorageQuotaEnforcer implement this.
NOTREACHED() << "Not implemented"; NOTREACHED() << "Not implemented";
...@@ -55,15 +37,14 @@ size_t TestingValueStore::GetBytesInUse() { ...@@ -55,15 +37,14 @@ size_t TestingValueStore::GetBytesInUse() {
} }
ValueStore::ReadResult TestingValueStore::Get(const std::string& key) { ValueStore::ReadResult TestingValueStore::Get(const std::string& key) {
return Get(CreateVector(key)); return Get(std::vector<std::string>(1, key));
} }
ValueStore::ReadResult TestingValueStore::Get( ValueStore::ReadResult TestingValueStore::Get(
const std::vector<std::string>& keys) { const std::vector<std::string>& keys) {
read_count_++; read_count_++;
if (fail_all_requests_) { if (error_code_ != OK)
return ReadResultError(); return MakeReadResult(TestingError());
}
DictionaryValue* settings = new DictionaryValue(); DictionaryValue* settings = new DictionaryValue();
for (std::vector<std::string>::const_iterator it = keys.begin(); for (std::vector<std::string>::const_iterator it = keys.begin();
...@@ -73,15 +54,14 @@ ValueStore::ReadResult TestingValueStore::Get( ...@@ -73,15 +54,14 @@ ValueStore::ReadResult TestingValueStore::Get(
settings->SetWithoutPathExpansion(*it, value->DeepCopy()); settings->SetWithoutPathExpansion(*it, value->DeepCopy());
} }
} }
return MakeReadResult(settings); return MakeReadResult(make_scoped_ptr(settings));
} }
ValueStore::ReadResult TestingValueStore::Get() { ValueStore::ReadResult TestingValueStore::Get() {
read_count_++; read_count_++;
if (fail_all_requests_) { if (error_code_ != OK)
return ReadResultError(); return MakeReadResult(TestingError());
} return MakeReadResult(make_scoped_ptr(storage_.DeepCopy()));
return MakeReadResult(storage_.DeepCopy());
} }
ValueStore::WriteResult TestingValueStore::Set( ValueStore::WriteResult TestingValueStore::Set(
...@@ -94,9 +74,8 @@ ValueStore::WriteResult TestingValueStore::Set( ...@@ -94,9 +74,8 @@ ValueStore::WriteResult TestingValueStore::Set(
ValueStore::WriteResult TestingValueStore::Set( ValueStore::WriteResult TestingValueStore::Set(
WriteOptions options, const DictionaryValue& settings) { WriteOptions options, const DictionaryValue& settings) {
write_count_++; write_count_++;
if (fail_all_requests_) { if (error_code_ != OK)
return WriteResultError(); return MakeWriteResult(TestingError());
}
scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList()); scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
for (DictionaryValue::Iterator it(settings); !it.IsAtEnd(); it.Advance()) { for (DictionaryValue::Iterator it(settings); !it.IsAtEnd(); it.Advance()) {
...@@ -111,19 +90,18 @@ ValueStore::WriteResult TestingValueStore::Set( ...@@ -111,19 +90,18 @@ ValueStore::WriteResult TestingValueStore::Set(
storage_.SetWithoutPathExpansion(it.key(), it.value().DeepCopy()); storage_.SetWithoutPathExpansion(it.key(), it.value().DeepCopy());
} }
} }
return MakeWriteResult(changes.release()); return MakeWriteResult(changes.Pass());
} }
ValueStore::WriteResult TestingValueStore::Remove(const std::string& key) { ValueStore::WriteResult TestingValueStore::Remove(const std::string& key) {
return Remove(CreateVector(key)); return Remove(std::vector<std::string>(1, key));
} }
ValueStore::WriteResult TestingValueStore::Remove( ValueStore::WriteResult TestingValueStore::Remove(
const std::vector<std::string>& keys) { const std::vector<std::string>& keys) {
write_count_++; write_count_++;
if (fail_all_requests_) { if (error_code_ != OK)
return WriteResultError(); return MakeWriteResult(TestingError());
}
scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList()); scoped_ptr<ValueStoreChangeList> changes(new ValueStoreChangeList());
for (std::vector<std::string>::const_iterator it = keys.begin(); for (std::vector<std::string>::const_iterator it = keys.begin();
...@@ -133,7 +111,7 @@ ValueStore::WriteResult TestingValueStore::Remove( ...@@ -133,7 +111,7 @@ ValueStore::WriteResult TestingValueStore::Remove(
changes->push_back(ValueStoreChange(*it, old_value.release(), NULL)); changes->push_back(ValueStoreChange(*it, old_value.release(), NULL));
} }
} }
return MakeWriteResult(changes.release()); return MakeWriteResult(changes.Pass());
} }
ValueStore::WriteResult TestingValueStore::Clear() { ValueStore::WriteResult TestingValueStore::Clear() {
...@@ -143,3 +121,8 @@ ValueStore::WriteResult TestingValueStore::Clear() { ...@@ -143,3 +121,8 @@ ValueStore::WriteResult TestingValueStore::Clear() {
} }
return Remove(keys); return Remove(keys);
} }
scoped_ptr<ValueStore::Error> TestingValueStore::TestingError() {
return make_scoped_ptr(new ValueStore::Error(
error_code_, kGenericErrorMessage, scoped_ptr<std::string>()));
}
chrome/browser/value_store/testing_value_store.h
View file @ c571dd4b
...@@ -15,8 +15,9 @@ class TestingValueStore : public ValueStore { ...@@ -15,8 +15,9 @@ class TestingValueStore : public ValueStore {
TestingValueStore(); TestingValueStore();
virtual ~TestingValueStore(); virtual ~TestingValueStore();
// Sets whether to fail all requests (default is false). // Sets the error code for requests. If OK, errors won't be thrown.
void SetFailAllRequests(bool fail_all_requests); // Defaults to OK.
void set_error_code(ErrorCode error_code) { error_code_ = error_code; }
// Accessors for the number of reads/writes done by this value store. Each // Accessors for the number of reads/writes done by this value store. Each
// Get* operation (except for the BytesInUse ones) counts as one read, and // Get* operation (except for the BytesInUse ones) counts as one read, and
...@@ -44,10 +45,12 @@ class TestingValueStore : public ValueStore { ...@@ -44,10 +45,12 @@ class TestingValueStore : public ValueStore {
virtual WriteResult Clear() OVERRIDE; virtual WriteResult Clear() OVERRIDE;
private: private:
scoped_ptr<ValueStore::Error> TestingError();
DictionaryValue storage_; DictionaryValue storage_;
int read_count_; int read_count_;
int write_count_; int write_count_;
bool fail_all_requests_; ErrorCode error_code_;
DISALLOW_COPY_AND_ASSIGN(TestingValueStore); DISALLOW_COPY_AND_ASSIGN(TestingValueStore);
}; };
......
chrome/browser/value_store/value_store.cc
View file @ c571dd4b
...@@ -6,58 +6,40 @@ ...@@ -6,58 +6,40 @@
#include "base/logging.h" #include "base/logging.h"
// Implementation of ReadResultType. // Implementation of Error.
ValueStore::ReadResultType::ReadResultType(DictionaryValue* settings) ValueStore::Error::Error(ErrorCode code,
: settings_(settings) { const std::string& message,
DCHECK(settings); scoped_ptr<std::string> key)
} : code(code), message(message), key(key.Pass()) {}
ValueStore::ReadResultType::ReadResultType(const std::string& error) ValueStore::Error::~Error() {}
: error_(error) {
DCHECK(!error.empty());
}
ValueStore::ReadResultType::~ReadResultType() {} // Implementation of ReadResultType.
bool ValueStore::ReadResultType::HasError() const { ValueStore::ReadResultType::ReadResultType(scoped_ptr<DictionaryValue> settings)
return !error_.empty(); : settings_(settings.Pass()) {
CHECK(settings_);
} }
scoped_ptr<DictionaryValue>& ValueStore::ReadResultType::settings() { ValueStore::ReadResultType::ReadResultType(scoped_ptr<Error> error)
DCHECK(!HasError()); : error_(error.Pass()) {
return settings_; CHECK(error_);
} }
const std::string& ValueStore::ReadResultType::error() const { ValueStore::ReadResultType::~ReadResultType() {}
DCHECK(HasError());
return error_;
}
// Implementation of WriteResultType. // Implementation of WriteResultType.
ValueStore::WriteResultType::WriteResultType(ValueStoreChangeList* changes) ValueStore::WriteResultType::WriteResultType(
: changes_(changes) { scoped_ptr<ValueStoreChangeList> changes)
DCHECK(changes); : changes_(changes.Pass()) {
CHECK(changes_);
} }
ValueStore::WriteResultType::WriteResultType(const std::string& error) ValueStore::WriteResultType::WriteResultType(scoped_ptr<Error> error)
: error_(error) { : error_(error.Pass()) {
DCHECK(!error.empty()); CHECK(error_);
} }
ValueStore::WriteResultType::~WriteResultType() {} ValueStore::WriteResultType::~WriteResultType() {}
bool ValueStore::WriteResultType::HasError() const {
return !error_.empty();
}
const ValueStoreChangeList& ValueStore::WriteResultType::changes() const {
DCHECK(!HasError());
return *changes_;
}
const std::string& ValueStore::WriteResultType::error() const {
DCHECK(HasError());
return error_;
}
chrome/browser/value_store/value_store.h
View file @ c571dd4b
...@@ -15,30 +15,85 @@ ...@@ -15,30 +15,85 @@
// Interface for a storage area for Value objects. // Interface for a storage area for Value objects.
class ValueStore { class ValueStore {
public: public:
// Error codes returned from storage methods.
enum ErrorCode {
OK,
// The failure was due to some kind of database corruption. Depending on
// what is corrupted, some part of the database may be recoverable.
//
// For example, if the on-disk representation of leveldb is corrupted, it's
// likely the whole database will need to be wiped and started again.
//
// If a single key has been committed with an invalid JSON representation,
// just that key can be deleted without affecting the rest of the database.
CORRUPTION,
// The failure was due to the store being read-only (for example, policy).
READ_ONLY,
// The failure was due to the store running out of space.
QUOTA_EXCEEDED,
// Any other error.
OTHER_ERROR,
};
// Bundles an ErrorCode with further metadata.
struct Error {
Error(ErrorCode code,
const std::string& message,
scoped_ptr<std::string> key);
~Error();
static scoped_ptr<Error> Create(ErrorCode code,
const std::string& message,
scoped_ptr<std::string> key) {
return make_scoped_ptr(new Error(code, message, key.Pass()));
}
// The error code.
const ErrorCode code;
// Message associated with the error.
const std::string message;
// The key associated with the error, if any. Use a scoped_ptr here
// because empty-string is a valid key.
//
// TODO(kalman): add test(s) for an empty key.
const scoped_ptr<std::string> key;
private:
DISALLOW_COPY_AND_ASSIGN(Error);
};
// The result of a read operation (Get). // The result of a read operation (Get).
class ReadResultType { class ReadResultType {
public: public:
// Ownership of |settings| taken. explicit ReadResultType(scoped_ptr<base::DictionaryValue> settings);
explicit ReadResultType(base::DictionaryValue* settings); explicit ReadResultType(scoped_ptr<Error> error);
explicit ReadResultType(const std::string& error);
~ReadResultType(); ~ReadResultType();
bool HasError() const { return error_; }
// Gets the settings read from the storage. Note that this represents // 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 // the root object. If you request the value for key "foo", that value will
// be in |settings.foo|. // be in |settings|.|foo|.
// Must only be called if HasError() is false. //
scoped_ptr<base::DictionaryValue>& settings(); // Must only be called if there is no error.
base::DictionaryValue& settings() { return *settings_; }
// Gets whether the operation failed. scoped_ptr<base::DictionaryValue> PassSettings() {
bool HasError() const; return settings_.Pass();
}
// Gets the error message describing the failure.
// Must only be called if HasError() is true. // Only call if HasError is true.
const std::string& error() const; const Error& error() const { return *error_; }
scoped_ptr<Error> PassError() { return error_.Pass(); }
private: private:
scoped_ptr<base::DictionaryValue> settings_; scoped_ptr<base::DictionaryValue> settings_;
const std::string error_; scoped_ptr<Error> error_;
DISALLOW_COPY_AND_ASSIGN(ReadResultType); DISALLOW_COPY_AND_ASSIGN(ReadResultType);
}; };
...@@ -47,25 +102,25 @@ class ValueStore { ...@@ -47,25 +102,25 @@ class ValueStore {
// The result of a write operation (Set/Remove/Clear). // The result of a write operation (Set/Remove/Clear).
class WriteResultType { class WriteResultType {
public: public:
// Ownership of |changes| taken. explicit WriteResultType(scoped_ptr<ValueStoreChangeList> changes);
explicit WriteResultType(ValueStoreChangeList* changes); explicit WriteResultType(scoped_ptr<Error> error);
explicit WriteResultType(const std::string& error);
~WriteResultType(); ~WriteResultType();
// Gets the list of changes to the settings which resulted from the write. bool HasError() const { return error_; }
// Must only be called if HasError() is false.
const ValueStoreChangeList& changes() const;
// Gets whether the operation failed. // Gets the list of changes to the settings which resulted from the write.
bool HasError() const; // Won't be present if the NO_GENERATE_CHANGES WriteOptions was given.
// Only call if HasError is false.
ValueStoreChangeList& changes() { return *changes_; }
scoped_ptr<ValueStoreChangeList> PassChanges() { return changes_.Pass(); }
// Gets the error message describing the failure. // Only call if HasError is true.
// Must only be called if HasError() is true. const Error& error() const { return *error_; }
const std::string& error() const; scoped_ptr<Error> PassError() { return error_.Pass(); }
private: private:
const scoped_ptr<ValueStoreChangeList> changes_; scoped_ptr<ValueStoreChangeList> changes_;
const std::string error_; scoped_ptr<Error> error_;
DISALLOW_COPY_AND_ASSIGN(WriteResultType); DISALLOW_COPY_AND_ASSIGN(WriteResultType);
}; };
...@@ -81,10 +136,6 @@ class ValueStore { ...@@ -81,10 +136,6 @@ class ValueStore {
// Don't generate the changes for a WriteResult. // Don't generate the changes for a WriteResult.
NO_GENERATE_CHANGES = 1<<2, 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; typedef int WriteOptions;
...@@ -92,13 +143,13 @@ class ValueStore { ...@@ -92,13 +143,13 @@ class ValueStore {
// Helpers for making a Read/WriteResult. // Helpers for making a Read/WriteResult.
template<typename T> template<typename T>
static ReadResult MakeReadResult(T arg) { static ReadResult MakeReadResult(scoped_ptr<T> arg) {
return ReadResult(new ReadResultType(arg)); return ReadResult(new ReadResultType(arg.Pass()));
} }
template<typename T> template<typename T>
static WriteResult MakeWriteResult(T arg) { static WriteResult MakeWriteResult(scoped_ptr<T> arg) {
return WriteResult(new WriteResultType(arg)); return WriteResult(new WriteResultType(arg.Pass()));
} }
// Gets the amount of space being used by a single value, in bytes. // Gets the amount of space being used by a single value, in bytes.
......
chrome/browser/value_store/value_store_frontend.cc
View file @ c571dd4b
...@@ -41,10 +41,10 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe<Backend> { ...@@ -41,10 +41,10 @@ class ValueStoreFrontend::Backend : public base::RefCountedThreadSafe<Backend> {
// callback. // callback.
scoped_ptr<base::Value> value; scoped_ptr<base::Value> value;
if (!result->HasError()) { if (!result->HasError()) {
result->settings()->RemoveWithoutPathExpansion(key, &value); result->settings().RemoveWithoutPathExpansion(key, &value);
} else { } else {
LOG(INFO) << "Reading " << key << " from " << db_path_.value() LOG(WARNING) << "Reading " << key << " from " << db_path_.value()
<< " failed: " << result->error(); << " failed: " << result->error().message;
} }
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
......
chrome/browser/value_store/value_store_frontend_unittest.cc
View file @ c571dd4b
...@@ -14,8 +14,6 @@ ...@@ -14,8 +14,6 @@
using content::BrowserThread; using content::BrowserThread;
// Test suite for CachingValueStore, using a test database with a few simple
// entries.
class ValueStoreFrontendTest : public testing::Test { class ValueStoreFrontendTest : public testing::Test {
public: public:
ValueStoreFrontendTest() ValueStoreFrontendTest()
......
chrome/browser/value_store/value_store_unittest.cc
View file @ c571dd4b
...@@ -56,11 +56,11 @@ testing::AssertionResult SettingsEq( ...@@ -56,11 +56,11 @@ testing::AssertionResult SettingsEq(
ValueStore::ReadResult actual_result) { ValueStore::ReadResult actual_result) {
if (actual_result->HasError()) { if (actual_result->HasError()) {
return testing::AssertionFailure() << return testing::AssertionFailure() <<
"Result has error: " << actual_result->error(); "Result has error: " << actual_result->error().message;
} }
std::string error; std::string error;
if (!ValuesEqual(&expected, actual_result->settings().get(), &error)) { if (!ValuesEqual(&expected, &actual_result->settings(), &error)) {
return testing::AssertionFailure() << error; return testing::AssertionFailure() << error;
} }
...@@ -75,7 +75,7 @@ testing::AssertionResult ChangesEq( ...@@ -75,7 +75,7 @@ testing::AssertionResult ChangesEq(
ValueStore::WriteResult actual_result) { ValueStore::WriteResult actual_result) {
if (actual_result->HasError()) { if (actual_result->HasError()) {
return testing::AssertionFailure() << return testing::AssertionFailure() <<
"Result has error: " << actual_result->error(); "Result has error: " << actual_result->error().message;
} }
const ValueStoreChangeList& actual = actual_result->changes(); const ValueStoreChangeList& actual = actual_result->changes();
......
chrome/browser/value_store/value_store_util.cc 0 → 100644
View file @ c571dd4b
// Copyright 2013 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/value_store_util.h"
namespace value_store_util {
scoped_ptr<std::string> NewKey(const std::string& key) {
return make_scoped_ptr(new std::string(key));
}
scoped_ptr<std::string> NoKey() {
return scoped_ptr<std::string>();
}
scoped_ptr<ValueStore::Error> NoError() {
return scoped_ptr<ValueStore::Error>();
}
} // namespace value_store_util
chrome/browser/value_store/value_store_util.h 0 → 100644
View file @ c571dd4b
// Copyright 2013 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_VALUE_STORE_UTIL_H_
#define CHROME_BROWSER_VALUE_STORE_VALUE_STORE_UTIL_H_
#include <string>
#include "base/memory/scoped_ptr.h"
#include "chrome/browser/value_store/value_store.h"
namespace value_store_util {
// Returns a copy of |key| as a scoped_ptr. Useful for creating keys for
// ValueStore::Error.
scoped_ptr<std::string> NewKey(const std::string& key);
// Returns an empty scoped_ptr. Useful for creating empty keys for
// ValueStore::Error.
scoped_ptr<std::string> NoKey();
// Return an empty Error. Useful for creating ValueStore::Error-less
// ValueStore::Read/WriteResults.
scoped_ptr<ValueStore::Error> NoError();
} // namespace value_store_util
#endif // CHROME_BROWSER_VALUE_STORE_VALUE_STORE_UTIL_H_
chrome/chrome_browser.gypi
View file @ c571dd4b
...@@ -2444,12 +2444,14 @@ ...@@ -2444,12 +2444,14 @@
'browser/value_store/leveldb_value_store.h', 'browser/value_store/leveldb_value_store.h',
'browser/value_store/testing_value_store.cc', 'browser/value_store/testing_value_store.cc',
'browser/value_store/testing_value_store.h', 'browser/value_store/testing_value_store.h',
'browser/value_store/value_store.cc',
'browser/value_store/value_store.h',
'browser/value_store/value_store_change.cc', 'browser/value_store/value_store_change.cc',
'browser/value_store/value_store_change.h', 'browser/value_store/value_store_change.h',
'browser/value_store/value_store_frontend.cc', 'browser/value_store/value_store_frontend.cc',
'browser/value_store/value_store_frontend.h', 'browser/value_store/value_store_frontend.h',
'browser/value_store/value_store.cc', 'browser/value_store/value_store_util.cc',
'browser/value_store/value_store.h', 'browser/value_store/value_store_util.h',
'browser/web_applications/web_app.cc', 'browser/web_applications/web_app.cc',
'browser/web_applications/web_app.h', 'browser/web_applications/web_app.h',
'browser/web_applications/web_app_android.cc', 'browser/web_applications/web_app_android.cc',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment