diff options
author | kalman@chromium.org <kalman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-09 01:12:50 +0000 |
---|---|---|
committer | kalman@chromium.org <kalman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-11-09 01:12:50 +0000 |
commit | 8650302994745872343d8de7a3b18b1ecb336228 (patch) | |
tree | f514263566c53bc5666cd3e4decb236d536b7c79 | |
parent | 3a1ba433272ed041e4b0d26e262272e3da600ede (diff) | |
download | chromium_src-8650302994745872343d8de7a3b18b1ecb336228.zip chromium_src-8650302994745872343d8de7a3b18b1ecb336228.tar.gz chromium_src-8650302994745872343d8de7a3b18b1ecb336228.tar.bz2 |
Various changes to the Extension Settings API.
Goals:
1 - Make it so that chrome.experimental.settings.get() can take an object with the default values to return in settings if any settings aren't set.
2 - Make it so that options pages get change events when the background page changes a setting.
3 - To solve the above: make it so that onChanged events are sent to all registered listeners, not just those which are on different processes (i.e. the incognito extension process).
4 - Remove any result in the callback from chrome.experimental.settings.set(), so that there is an unambiguous way to get changed settings (the event). It's also just generally pointless.
BUG=100825,101452,101459
TEST=*ExtensionSetting*
Review URL: http://codereview.chromium.org/8465002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@109155 0039d316-1c4b-4281-b951-d872f2087c98
33 files changed, 1200 insertions, 1303 deletions
diff --git a/chrome/browser/extensions/extension_setting_change.cc b/chrome/browser/extensions/extension_setting_change.cc new file mode 100644 index 0000000..87fed61 --- /dev/null +++ b/chrome/browser/extensions/extension_setting_change.cc @@ -0,0 +1,57 @@ +// Copyright (c) 2011 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/extensions/extension_setting_change.h" + +#include "base/json/json_writer.h" +#include "base/logging.h" +#include "base/values.h" + +/* static */ +std::string ExtensionSettingChange::GetEventJson( + const ExtensionSettingChangeList& changes) { + ListValue changes_value; + for (ExtensionSettingChangeList::const_iterator it = changes.begin(); + it != changes.end(); ++it) { + DictionaryValue* change_value = new DictionaryValue(); + change_value->SetString("key", it->key()); + if (it->old_value()) { + change_value->Set("oldValue", it->old_value()->DeepCopy()); + } + if (it->new_value()) { + change_value->Set("newValue", it->new_value()->DeepCopy()); + } + changes_value.Append(change_value); + } + std::string json; + base::JSONWriter::Write(&changes_value, false, &json); + return json; +} + +ExtensionSettingChange::ExtensionSettingChange( + const std::string& key, Value* old_value, Value* new_value) + : inner_(new Inner(key, old_value, new_value)) {} + +ExtensionSettingChange::~ExtensionSettingChange() {} + +const std::string& ExtensionSettingChange::key() const { + DCHECK(inner_.get()); + return inner_->key_; +} + +const Value* ExtensionSettingChange::old_value() const { + DCHECK(inner_.get()); + return inner_->old_value_.get(); +} + +const Value* ExtensionSettingChange::new_value() const { + DCHECK(inner_.get()); + return inner_->new_value_.get(); +} + +ExtensionSettingChange::Inner::Inner( + const std::string& key, base::Value* old_value, base::Value* new_value) + : key_(key), old_value_(old_value), new_value_(new_value) {} + +ExtensionSettingChange::Inner::~Inner() {} diff --git a/chrome/browser/extensions/extension_setting_change.h b/chrome/browser/extensions/extension_setting_change.h new file mode 100644 index 0000000..fb6e29c --- /dev/null +++ b/chrome/browser/extensions/extension_setting_change.h @@ -0,0 +1,60 @@ +// Copyright (c) 2011 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_EXTENSIONS_EXTENSION_SETTING_CHANGE_H_ +#define CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTING_CHANGE_H_ +#pragma once + +#include <string> + +#include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" +#include "base/values.h" + +class ExtensionSettingChange; +typedef std::vector<ExtensionSettingChange> ExtensionSettingChangeList; + +// A change to a setting. Safe/efficient to copy. +class ExtensionSettingChange { + public: + // Converts an ExtensionSettingChangeList into JSON of the form: + // [ { "key": "foo", "oldValue": "bar", "newValue": "baz" } ] + static std::string GetEventJson(const ExtensionSettingChangeList& changes); + + // Ownership of |old_value| and |new_value| taken. + ExtensionSettingChange( + const std::string& key, base::Value* old_value, base::Value* new_value); + + ~ExtensionSettingChange(); + + // Gets the key of the setting which changed. + const std::string& key() const; + + // Gets the value of the setting before the change, or NULL if there was no + // old value. + const base::Value* old_value() const; + + // Gets the value of the setting after the change, or NULL if there is no new + // value. + const base::Value* new_value() const; + + private: + class Inner : public base::RefCountedThreadSafe<Inner> { + public: + Inner( + const std::string& key, base::Value* old_value, base::Value* new_value); + + const std::string key_; + const scoped_ptr<base::Value> old_value_; + const scoped_ptr<base::Value> new_value_; + + private: + friend class base::RefCountedThreadSafe<Inner>; + virtual ~Inner(); + }; + + scoped_refptr<Inner> inner_; +}; + +#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTING_CHANGE_H_ diff --git a/chrome/browser/extensions/extension_setting_changes.cc b/chrome/browser/extensions/extension_setting_changes.cc deleted file mode 100644 index ce1a95c..0000000 --- a/chrome/browser/extensions/extension_setting_changes.cc +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) 2011 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/extensions/extension_setting_changes.h" - -#include "base/json/json_writer.h" - -void ExtensionSettingChanges::Builder::AppendChange( - const std::string& key, Value* old_value, Value* new_value) { - DictionaryValue* change = new DictionaryValue(); - changes_.Append(change); - - change->SetString("key", key); - if (old_value) { - change->Set("oldValue", old_value); - } - if (new_value) { - change->Set("newValue", new_value); - } -} - -ExtensionSettingChanges ExtensionSettingChanges::Builder::Build() { - return ExtensionSettingChanges(&changes_); -} - -ExtensionSettingChanges::ExtensionSettingChanges(ListValue* changes) - : inner_(new Inner()) { - inner_->changes_.Swap(changes); -} - -ExtensionSettingChanges::~ExtensionSettingChanges() {} - -std::string ExtensionSettingChanges::ToJson() const { - std::string changes_json; - base::JSONWriter::Write(&inner_->changes_, false, &changes_json); - return changes_json; -} diff --git a/chrome/browser/extensions/extension_setting_changes.h b/chrome/browser/extensions/extension_setting_changes.h deleted file mode 100644 index af9f363..0000000 --- a/chrome/browser/extensions/extension_setting_changes.h +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright (c) 2011 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_EXTENSIONS_EXTENSION_SETTING_CHANGES_H_ -#define CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTING_CHANGES_H_ -#pragma once - -#include "base/memory/ref_counted.h" -#include "base/values.h" - -// A list of changes to extension settings. Create using the Builder class. -// The Changes object is thread-safe and efficient to copy, while the Builder -// object isn't. -class ExtensionSettingChanges { - public: - // Non-thread-safe builder for ExtensionSettingChanges. - class Builder { - public: - Builder() {} - - // Appends a change for a setting. - void AppendChange( - const std::string& key, - // Ownership taken. May be NULL to imply there is no old value. - Value* old_value, - // Ownership taken. May be NULL to imply there is no new value. - Value* new_value); - - // Creates an ExtensionSettingChanges object. The Builder should not be - // used after calling this. - ExtensionSettingChanges Build(); - - private: - ListValue changes_; - - DISALLOW_COPY_AND_ASSIGN(Builder); - }; - - ~ExtensionSettingChanges(); - - // Gets the JSON serialized form of the changes, for example: - // [ { "key": "foo", "oldValue": "bar", "newValue": "baz" } ] - std::string ToJson() const; - - private: - // Create using the Builder class. |changes| will be cleared. - explicit ExtensionSettingChanges(ListValue* changes); - - // Ref-counted inner class, a wrapper over a non-thread-safe string. - class Inner : public base::RefCountedThreadSafe<Inner> { - public: - Inner() {} - - // swap() this with the changes from the Builder after construction. - ListValue changes_; - - private: - virtual ~Inner() {} - friend class base::RefCountedThreadSafe<Inner>; - }; - - scoped_refptr<Inner> inner_; -}; - -#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTING_CHANGES_H_ diff --git a/chrome/browser/extensions/extension_settings_api.cc b/chrome/browser/extensions/extension_settings_api.cc index 60cb933..d96c47b 100644 --- a/chrome/browser/extensions/extension_settings_api.cc +++ b/chrome/browser/extensions/extension_settings_api.cc @@ -43,37 +43,32 @@ void SettingsFunction::RunWithStorageOnFileThread( base::Bind(&SettingsFunction::SendResponse, this, success)); } -bool SettingsFunction::UseResult( - scoped_refptr<ExtensionSettingsObserverList> observers, - const ExtensionSettingsStorage::Result& storage_result) { +bool SettingsFunction::UseReadResult( + const ExtensionSettingsStorage::ReadResult& result) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - if (storage_result.HasError()) { - error_ = storage_result.GetError(); + if (result.HasError()) { + error_ = result.error(); return false; } - const DictionaryValue* settings = storage_result.GetSettings(); - if (settings) { - result_.reset(settings->DeepCopy()); + result_.reset(result.settings().DeepCopy()); + return true; +} + +bool SettingsFunction::UseWriteResult( + scoped_refptr<ExtensionSettingsObserverList> observers, + const ExtensionSettingsStorage::WriteResult& result) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + if (result.HasError()) { + error_ = result.error(); + return false; } - const std::set<std::string>* changed_keys = storage_result.GetChangedKeys(); - if (changed_keys && !changed_keys->empty()) { - ExtensionSettingChanges::Builder changes; - for (std::set<std::string>::const_iterator it = changed_keys->begin(); - it != changed_keys->end(); ++it) { - const Value* old_value = storage_result.GetOldValue(*it); - const Value* new_value = storage_result.GetNewValue(*it); - changes.AppendChange( - *it, - old_value ? old_value->DeepCopy() : NULL, - new_value ? new_value->DeepCopy() : NULL); - } + if (!result.changes().empty()) { observers->Notify( &ExtensionSettingsObserver::OnSettingsChanged, - profile(), extension_id(), - changes.Build()); + ExtensionSettingChange::GetEventJson(result.changes())); } return true; @@ -93,25 +88,57 @@ static void AddAllStringValues( } } +// Gets the keys of a DictionaryValue. +static std::vector<std::string> GetKeys(const DictionaryValue& dict) { + std::vector<std::string> keys; + for (DictionaryValue::key_iterator it = dict.begin_keys(); + it != dict.end_keys(); ++it) { + keys.push_back(*it); + } + return keys; +} + bool GetSettingsFunction::RunWithStorage( scoped_refptr<ExtensionSettingsObserverList> observers, ExtensionSettingsStorage* storage) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); Value *input; EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input)); - std::string as_string; - ListValue* as_list; - if (input->IsType(Value::TYPE_NULL)) { - return UseResult(observers, storage->Get()); - } else if (input->GetAsString(&as_string)) { - return UseResult(observers, storage->Get(as_string)); - } else if (input->GetAsList(&as_list)) { - std::vector<std::string> string_list; - AddAllStringValues(*as_list, &string_list); - return UseResult(observers, storage->Get(string_list)); + + switch (input->GetType()) { + case Value::TYPE_NULL: + return UseReadResult(storage->Get()); + + case Value::TYPE_STRING: { + std::string as_string; + input->GetAsString(&as_string); + return UseReadResult(storage->Get(as_string)); + } + + case Value::TYPE_LIST: { + std::vector<std::string> as_string_list; + AddAllStringValues(*static_cast<ListValue*>(input), &as_string_list); + return UseReadResult(storage->Get(as_string_list)); + } + + case Value::TYPE_DICTIONARY: { + DictionaryValue* as_dict = static_cast<DictionaryValue*>(input); + ExtensionSettingsStorage::ReadResult result = + storage->Get(GetKeys(*as_dict)); + if (result.HasError()) { + return UseReadResult(result); + } + + DictionaryValue* with_default_values = as_dict->DeepCopy(); + with_default_values->MergeDictionary(&result.settings()); + return UseReadResult( + ExtensionSettingsStorage::ReadResult(with_default_values)); + } + + default: + return UseReadResult( + ExtensionSettingsStorage::ReadResult(kUnsupportedArgumentType)); } - return UseResult( - observers, ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); } bool SetSettingsFunction::RunWithStorage( @@ -120,7 +147,7 @@ bool SetSettingsFunction::RunWithStorage( DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DictionaryValue *input; EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &input)); - return UseResult(observers, storage->Set(*input)); + return UseWriteResult(observers, storage->Set(*input)); } bool RemoveSettingsFunction::RunWithStorage( @@ -129,22 +156,30 @@ bool RemoveSettingsFunction::RunWithStorage( DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); Value *input; EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &input)); - std::string as_string; - ListValue* as_list; - if (input->GetAsString(&as_string)) { - return UseResult(observers, storage->Remove(as_string)); - } else if (input->GetAsList(&as_list)) { - std::vector<std::string> string_list; - AddAllStringValues(*as_list, &string_list); - return UseResult(observers, storage->Remove(string_list)); - } - return UseResult( - observers, ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); + + switch (input->GetType()) { + case Value::TYPE_STRING: { + std::string as_string; + input->GetAsString(&as_string); + return UseWriteResult(observers, storage->Remove(as_string)); + } + + case Value::TYPE_LIST: { + std::vector<std::string> as_string_list; + AddAllStringValues(*static_cast<ListValue*>(input), &as_string_list); + return UseWriteResult(observers, storage->Remove(as_string_list)); + } + + default: + return UseWriteResult( + observers, + ExtensionSettingsStorage::WriteResult(kUnsupportedArgumentType)); + }; } bool ClearSettingsFunction::RunWithStorage( scoped_refptr<ExtensionSettingsObserverList> observers, ExtensionSettingsStorage* storage) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - return UseResult(observers, storage->Clear()); + return UseWriteResult(observers, storage->Clear()); } diff --git a/chrome/browser/extensions/extension_settings_api.h b/chrome/browser/extensions/extension_settings_api.h index 6893ddd..b421156a 100644 --- a/chrome/browser/extensions/extension_settings_api.h +++ b/chrome/browser/extensions/extension_settings_api.h @@ -26,11 +26,17 @@ class SettingsFunction : public AsyncExtensionFunction { scoped_refptr<ExtensionSettingsObserverList> observers, ExtensionSettingsStorage* storage) = 0; - // Sets error_ or result_ depending on the value of a storage Result, and - // returns whether the Result implies success (i.e. !error). - bool UseResult( + // Sets error_ or result_ depending on the value of a storage ReadResult, and + // returns whether the result implies success (i.e. !error). + bool UseReadResult( + const ExtensionSettingsStorage::ReadResult& result); + + // Sets error_ depending on the value of a storage WriteResult, sends a + // change notification if needed, and returns whether the result implies + // success (i.e. !error). + bool UseWriteResult( scoped_refptr<ExtensionSettingsObserverList> observers, - const ExtensionSettingsStorage::Result& storage_result); + const ExtensionSettingsStorage::WriteResult& result); private: // Called via PostTask from RunImpl. Calls RunWithStorage and then diff --git a/chrome/browser/extensions/extension_settings_apitest.cc b/chrome/browser/extensions/extension_settings_apitest.cc index a2dc5e1..35f870f 100644 --- a/chrome/browser/extensions/extension_settings_apitest.cc +++ b/chrome/browser/extensions/extension_settings_apitest.cc @@ -179,11 +179,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionSettingsApiTest, LoadAndReplyWhenSatisfied( "assertNoNotifications", "assertNoNotifications", "split_incognito"); ReplyWhenSatisfied("noop", "setFoo"); - ReplyWhenSatisfied("assertAddFooNotification", "assertNoNotifications"); - ReplyWhenSatisfied("clearNotifications", "noop"); + ReplyWhenSatisfied("assertAddFooNotification", "assertAddFooNotification"); + ReplyWhenSatisfied("clearNotifications", "clearNotifications"); ReplyWhenSatisfied("removeFoo", "noop"); FinalReplyWhenSatisfied( - "assertNoNotifications", "assertDeleteFooNotification"); + "assertDeleteFooNotification", "assertDeleteFooNotification"); EXPECT_TRUE(catcher.GetNextResult()) << catcher.message(); EXPECT_TRUE(catcher_incognito.GetNextResult()) << catcher.message(); diff --git a/chrome/browser/extensions/extension_settings_backend.cc b/chrome/browser/extensions/extension_settings_backend.cc index ef87b51..e83b8ac 100644 --- a/chrome/browser/extensions/extension_settings_backend.cc +++ b/chrome/browser/extensions/extension_settings_backend.cc @@ -117,17 +117,6 @@ void ExtensionSettingsBackend::DeleteStorage(const std::string& extension_id) { storage_objs_.erase(maybe_storage); } -void ExtensionSettingsBackend::TriggerOnSettingsChanged( - Profile* profile, - const std::string& extension_id, - const ExtensionSettingChanges& changes) const { - observers_->Notify( - &ExtensionSettingsObserver::OnSettingsChanged, - profile, - extension_id, - changes); -} - std::set<std::string> ExtensionSettingsBackend::GetKnownExtensionIDs() const { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); std::set<std::string> result; @@ -157,6 +146,17 @@ std::set<std::string> ExtensionSettingsBackend::GetKnownExtensionIDs() const { return result; } +static void AddAllSyncData( + const std::string& extension_id, + const DictionaryValue& src, + SyncDataList* dst) { + for (DictionaryValue::Iterator it(src); it.HasNext(); it.Advance()) { + dst->push_back( + extension_settings_sync_util::CreateData( + extension_id, it.key(), it.value())); + } +} + SyncDataList ExtensionSettingsBackend::GetAllSyncData( syncable::ModelType type) const { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); @@ -172,21 +172,14 @@ SyncDataList ExtensionSettingsBackend::GetAllSyncData( for (std::set<std::string>::const_iterator it = known_extension_ids.begin(); it != known_extension_ids.end(); ++it) { - ExtensionSettingsStorage::Result maybe_settings = GetStorage(*it)->Get(); + ExtensionSettingsStorage::ReadResult maybe_settings = + GetStorage(*it)->Get(); if (maybe_settings.HasError()) { LOG(WARNING) << "Failed to get settings for " << *it << ": " << - maybe_settings.GetError(); + maybe_settings.error(); continue; } - - const DictionaryValue* settings = maybe_settings.GetSettings(); - for (DictionaryValue::key_iterator key_it = settings->begin_keys(); - key_it != settings->end_keys(); ++key_it) { - Value *value = NULL; - settings->GetWithoutPathExpansion(*key_it, &value); - all_sync_data.push_back( - extension_settings_sync_util::CreateData(*it, *key_it, *value)); - } + AddAllSyncData(*it, maybe_settings.settings(), &all_sync_data); } return all_sync_data; diff --git a/chrome/browser/extensions/extension_settings_backend.h b/chrome/browser/extensions/extension_settings_backend.h index 3a0229d..15c5ba5 100644 --- a/chrome/browser/extensions/extension_settings_backend.h +++ b/chrome/browser/extensions/extension_settings_backend.h @@ -42,13 +42,6 @@ class ExtensionSettingsBackend : public SyncableService { // Deletes all setting data for an extension. Call on the FILE thread. void DeleteStorage(const std::string& extension_id); - // Sends a change event to the observer list. |profile| is the profile which - // generated the change. Must be called on the FILE thread. - void TriggerOnSettingsChanged( - Profile* profile, - const std::string& extension_id, - const ExtensionSettingChanges& changes) const; - // SyncableService implementation. virtual SyncDataList GetAllSyncData(syncable::ModelType type) const OVERRIDE; virtual SyncError MergeDataAndStartSyncing( diff --git a/chrome/browser/extensions/extension_settings_frontend.cc b/chrome/browser/extensions/extension_settings_frontend.cc index 077cfa8..0281ad8 100644 --- a/chrome/browser/extensions/extension_settings_frontend.cc +++ b/chrome/browser/extensions/extension_settings_frontend.cc @@ -80,31 +80,24 @@ void DeleteStorageOnFileThread( } // namespace -class ExtensionSettingsFrontend::DefaultObserver - : public ExtensionSettingsObserver { - public: - explicit DefaultObserver(Profile* profile) : target_profile_(profile) {} - virtual ~DefaultObserver() {} - - virtual void OnSettingsChanged( - const Profile* origin_profile, - const std::string& extension_id, - const ExtensionSettingChanges& changes) OVERRIDE { - if (origin_profile != target_profile_) { - target_profile_->GetExtensionEventRouter()->DispatchEventToExtension( - extension_id, - extension_event_names::kOnSettingsChanged, - // This is the list of function arguments to pass to the onChanged - // handler of extensions, a single argument with the list of changes. - std::string("[") + changes.ToJson() + "]", - target_profile_, - GURL()); - } - } +// DefaultObserver. - private: - Profile* target_profile_; -}; +ExtensionSettingsFrontend::DefaultObserver::DefaultObserver(Profile* profile) + : profile_(profile) {} + +ExtensionSettingsFrontend::DefaultObserver::~DefaultObserver() {} + +void ExtensionSettingsFrontend::DefaultObserver::OnSettingsChanged( + const std::string& extension_id, const std::string& changes_json) { + profile_->GetExtensionEventRouter()->DispatchEventToExtension( + extension_id, + extension_event_names::kOnSettingsChanged, + // This is the list of function arguments to pass to the onChanged + // handler of extensions, a single argument with the list of changes. + std::string("[") + changes_json + "]", + NULL, + GURL()); +} class ExtensionSettingsFrontend::Core : public base::RefCountedThreadSafe<Core> { @@ -170,18 +163,12 @@ class ExtensionSettingsFrontend::Core ExtensionSettingsFrontend::ExtensionSettingsFrontend(Profile* profile) : profile_(profile), observers_(new ExtensionSettingsObserverList()), + default_observer_(profile), core_(new ExtensionSettingsFrontend::Core(observers_)) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(!profile->IsOffTheRecord()); - // This class listens to all PROFILE_{CREATED,DESTROYED} events but we're - // only interested in those for the original Profile given on construction - // and its incognito version. - registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED, - content::NotificationService::AllBrowserContextsAndSources()); - registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED, - content::NotificationService::AllBrowserContextsAndSources()); - OnProfileCreated(profile); + observers_->AddObserver(&default_observer_); BrowserThread::PostTask( BrowserThread::FILE, @@ -194,6 +181,7 @@ ExtensionSettingsFrontend::ExtensionSettingsFrontend(Profile* profile) ExtensionSettingsFrontend::~ExtensionSettingsFrontend() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + observers_->RemoveObserver(&default_observer_); } void ExtensionSettingsFrontend::RunWithSyncableService( @@ -248,57 +236,3 @@ ExtensionSettingsFrontend::GetObservers() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); return observers_; } - -void ExtensionSettingsFrontend::Observe( - int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - switch (type) { - case chrome::NOTIFICATION_PROFILE_CREATED: - OnProfileCreated(content::Source<Profile>(source).ptr()); - break; - case chrome::NOTIFICATION_PROFILE_DESTROYED: - OnProfileDestroyed(content::Source<Profile>(source).ptr()); - break; - default: - NOTREACHED(); - } -} - -void ExtensionSettingsFrontend::OnProfileCreated(Profile* new_profile) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (new_profile == profile_) { - ClearDefaultObserver(&original_profile_observer); - SetDefaultObserver(new_profile, &original_profile_observer); - } else if (new_profile->GetOriginalProfile() == profile_) { - DCHECK(new_profile->IsOffTheRecord()); - ClearDefaultObserver(&incognito_profile_observer_); - SetDefaultObserver(new_profile, &incognito_profile_observer_); - } -} - -void ExtensionSettingsFrontend::OnProfileDestroyed(Profile* old_profile) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (old_profile == profile_) { - ClearDefaultObserver(&original_profile_observer); - } else if (old_profile->GetOriginalProfile() == profile_) { - DCHECK(old_profile->IsOffTheRecord()); - ClearDefaultObserver(&incognito_profile_observer_); - } -} - -void ExtensionSettingsFrontend::SetDefaultObserver( - Profile* profile, scoped_ptr<DefaultObserver>* observer) { - DCHECK(!observer->get()); - observer->reset(new DefaultObserver(profile)); - observers_->AddObserver(observer->get()); -} - -void ExtensionSettingsFrontend::ClearDefaultObserver( - scoped_ptr<DefaultObserver>* observer) { - if (observer->get()) { - observers_->RemoveObserver(observer->get()); - observer->reset(); - } -} diff --git a/chrome/browser/extensions/extension_settings_frontend.h b/chrome/browser/extensions/extension_settings_frontend.h index ab63f18..6f2e023 100644 --- a/chrome/browser/extensions/extension_settings_frontend.h +++ b/chrome/browser/extensions/extension_settings_frontend.h @@ -23,7 +23,7 @@ class ExtensionSettingsStorage; // The component of extension settings which runs on the UI thread, as opposed // to ExtensionSettingsBackend which lives on the FILE thread. // All public methods must be called on the UI thread. -class ExtensionSettingsFrontend : public content::NotificationObserver { +class ExtensionSettingsFrontend { public: explicit ExtensionSettingsFrontend(Profile* profile); virtual ~ExtensionSettingsFrontend(); @@ -48,50 +48,36 @@ class ExtensionSettingsFrontend : public content::NotificationObserver { // Gets the thread-safe observer list. scoped_refptr<ExtensionSettingsObserverList> GetObservers(); - // NotificationObserver implementation. - virtual void Observe( - int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) OVERRIDE; - private: - // Observer which sends events to a target profile iff the profile isn't the - // originating profile for the event. - class DefaultObserver; - - // Called when a profile is created. - void OnProfileCreated(Profile* profile); - - // Called when a profile is destroyed. - void OnProfileDestroyed(Profile* profile); - - // Creates a scoped DefaultObserver and adds it as an Observer. - void SetDefaultObserver( - Profile* profile, scoped_ptr<DefaultObserver>* observer); - - // If a scoped DefaultObserver exists, clears it and removes it as an - // Observer. - void ClearDefaultObserver(scoped_ptr<DefaultObserver>* observer); - - // The (original) Profile this Frontend belongs to. Note that we don't store - // the incognito version of the profile because it will change as it's - // created and destroyed during the lifetime of Chrome. + // Settings change Observer which forwards changes on to the extension + // processes for |profile| and its incognito partner if it exists. + class DefaultObserver : public ExtensionSettingsObserver { + public: + explicit DefaultObserver(Profile* profile); + virtual ~DefaultObserver(); + + // ExtensionSettingsObserver implementation. + virtual void OnSettingsChanged( + const std::string& extension_id, + const std::string& change_json) OVERRIDE; + + private: + Profile* const profile_; + }; + + // The (non-incognito) Profile this Frontend belongs to. Profile* const profile_; // List of observers to settings changes. scoped_refptr<ExtensionSettingsObserverList> observers_; - // The default original and incognito mode profile observers. - scoped_ptr<DefaultObserver> original_profile_observer; - scoped_ptr<DefaultObserver> incognito_profile_observer_; + // Observer for |profile_|. + DefaultObserver default_observer_; // Ref-counted container for the ExtensionSettingsBackend object. class Core; scoped_refptr<Core> core_; - // For profile created/destroyed notifications. - content::NotificationRegistrar registrar_; - DISALLOW_COPY_AND_ASSIGN(ExtensionSettingsFrontend); }; diff --git a/chrome/browser/extensions/extension_settings_frontend_unittest.cc b/chrome/browser/extensions/extension_settings_frontend_unittest.cc index a7eb634..0c69c3e 100644 --- a/chrome/browser/extensions/extension_settings_frontend_unittest.cc +++ b/chrome/browser/extensions/extension_settings_frontend_unittest.cc @@ -60,20 +60,26 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) { // The correctness of Get/Set/Remove/Clear is tested elsewhere so no need to // be too rigorous. - StringValue bar("bar"); - ExtensionSettingsStorage::Result result = storage->Set("foo", bar); - ASSERT_FALSE(result.HasError()); + { + StringValue bar("bar"); + ExtensionSettingsStorage::WriteResult result = storage->Set("foo", bar); + ASSERT_FALSE(result.HasError()); + } - result = storage->Get(); - ASSERT_FALSE(result.HasError()); - EXPECT_FALSE(result.GetSettings()->empty()); + { + ExtensionSettingsStorage::ReadResult result = storage->Get(); + ASSERT_FALSE(result.HasError()); + EXPECT_FALSE(result.settings().empty()); + } frontend_.reset(new ExtensionSettingsFrontend(profile_.get())); storage = GetStorage(id, frontend_.get()); - result = storage->Get(); - ASSERT_FALSE(result.HasError()); - EXPECT_FALSE(result.GetSettings()->empty()); + { + ExtensionSettingsStorage::ReadResult result = storage->Get(); + ASSERT_FALSE(result.HasError()); + EXPECT_FALSE(result.settings().empty()); + } } TEST_F(ExtensionSettingsFrontendTest, SettingsClearedOnUninstall) { @@ -83,9 +89,11 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsClearedOnUninstall) { ExtensionSettingsStorage* storage = GetStorage(id, frontend_.get()); - StringValue bar("bar"); - ExtensionSettingsStorage::Result result = storage->Set("foo", bar); - ASSERT_FALSE(result.HasError()); + { + StringValue bar("bar"); + ExtensionSettingsStorage::WriteResult result = storage->Set("foo", bar); + ASSERT_FALSE(result.HasError()); + } // This would be triggered by extension uninstall via an ExtensionDataDeleter. frontend_->DeleteStorageSoon(id); @@ -93,9 +101,11 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsClearedOnUninstall) { // The storage area may no longer be valid post-uninstall, so re-request. storage = GetStorage(id, frontend_.get()); - result = storage->Get(); - ASSERT_FALSE(result.HasError()); - EXPECT_TRUE(result.GetSettings()->empty()); + { + ExtensionSettingsStorage::ReadResult result = storage->Get(); + ASSERT_FALSE(result.HasError()); + EXPECT_TRUE(result.settings().empty()); + } } TEST_F(ExtensionSettingsFrontendTest, LeveldbDatabaseDeletedFromDiskOnClear) { @@ -105,16 +115,20 @@ TEST_F(ExtensionSettingsFrontendTest, LeveldbDatabaseDeletedFromDiskOnClear) { ExtensionSettingsStorage* storage = GetStorage(id, frontend_.get()); - StringValue bar("bar"); - ExtensionSettingsStorage::Result result = storage->Set("foo", bar); - ASSERT_FALSE(result.HasError()); - EXPECT_TRUE(file_util::PathExists(temp_dir_.path())); + { + StringValue bar("bar"); + ExtensionSettingsStorage::WriteResult result = storage->Set("foo", bar); + ASSERT_FALSE(result.HasError()); + EXPECT_TRUE(file_util::PathExists(temp_dir_.path())); + } // Should need to both clear the database and delete the frontend for the // leveldb database to be deleted from disk. - result = storage->Clear(); - ASSERT_FALSE(result.HasError()); - EXPECT_TRUE(file_util::PathExists(temp_dir_.path())); + { + ExtensionSettingsStorage::WriteResult result = storage->Clear(); + ASSERT_FALSE(result.HasError()); + EXPECT_TRUE(file_util::PathExists(temp_dir_.path())); + } frontend_.reset(); MessageLoop::current()->RunAllPending(); diff --git a/chrome/browser/extensions/extension_settings_leveldb_storage.cc b/chrome/browser/extensions/extension_settings_leveldb_storage.cc index 96e11f6..1fb280d 100644 --- a/chrome/browser/extensions/extension_settings_leveldb_storage.cc +++ b/chrome/browser/extensions/extension_settings_leveldb_storage.cc @@ -91,21 +91,21 @@ ExtensionSettingsLeveldbStorage::~ExtensionSettingsLeveldbStorage() { } } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsLeveldbStorage::Get( const std::string& key) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); scoped_ptr<Value> setting; if (!ReadFromDb(leveldb::ReadOptions(), key, &setting)) { - return Result(kGenericOnFailureMessage); + return ReadResult(kGenericOnFailureMessage); } DictionaryValue* settings = new DictionaryValue(); if (setting.get()) { settings->SetWithoutPathExpansion(key, setting.release()); } - return Result(settings, NULL, NULL); + return ReadResult(settings); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsLeveldbStorage::Get( const std::vector<std::string>& keys) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); leveldb::ReadOptions options; @@ -119,17 +119,17 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( it != keys.end(); ++it) { scoped_ptr<Value> setting; if (!ReadFromDb(options, *it, &setting)) { - return Result(kGenericOnFailureMessage); + return ReadResult(kGenericOnFailureMessage); } if (setting.get()) { settings->SetWithoutPathExpansion(*it, setting.release()); } } - return Result(settings.release(), NULL, NULL); + return ReadResult(settings.release()); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() { +ExtensionSettingsStorage::ReadResult ExtensionSettingsLeveldbStorage::Get() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); base::JSONReader json_reader; leveldb::ReadOptions options = leveldb::ReadOptions(); @@ -153,13 +153,13 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() { if (!it->status().ok()) { LOG(ERROR) << "DB iteration failed: " << it->status().ToString(); - return Result(kGenericOnFailureMessage); + return ReadResult(kGenericOnFailureMessage); } - return Result(settings.release(), NULL, NULL); + return ReadResult(settings.release()); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( +ExtensionSettingsStorage::WriteResult ExtensionSettingsLeveldbStorage::Set( const std::string& key, const Value& value) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DictionaryValue settings; @@ -167,45 +167,40 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( return Set(settings); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( +ExtensionSettingsStorage::WriteResult ExtensionSettingsLeveldbStorage::Set( const DictionaryValue& settings) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - scoped_ptr<DictionaryValue> old_settings(new DictionaryValue()); - scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>()); + std::string value_as_json; leveldb::WriteBatch batch; + scoped_ptr<ExtensionSettingChangeList> changes( + new ExtensionSettingChangeList()); - for (DictionaryValue::key_iterator it = settings.begin_keys(); - it != settings.end_keys(); ++it) { + for (DictionaryValue::Iterator it(settings); it.HasNext(); it.Advance()) { scoped_ptr<Value> old_value; - if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) { - return Result(kGenericOnFailureMessage); + if (!ReadFromDb(leveldb::ReadOptions(), it.key(), &old_value)) { + return WriteResult(kGenericOnFailureMessage); } - Value* new_value = NULL; - settings.GetWithoutPathExpansion(*it, &new_value); - if (!old_value.get() || !old_value->Equals(new_value)) { - changed_keys->insert(*it); - base::JSONWriter::Write(new_value, false, &value_as_json); - batch.Put(*it, value_as_json); - } - - if (old_value.get()) { - old_settings->SetWithoutPathExpansion(*it, old_value.release()); + if (!old_value.get() || !old_value->Equals(&it.value())) { + changes->push_back( + ExtensionSettingChange( + it.key(), old_value.release(), it.value().DeepCopy())); + base::JSONWriter::Write(&it.value(), false, &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 Result(kGenericOnFailureMessage); + return WriteResult(kGenericOnFailureMessage); } - return Result( - settings.DeepCopy(), old_settings.release(), changed_keys.release()); + return WriteResult(changes.release()); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( +ExtensionSettingsStorage::WriteResult ExtensionSettingsLeveldbStorage::Remove( const std::string& key) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); std::vector<std::string> keys; @@ -213,23 +208,24 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( return Remove(keys); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( +ExtensionSettingsStorage::WriteResult ExtensionSettingsLeveldbStorage::Remove( const std::vector<std::string>& keys) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - scoped_ptr<DictionaryValue> old_settings(new DictionaryValue()); - scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>()); leveldb::WriteBatch batch; + scoped_ptr<ExtensionSettingChangeList> changes( + new ExtensionSettingChangeList()); + 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 Result(kGenericOnFailureMessage); + return WriteResult(kGenericOnFailureMessage); } if (old_value.get()) { - changed_keys->insert(*it); - old_settings->SetWithoutPathExpansion(*it, old_value.release()); + changes->push_back( + ExtensionSettingChange(*it, old_value.release(), NULL)); batch.Delete(*it); } } @@ -237,50 +233,50 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch); if (!status.ok() && !status.IsNotFound()) { LOG(WARNING) << "DB batch delete failed: " << status.ToString(); - return Result(kGenericOnFailureMessage); + return WriteResult(kGenericOnFailureMessage); } - return Result(NULL, old_settings.release(), changed_keys.release()); + return WriteResult(changes.release()); } -ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() { +ExtensionSettingsStorage::WriteResult ExtensionSettingsLeveldbStorage::Clear() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - scoped_ptr<DictionaryValue> old_settings(new DictionaryValue()); - scoped_ptr<std::set<std::string> > changed_keys(new std::set<std::string>()); + leveldb::ReadOptions options; // All interaction with the db is done on the same thread, so snapshotting // isn't strictly necessary. This is just defensive. leveldb::WriteBatch batch; + scoped_ptr<ExtensionSettingChangeList> changes( + new ExtensionSettingChangeList()); ScopedSnapshot snapshot(db_.get()); options.snapshot = snapshot.get(); scoped_ptr<leveldb::Iterator> it(db_->NewIterator(options)); for (it->SeekToFirst(); it->Valid(); it->Next()) { const std::string key = it->key().ToString(); - const std::string old_value_as_json = it->value().ToString(); - changed_keys->insert(key); + const std::string old_value_json = it->value().ToString(); Value* old_value = - base::JSONReader().JsonToValue(old_value_as_json, false, false); + base::JSONReader().JsonToValue(old_value_json, false, false); if (old_value) { - old_settings->SetWithoutPathExpansion(key, old_value); + changes->push_back(ExtensionSettingChange(key, old_value, NULL)); } else { - LOG(ERROR) << "Invalid JSON in database: " << old_value_as_json; + LOG(ERROR) << "Invalid JSON in database: " << old_value_json; } batch.Delete(key); } if (!it->status().ok()) { LOG(WARNING) << "Clear iteration failed: " << it->status().ToString(); - return Result(kGenericOnFailureMessage); + return WriteResult(kGenericOnFailureMessage); } leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch); if (!status.ok() && !status.IsNotFound()) { LOG(WARNING) << "Clear failed: " << status.ToString(); - return Result(kGenericOnFailureMessage); + return WriteResult(kGenericOnFailureMessage); } - return Result(NULL, old_settings.release(), changed_keys.release()); + return WriteResult(changes.release()); } bool ExtensionSettingsLeveldbStorage::ReadFromDb( diff --git a/chrome/browser/extensions/extension_settings_leveldb_storage.h b/chrome/browser/extensions/extension_settings_leveldb_storage.h index eb5831e..636ab3a 100644 --- a/chrome/browser/extensions/extension_settings_leveldb_storage.h +++ b/chrome/browser/extensions/extension_settings_leveldb_storage.h @@ -32,14 +32,14 @@ class ExtensionSettingsLeveldbStorage : public ExtensionSettingsStorage { virtual ~ExtensionSettingsLeveldbStorage(); // ExtensionSettingsStorage implementation. - virtual Result Get(const std::string& key) OVERRIDE; - virtual Result Get(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Get() OVERRIDE; - virtual Result Set(const std::string& key, const Value& value) OVERRIDE; - virtual Result Set(const DictionaryValue& settings) OVERRIDE; - virtual Result Remove(const std::string& key) OVERRIDE; - virtual Result Remove(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Clear() OVERRIDE; + virtual ReadResult Get(const std::string& key) OVERRIDE; + virtual ReadResult Get(const std::vector<std::string>& keys) OVERRIDE; + virtual ReadResult Get() OVERRIDE; + virtual WriteResult Set(const std::string& key, const Value& value) OVERRIDE; + virtual WriteResult Set(const DictionaryValue& settings) OVERRIDE; + virtual WriteResult Remove(const std::string& key) OVERRIDE; + virtual WriteResult Remove(const std::vector<std::string>& keys) OVERRIDE; + virtual WriteResult Clear() OVERRIDE; private: // Ownership of db is taken. diff --git a/chrome/browser/extensions/extension_settings_observer.h b/chrome/browser/extensions/extension_settings_observer.h index 5acb049..a8f9b82 100644 --- a/chrome/browser/extensions/extension_settings_observer.h +++ b/chrome/browser/extensions/extension_settings_observer.h @@ -7,23 +7,14 @@ #pragma once #include "base/observer_list_threadsafe.h" -#include "chrome/browser/extensions/extension_setting_changes.h" - -class Profile; // Interface for classes that listen to changes to extension settings. class ExtensionSettingsObserver { public: // Called when a list of settings have changed for an extension. - // - // |profile| is the profile of the event originator, or NULL if the event - // didn't originate from a background page. This is so that events can avoid - // being sent back to the background page which made the change. When the - // settings API is exposed to content scripts, this will need to be changed. virtual void OnSettingsChanged( - const Profile* profile, const std::string& extension_id, - const ExtensionSettingChanges& changes) = 0; + const std::string& changes_json) = 0; protected: virtual ~ExtensionSettingsObserver(); diff --git a/chrome/browser/extensions/extension_settings_quota_unittest.cc b/chrome/browser/extensions/extension_settings_quota_unittest.cc index ad8d6cd..57736ed 100644 --- a/chrome/browser/extensions/extension_settings_quota_unittest.cc +++ b/chrome/browser/extensions/extension_settings_quota_unittest.cc @@ -55,8 +55,8 @@ class ExtensionSettingsQuotaTest : public testing::Test { // Returns whether the settings in |storage_| and |delegate_| are the same as // |settings|. bool SettingsEqual(const DictionaryValue& settings) { - return settings.Equals(storage_->Get().GetSettings()) && - settings.Equals(delegate_->Get().GetSettings()); + return settings.Equals(&storage_->Get().settings()) && + settings.Equals(&delegate_->Get().settings()); } // Values with different serialized sizes. @@ -215,7 +215,7 @@ TEST_F(ExtensionSettingsQuotaTest, RemovingNonexistentSettings) { DictionaryValue to_set; to_set.Set("b1", byte_value_16_->DeepCopy()); to_set.Set("b2", byte_value_16_->DeepCopy()); - EXPECT_TRUE(to_set.Equals(storage_->Set(to_set).GetSettings())); + storage_->Set(to_set); settings.Set("b1", byte_value_16_->DeepCopy()); settings.Set("b2", byte_value_16_->DeepCopy()); EXPECT_TRUE(storage_->Set("a", *byte_value_1_).HasError()); @@ -237,7 +237,7 @@ TEST_F(ExtensionSettingsQuotaTest, RemovingNonexistentSettings) { to_set.Clear(); to_set.Set("b1", byte_value_1_->DeepCopy()); to_set.Set("b2", byte_value_1_->DeepCopy()); - EXPECT_TRUE(to_set.Equals(storage_->Set(to_set).GetSettings())); + storage_->Set(to_set); settings.Set("b1", byte_value_1_->DeepCopy()); settings.Set("b2", byte_value_1_->DeepCopy()); storage_->Set("b3", *byte_value_1_); @@ -262,34 +262,38 @@ TEST_F(ExtensionSettingsQuotaTest, Clear) { CreateStorage(40, UINT_MAX, 5); // Test running out of byte quota. - DictionaryValue to_set; - to_set.Set("a", byte_value_16_->DeepCopy()); - to_set.Set("b", byte_value_16_->DeepCopy()); - EXPECT_FALSE(storage_->Set(to_set).HasError()); - EXPECT_TRUE(storage_->Set("c", *byte_value_16_).HasError()); - - EXPECT_FALSE(storage_->Clear().HasError()); - - // (repeat) - EXPECT_FALSE(storage_->Set(to_set).HasError()); - EXPECT_TRUE(storage_->Set("c", *byte_value_16_).HasError()); + { + DictionaryValue to_set; + to_set.Set("a", byte_value_16_->DeepCopy()); + to_set.Set("b", byte_value_16_->DeepCopy()); + EXPECT_FALSE(storage_->Set(to_set).HasError()); + EXPECT_TRUE(storage_->Set("c", *byte_value_16_).HasError()); + + EXPECT_FALSE(storage_->Clear().HasError()); + + // (repeat) + EXPECT_FALSE(storage_->Set(to_set).HasError()); + EXPECT_TRUE(storage_->Set("c", *byte_value_16_).HasError()); + } // Test reaching max keys. storage_->Clear(); - to_set.Clear(); - to_set.Set("a", byte_value_1_->DeepCopy()); - to_set.Set("b", byte_value_1_->DeepCopy()); - to_set.Set("c", byte_value_1_->DeepCopy()); - to_set.Set("d", byte_value_1_->DeepCopy()); - to_set.Set("e", byte_value_1_->DeepCopy()); - EXPECT_FALSE(storage_->Set(to_set).HasError()); - EXPECT_TRUE(storage_->Set("f", *byte_value_1_).HasError()); - - storage_->Clear(); - - // (repeat) - EXPECT_FALSE(storage_->Set(to_set).HasError()); - EXPECT_TRUE(storage_->Set("f", *byte_value_1_).HasError()); + { + DictionaryValue to_set; + to_set.Set("a", byte_value_1_->DeepCopy()); + to_set.Set("b", byte_value_1_->DeepCopy()); + to_set.Set("c", byte_value_1_->DeepCopy()); + to_set.Set("d", byte_value_1_->DeepCopy()); + to_set.Set("e", byte_value_1_->DeepCopy()); + EXPECT_FALSE(storage_->Set(to_set).HasError()); + EXPECT_TRUE(storage_->Set("f", *byte_value_1_).HasError()); + + storage_->Clear(); + + // (repeat) + EXPECT_FALSE(storage_->Set(to_set).HasError()); + EXPECT_TRUE(storage_->Set("f", *byte_value_1_).HasError()); + } } TEST_F(ExtensionSettingsQuotaTest, ChangingUsedBytesWithSet) { diff --git a/chrome/browser/extensions/extension_settings_storage.cc b/chrome/browser/extensions/extension_settings_storage.cc index 3259748..cb464c3 100644 --- a/chrome/browser/extensions/extension_settings_storage.cc +++ b/chrome/browser/extensions/extension_settings_storage.cc @@ -2,80 +2,75 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "base/logging.h" #include "chrome/browser/extensions/extension_settings_storage.h" -// Implementation of ExtensionSettingsStorage::Result +#include "base/logging.h" + +// Implementation of ReadResult. -ExtensionSettingsStorage::Result::Result( - DictionaryValue* settings, - DictionaryValue* old_settings, - std::set<std::string>* changed_keys) - : inner_(new Inner(settings, old_settings, changed_keys, std::string())) {} +ExtensionSettingsStorage::ReadResult::ReadResult(DictionaryValue* settings) + : inner_(new Inner(settings, "")) { + DCHECK(settings); +} -ExtensionSettingsStorage::Result::Result(const std::string& error) - : inner_(new Inner(NULL, NULL, new std::set<std::string>(), error)) { +ExtensionSettingsStorage::ReadResult::ReadResult(const std::string& error) + : inner_(new Inner(NULL, error)) { DCHECK(!error.empty()); } -ExtensionSettingsStorage::Result::~Result() {} +ExtensionSettingsStorage::ReadResult::~ReadResult() {} -const DictionaryValue* ExtensionSettingsStorage::Result::GetSettings() const { - DCHECK(!HasError()); - return inner_->settings_.get(); +bool ExtensionSettingsStorage::ReadResult::HasError() const { + return !inner_->error_.empty(); } -const std::set<std::string>* -ExtensionSettingsStorage::Result::GetChangedKeys() const { +const DictionaryValue& ExtensionSettingsStorage::ReadResult::settings() const { DCHECK(!HasError()); - return inner_->changed_keys_.get(); + return *inner_->settings_; } -const Value* ExtensionSettingsStorage::Result::GetOldValue( - const std::string& key) const { - DCHECK(!HasError()); - if (!inner_->changed_keys_.get()) { - return NULL; - } - Value* old_value = NULL; - if (inner_->changed_keys_->count(key)) { - inner_->old_settings_->GetWithoutPathExpansion(key, &old_value); - } else if (inner_->settings_.get()) { - inner_->settings_->GetWithoutPathExpansion(key, &old_value); - } - return old_value; +const std::string& ExtensionSettingsStorage::ReadResult::error() const { + DCHECK(HasError()); + return inner_->error_; } -const Value* ExtensionSettingsStorage::Result::GetNewValue( - const std::string& key) const { - DCHECK(!HasError()); - if (!inner_->changed_keys_.get()) { - return NULL; - } - Value* new_value = NULL; - if (inner_->settings_.get()) { - inner_->settings_->GetWithoutPathExpansion(key, &new_value); - } - return new_value; +ExtensionSettingsStorage::ReadResult::Inner::Inner( + DictionaryValue* settings, const std::string& error) + : settings_(settings), error_(error) {} + +ExtensionSettingsStorage::ReadResult::Inner::~Inner() {} + +// Implementation of WriteResult. + +ExtensionSettingsStorage::WriteResult::WriteResult( + ExtensionSettingChangeList* changes) : inner_(new Inner(changes, "")) { + DCHECK(changes); +} + +ExtensionSettingsStorage::WriteResult::WriteResult(const std::string& error) + : inner_(new Inner(NULL, error)) { + DCHECK(!error.empty()); } -bool ExtensionSettingsStorage::Result::HasError() const { +ExtensionSettingsStorage::WriteResult::~WriteResult() {} + +bool ExtensionSettingsStorage::WriteResult::HasError() const { return !inner_->error_.empty(); } -const std::string& ExtensionSettingsStorage::Result::GetError() const { +const ExtensionSettingChangeList& +ExtensionSettingsStorage::WriteResult::changes() const { + DCHECK(!HasError()); + return *inner_->changes_; +} + +const std::string& ExtensionSettingsStorage::WriteResult::error() const { DCHECK(HasError()); return inner_->error_; } -ExtensionSettingsStorage::Result::Inner::Inner( - DictionaryValue* settings, - DictionaryValue* old_settings, - std::set<std::string>* changed_keys, - const std::string& error) - : settings_(settings), - old_settings_(old_settings), - changed_keys_(changed_keys), - error_(error) {} - -ExtensionSettingsStorage::Result::Inner::~Inner() {} +ExtensionSettingsStorage::WriteResult::Inner::Inner( + ExtensionSettingChangeList* changes, const std::string& error) + : changes_(changes), error_(error) {} + +ExtensionSettingsStorage::WriteResult::Inner::~Inner() {} diff --git a/chrome/browser/extensions/extension_settings_storage.h b/chrome/browser/extensions/extension_settings_storage.h index 106f8fb..c13c5d0 100644 --- a/chrome/browser/extensions/extension_settings_storage.h +++ b/chrome/browser/extensions/extension_settings_storage.h @@ -11,71 +11,77 @@ #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/values.h" +#include "chrome/browser/extensions/extension_setting_change.h" // Interface for extension settings storage classes. // -// All methods *must* be run on the FILE thread, inclusing construction and -// destructions. +// All methods *must* be run on the FILE thread, including construction and +// destruction. class ExtensionSettingsStorage { public: - // The result of an operation. - // - // Supports lightweight copying. - class Result { + // The result of a read operation (Get). Safe/efficient to copy. + class ReadResult { public: - // Ownership of all arguments are taken. - // |settings| may be NULL when the result is for an operation which - // generates no setting values (e.g. Remove(), Clear()). - // |changed_keys| and |old_settings| may be NULL when the result is for an - // operation which cannot change settings (e.g. Get()). - Result( - DictionaryValue* settings, - DictionaryValue* old_settings, - std::set<std::string>* changed_keys); - explicit Result(const std::string& error); - ~Result(); - - // The dictionary result of the computation. NULL does not imply invalid; - // HasError() should be used to test this. - // Ownership remains with this. - const DictionaryValue* GetSettings() const; - - // Gets the set of setting keys which changed as a result of the - // computation. This includes all settings that existed and removed, all - // settings which changed when set, and all setting keys cleared. May be - // NULL for operations which cannot change settings, such as Get(). - const std::set<std::string>* GetChangedKeys() const; - - // Gets the value of a setting prior to the operation which generated - // this Result, or NULL if the setting had no original value or this Result - // is from an operation that didn't modify the settings (such as Get()). - // Ownerships remains with this. - const Value* GetOldValue(const std::string& key) const; - - // Like GetOldValue, but gets the new value. - const Value* GetNewValue(const std::string& key) const; - - // Whether there was an error in the computation. If so, the results of - // GetSettings and ReleaseSettings are not valid. + // Ownership of |settings| taken. + explicit ReadResult(DictionaryValue* settings); + explicit ReadResult(const std::string& error); + ~ReadResult(); + + // Gets the settings read from the storage. + // Must only be called if HasError() is false. + const DictionaryValue& settings() const; + + // Gets whether the operation failed. bool HasError() const; - // Gets the error message, if any. - const std::string& GetError() const; + // Gets the error message describing the failure. + // Must only be called if HasError() is true. + const std::string& error() const; private: - struct Inner : public base::RefCountedThreadSafe<Inner> { - // Empty error implies no error. - Inner( - DictionaryValue* settings, - DictionaryValue* old_settings, - std::set<std::string>* changed_keys, - const std::string& error); - ~Inner(); - + class Inner : public base::RefCountedThreadSafe<Inner> { + public: + Inner(DictionaryValue* settings, const std::string& error); const scoped_ptr<DictionaryValue> settings_; - const scoped_ptr<DictionaryValue> old_settings_; - const scoped_ptr<std::set<std::string> > changed_keys_; const std::string error_; + + private: + friend class base::RefCountedThreadSafe<Inner>; + virtual ~Inner(); + }; + + scoped_refptr<Inner> inner_; + }; + + // The result of a write operation (Set/Remove/Clear). Safe/efficient to copy. + class WriteResult { + public: + // Ownership of |changes| taken. + explicit WriteResult(ExtensionSettingChangeList* changes); + explicit WriteResult(const std::string& error); + ~WriteResult(); + + // Gets the list of changes to the settings which resulted from the write. + // Must only be called if HasError() is false. + const ExtensionSettingChangeList& changes() const; + + // Gets whether the operation failed. + bool HasError() const; + + // Gets the error message describing the failure. + // Must only be called if HasError() is true. + const std::string& error() const; + + private: + class Inner : public base::RefCountedThreadSafe<Inner> { + public: + Inner(ExtensionSettingChangeList* changes, const std::string& error); + const scoped_ptr<ExtensionSettingChangeList> changes_; + const std::string error_; + + private: + friend class base::RefCountedThreadSafe<Inner>; + virtual ~Inner(); }; scoped_refptr<Inner> inner_; @@ -84,36 +90,28 @@ class ExtensionSettingsStorage { virtual ~ExtensionSettingsStorage() {} // Gets a single value from storage. - // If successful, result maps the key to its value. - virtual Result Get(const std::string& key) = 0; + virtual ReadResult Get(const std::string& key) = 0; // Gets multiple values from storage. - // If successful, result maps each key to its value. - virtual Result Get(const std::vector<std::string>& keys) = 0; + virtual ReadResult Get(const std::vector<std::string>& keys) = 0; // Gets all values from storage. - // If successful, result maps every key to its value. - virtual Result Get() = 0; + virtual ReadResult Get() = 0; // Sets a single key to a new value. - // If successful, result maps the key to the given value. - virtual Result Set(const std::string& key, const Value& value) = 0; + virtual WriteResult Set(const std::string& key, const Value& value) = 0; // Sets multiple keys to new values. - // If successful, result is identical to the given dictionary. - virtual Result Set(const DictionaryValue& values) = 0; + virtual WriteResult Set(const DictionaryValue& values) = 0; // Removes a key from the storage. - // If successful, result value is NULL. - virtual Result Remove(const std::string& key) = 0; + virtual WriteResult Remove(const std::string& key) = 0; // Removes multiple keys from the storage. - // If successful, result value is NULL. - virtual Result Remove(const std::vector<std::string>& keys) = 0; + virtual WriteResult Remove(const std::vector<std::string>& keys) = 0; // Clears the storage. - // If successful, result value is NULL. - virtual Result Clear() = 0; + virtual WriteResult Clear() = 0; }; #endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_STORAGE_H_ diff --git a/chrome/browser/extensions/extension_settings_storage_cache.cc b/chrome/browser/extensions/extension_settings_storage_cache.cc index 2df99ed..0c74043 100644 --- a/chrome/browser/extensions/extension_settings_storage_cache.cc +++ b/chrome/browser/extensions/extension_settings_storage_cache.cc @@ -9,25 +9,25 @@ ExtensionSettingsStorageCache::ExtensionSettingsStorageCache( ExtensionSettingsStorageCache::~ExtensionSettingsStorageCache() {} -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsStorageCache::Get( const std::string& key) { Value *value; if (GetFromCache(key, &value)) { DictionaryValue* settings = new DictionaryValue(); settings->SetWithoutPathExpansion(key, value); - return Result(settings, NULL, NULL); + return ReadResult(settings); } - Result result = delegate_->Get(key); + ReadResult result = delegate_->Get(key); if (result.HasError()) { return result; } - cache_.MergeDictionary(result.GetSettings()); + cache_.MergeDictionary(&result.settings()); return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsStorageCache::Get( const std::vector<std::string>& keys) { scoped_ptr<DictionaryValue> from_cache(new DictionaryValue()); std::vector<std::string> missing_keys; @@ -43,84 +43,78 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( } if (missing_keys.empty()) { - return Result(from_cache.release(), NULL, NULL); + return ReadResult(from_cache.release()); } - Result result = delegate_->Get(keys); + ReadResult result = delegate_->Get(missing_keys); if (result.HasError()) { return result; } - cache_.MergeDictionary(result.GetSettings()); - - DictionaryValue* settings_with_cache = result.GetSettings()->DeepCopy(); - settings_with_cache->MergeDictionary(from_cache.get()); - return Result(settings_with_cache, NULL, NULL); + cache_.MergeDictionary(&result.settings()); + from_cache->MergeDictionary(&result.settings()); + return ReadResult(from_cache.release()); } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get() { - Result result = delegate_->Get(); +ExtensionSettingsStorage::ReadResult ExtensionSettingsStorageCache::Get() { + ReadResult result = delegate_->Get(); if (!result.HasError()) { - cache_.MergeDictionary(result.GetSettings()); + cache_.MergeDictionary(&result.settings()); } return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Set( +ExtensionSettingsStorage::WriteResult ExtensionSettingsStorageCache::Set( const std::string& key, const Value& value) { - Result result = delegate_->Set(key, value); + WriteResult result = delegate_->Set(key, value); if (!result.HasError()) { - cache_.MergeDictionary(result.GetSettings()); + cache_.SetWithoutPathExpansion(key, value.DeepCopy()); } return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Set( +ExtensionSettingsStorage::WriteResult ExtensionSettingsStorageCache::Set( const DictionaryValue& settings) { - Result result = delegate_->Set(settings); + WriteResult result = delegate_->Set(settings); if (result.HasError()) { return result; } - const std::set<std::string>* changed_keys = result.GetChangedKeys(); - DCHECK(changed_keys); - for (std::set<std::string>::const_iterator it = changed_keys->begin(); - it != changed_keys->end(); ++it) { - Value* new_value = NULL; - result.GetSettings()->GetWithoutPathExpansion(*it, &new_value); - DCHECK(new_value); - cache_.SetWithoutPathExpansion(*it, new_value->DeepCopy()); + for (ExtensionSettingChangeList::const_iterator it = result.changes().begin(); + it != result.changes().end(); ++it) { + DCHECK(it->new_value()); + cache_.SetWithoutPathExpansion(it->key(), it->new_value()->DeepCopy()); } + return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Remove( +ExtensionSettingsStorage::WriteResult ExtensionSettingsStorageCache::Remove( const std::string& key) { - Result result = delegate_->Remove(key); + WriteResult result = delegate_->Remove(key); if (!result.HasError()) { cache_.RemoveWithoutPathExpansion(key, NULL); } return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Remove( +ExtensionSettingsStorage::WriteResult ExtensionSettingsStorageCache::Remove( const std::vector<std::string>& keys) { - Result result = delegate_->Remove(keys); + WriteResult result = delegate_->Remove(keys); if (result.HasError()) { return result; } - const std::set<std::string>* changed_keys = result.GetChangedKeys(); - DCHECK(changed_keys); - for (std::set<std::string>::const_iterator it = changed_keys->begin(); - it != changed_keys->end(); ++it) { - cache_.RemoveWithoutPathExpansion(*it, NULL); + for (ExtensionSettingChangeList::const_iterator it = result.changes().begin(); + it != result.changes().end(); ++it) { + cache_.RemoveWithoutPathExpansion(it->key(), NULL); } + return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Clear() { - Result result = delegate_->Clear(); +ExtensionSettingsStorage::WriteResult ExtensionSettingsStorageCache::Clear() { + WriteResult result = delegate_->Clear(); if (!result.HasError()) { cache_.Clear(); } diff --git a/chrome/browser/extensions/extension_settings_storage_cache.h b/chrome/browser/extensions/extension_settings_storage_cache.h index 3e4e5d1..b318130 100644 --- a/chrome/browser/extensions/extension_settings_storage_cache.h +++ b/chrome/browser/extensions/extension_settings_storage_cache.h @@ -25,14 +25,14 @@ class ExtensionSettingsStorageCache : public ExtensionSettingsStorage { virtual ~ExtensionSettingsStorageCache(); // ExtensionSettingsStorage implementation. - virtual Result Get(const std::string& key) OVERRIDE; - virtual Result Get(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Get() OVERRIDE; - virtual Result Set(const std::string& key, const Value& value) OVERRIDE; - virtual Result Set(const DictionaryValue& settings) OVERRIDE; - virtual Result Remove(const std::string& key) OVERRIDE; - virtual Result Remove(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Clear() OVERRIDE; + virtual ReadResult Get(const std::string& key) OVERRIDE; + virtual ReadResult Get(const std::vector<std::string>& keys) OVERRIDE; + virtual ReadResult Get() OVERRIDE; + virtual WriteResult Set(const std::string& key, const Value& value) OVERRIDE; + virtual WriteResult Set(const DictionaryValue& settings) OVERRIDE; + virtual WriteResult Remove(const std::string& key) OVERRIDE; + virtual WriteResult Remove(const std::vector<std::string>& keys) OVERRIDE; + virtual WriteResult Clear() OVERRIDE; private: // Returns whether the value was found in the cache. diff --git a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc index 6186c87..246f224 100644 --- a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc +++ b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc @@ -51,7 +51,7 @@ void Free( } // Returns an error result and logs the quota exceeded to UMA. -ExtensionSettingsStorage::Result QuotaExceededFor(Resource resource) { +ExtensionSettingsStorage::WriteResult QuotaExceededFor(Resource resource) { switch (resource) { case TOTAL_BYTES: UMA_HISTOGRAM_COUNTS_100( @@ -68,7 +68,7 @@ ExtensionSettingsStorage::Result QuotaExceededFor(Resource resource) { default: NOTREACHED(); } - return ExtensionSettingsStorage::Result(kExceededQuotaErrorMessage); + return ExtensionSettingsStorage::WriteResult(kExceededQuotaErrorMessage); } } // namespace @@ -83,41 +83,39 @@ ExtensionSettingsStorageQuotaEnforcer::ExtensionSettingsStorageQuotaEnforcer( max_keys_(max_keys), delegate_(delegate), used_total_(0) { - Result maybe_initial_settings = delegate->Get(); - if (maybe_initial_settings.HasError()) { + ReadResult maybe_settings = delegate->Get(); + if (maybe_settings.HasError()) { LOG(WARNING) << "Failed to get initial settings for quota: " << - maybe_initial_settings.GetError(); + maybe_settings.error(); return; } - const DictionaryValue* initial_settings = - maybe_initial_settings.GetSettings(); - for (DictionaryValue::key_iterator it = initial_settings->begin_keys(); - it != initial_settings->end_keys(); ++it) { - Value *value; - initial_settings->GetWithoutPathExpansion(*it, &value); - Allocate(*it, *value, &used_total_, &used_per_setting_); + for (DictionaryValue::Iterator it(maybe_settings.settings()); it.HasNext(); + it.Advance()) { + Allocate(it.key(), it.value(), &used_total_, &used_per_setting_); } } ExtensionSettingsStorageQuotaEnforcer::~ExtensionSettingsStorageQuotaEnforcer( ) {} -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsStorageQuotaEnforcer::Get( const std::string& key) { return delegate_->Get(key); } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Get( +ExtensionSettingsStorage::ReadResult ExtensionSettingsStorageQuotaEnforcer::Get( const std::vector<std::string>& keys) { return delegate_->Get(keys); } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Get() { +ExtensionSettingsStorage::ReadResult +ExtensionSettingsStorageQuotaEnforcer::Get() { return delegate_->Get(); } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( +ExtensionSettingsStorage::WriteResult +ExtensionSettingsStorageQuotaEnforcer::Set( const std::string& key, const Value& value) { size_t new_used_total = used_total_; std::map<std::string, size_t> new_used_per_setting = used_per_setting_; @@ -133,7 +131,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( return QuotaExceededFor(KEY_COUNT); } - Result result = delegate_->Set(key, value); + WriteResult result = delegate_->Set(key, value); if (result.HasError()) { return result; } @@ -143,17 +141,14 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( +ExtensionSettingsStorage::WriteResult +ExtensionSettingsStorageQuotaEnforcer::Set( const DictionaryValue& values) { size_t new_used_total = used_total_; std::map<std::string, size_t> new_used_per_setting = used_per_setting_; - for (DictionaryValue::key_iterator it = values.begin_keys(); - it != values.end_keys(); ++it) { - Value* value; - values.GetWithoutPathExpansion(*it, &value); - Allocate(*it, *value, &new_used_total, &new_used_per_setting); - - if (new_used_per_setting[*it] > quota_bytes_per_setting_) { + for (DictionaryValue::Iterator it(values); it.HasNext(); it.Advance()) { + Allocate(it.key(), it.value(), &new_used_total, &new_used_per_setting); + if (new_used_per_setting[it.key()] > quota_bytes_per_setting_) { return QuotaExceededFor(BYTES_PER_SETTING); } } @@ -165,7 +160,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( return QuotaExceededFor(KEY_COUNT); } - Result result = delegate_->Set(values); + WriteResult result = delegate_->Set(values); if (result.HasError()) { return result; } @@ -175,9 +170,10 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Set( return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Remove( +ExtensionSettingsStorage::WriteResult +ExtensionSettingsStorageQuotaEnforcer::Remove( const std::string& key) { - Result result = delegate_->Remove(key); + WriteResult result = delegate_->Remove(key); if (result.HasError()) { return result; } @@ -185,9 +181,10 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Remove( return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Remove( +ExtensionSettingsStorage::WriteResult +ExtensionSettingsStorageQuotaEnforcer::Remove( const std::vector<std::string>& keys) { - Result result = delegate_->Remove(keys); + WriteResult result = delegate_->Remove(keys); if (result.HasError()) { return result; } @@ -199,9 +196,10 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Remove( return result; } -ExtensionSettingsStorage::Result ExtensionSettingsStorageQuotaEnforcer::Clear( +ExtensionSettingsStorage::WriteResult +ExtensionSettingsStorageQuotaEnforcer::Clear( ) { - Result result = delegate_->Clear(); + WriteResult result = delegate_->Clear(); if (result.HasError()) { return result; } diff --git a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.h b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.h index 2e56ba7..582c0bf 100644 --- a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.h +++ b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.h @@ -24,14 +24,14 @@ class ExtensionSettingsStorageQuotaEnforcer : public ExtensionSettingsStorage { virtual ~ExtensionSettingsStorageQuotaEnforcer(); // ExtensionSettingsStorage implementation. - virtual Result Get(const std::string& key) OVERRIDE; - virtual Result Get(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Get() OVERRIDE; - virtual Result Set(const std::string& key, const Value& value) OVERRIDE; - virtual Result Set(const DictionaryValue& settings) OVERRIDE; - virtual Result Remove(const std::string& key) OVERRIDE; - virtual Result Remove(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Clear() OVERRIDE; + virtual ReadResult Get(const std::string& key) OVERRIDE; + virtual ReadResult Get(const std::vector<std::string>& keys) OVERRIDE; + virtual ReadResult Get() OVERRIDE; + virtual WriteResult Set(const std::string& key, const Value& value) OVERRIDE; + virtual WriteResult Set(const DictionaryValue& settings) OVERRIDE; + virtual WriteResult Remove(const std::string& key) OVERRIDE; + virtual WriteResult Remove(const std::vector<std::string>& keys) OVERRIDE; + virtual WriteResult Clear() OVERRIDE; private: // The storage quota in bytes. diff --git a/chrome/browser/extensions/extension_settings_storage_unittest.cc b/chrome/browser/extensions/extension_settings_storage_unittest.cc index 4bb04ee..73e1940 100644 --- a/chrome/browser/extensions/extension_settings_storage_unittest.cc +++ b/chrome/browser/extensions/extension_settings_storage_unittest.cc @@ -58,60 +58,74 @@ bool ValuesEqual( return true; } -// Returns whether the result of a storage operation has the expected settings -// and changed keys. +// Returns whether the read result of a storage operation has the expected +// settings. testing::AssertionResult SettingsEq( - const char* _1, const char* _2, const char* _3, const char* _4, - DictionaryValue* expected_settings, - DictionaryValue* expected_old_settings, - std::set<std::string>* expected_changed_keys, - ExtensionSettingsStorage::Result actual) { - std::string error; - - if (actual.HasError()) { + const char* _1, const char* _2, + const DictionaryValue& expected, + const ExtensionSettingsStorage::ReadResult& actual_result) { + if (actual_result.HasError()) { return testing::AssertionFailure() << - "Expected: " << GetJSON(*expected_settings) << - ", " << ToString(*expected_changed_keys) << "\n" << - ", actual has error: " << actual.GetError(); + "Result has error: " << actual_result.error(); } - if (!ValuesEqual(expected_settings, actual.GetSettings(), &error)) { - return testing::AssertionFailure() << "For settings, " << error; + std::string error; + if (!ValuesEqual(&expected, &actual_result.settings(), &error)) { + return testing::AssertionFailure() << error; } - if (!expected_changed_keys && actual.GetChangedKeys()) { + return testing::AssertionSuccess(); +} + +// Returns whether the write result of a storage operation has the expected +// changes. +testing::AssertionResult ChangesEq( + const char* _1, const char* _2, + const ExtensionSettingChangeList& expected, + const ExtensionSettingsStorage::WriteResult& actual_result) { + if (actual_result.HasError()) { return testing::AssertionFailure() << - "Expected NULL changed keys, actual: " << - ToString(*actual.GetChangedKeys()); + "Result has error: " << actual_result.error(); } - if (expected_changed_keys && !actual.GetChangedKeys()) { + + const ExtensionSettingChangeList& actual = actual_result.changes(); + if (expected.size() != actual.size()) { return testing::AssertionFailure() << - "Expected: " << ToString(*expected_changed_keys) << ", actual NULL"; + "Actual has wrong size, expecting " << expected.size() << + " but was " << actual.size(); } - if (expected_changed_keys != actual.GetChangedKeys() && - *expected_changed_keys != *actual.GetChangedKeys()) { - return testing::AssertionFailure() << - "Expected: " << ToString(*expected_changed_keys) << - ", actual: " << ToString(*actual.GetChangedKeys()); - } - - const std::set<std::string>* changed_keys = actual.GetChangedKeys(); - if (changed_keys) { - for (std::set<std::string>::const_iterator it = changed_keys->begin(); - it != changed_keys->end(); ++it) { - Value *expected_old_value = NULL; - expected_old_settings->GetWithoutPathExpansion(*it, &expected_old_value); - if (!ValuesEqual(expected_old_value, actual.GetOldValue(*it), &error)) { - return testing::AssertionFailure() << "Old values not equal: " << error; - } - - Value *expected_new_value = NULL; - if (expected_settings) { - expected_settings->GetWithoutPathExpansion(*it, &expected_new_value); - } - if (!ValuesEqual(expected_new_value, actual.GetNewValue(*it), &error)) { - return testing::AssertionFailure() << "New values not equal: " << error; - } + + std::map<std::string, linked_ptr<ExtensionSettingChange> > expected_as_map; + for (ExtensionSettingChangeList::const_iterator it = expected.begin(); + it != expected.end(); ++it) { + expected_as_map[it->key()] = + linked_ptr<ExtensionSettingChange>(new ExtensionSettingChange(*it)); + } + + std::set<std::string> keys_seen; + + for (ExtensionSettingChangeList::const_iterator it = actual.begin(); + it != actual.end(); ++it) { + if (keys_seen.count(it->key())) { + return testing::AssertionFailure() << + "Multiple changes seen for key: " << it->key(); + } + keys_seen.insert(it->key()); + + if (!expected_as_map.count(it->key())) { + return testing::AssertionFailure() << + "Actual has unexpected change for key: " << it->key(); + } + + ExtensionSettingChange expected_change = *expected_as_map[it->key()]; + std::string error; + if (!ValuesEqual(expected_change.new_value(), it->new_value(), &error)) { + return testing::AssertionFailure() << + "New value for " << it->key() << " was unexpected: " << error; + } + if (!ValuesEqual(expected_change.old_value(), it->old_value(), &error)) { + return testing::AssertionFailure() << + "Old value for " << it->key() << " was unexpected: " << error; } } @@ -173,179 +187,143 @@ void ExtensionSettingsStorageTest::TearDown() { } TEST_P(ExtensionSettingsStorageTest, GetWhenEmpty) { - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, GetWithSingleValue) { - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), empty_dict_.get(), &set1_, storage_->Set(key1_, *val1_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key2_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get()); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, NULL, val1_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(key1_, *val1_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key2_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key3_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, GetWithMultipleValues) { - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), empty_dict_.get(), &set12_, storage_->Set(*dict12_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get()); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, NULL, val1_->DeepCopy())); + changes.push_back(ExtensionSettingChange(key2_, NULL, val2_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(*dict12_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key3_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, RemoveWhenEmpty) { - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Remove(key1_)); + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, RemoveWithSingleValue) { - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), empty_dict_.get(), &set1_, storage_->Set(*dict1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, dict1_.get(), &set1_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key2_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + storage_->Set(*dict1_); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val1_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(key1_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key2_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list12_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, RemoveWithMultipleValues) { - EXPECT_PRED_FORMAT4(SettingsEq, - dict123_.get(), empty_dict_.get(), &set123_, storage_->Set(*dict123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, dict3_.get(), &set3_, storage_->Remove(key3_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get()); - - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, dict12_.get(), &set12_, storage_->Remove(list12_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + storage_->Set(*dict123_); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key3_, val3_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(key3_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key3_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(list1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get(list12_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(list13_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get()); + + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val1_->DeepCopy(), NULL)); + changes.push_back(ExtensionSettingChange(key2_, val2_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(list12_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key3_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list12_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list13_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, SetWhenOverwriting) { - DictionaryValue key1_val2; - key1_val2.Set(key1_, val2_->DeepCopy()); - - EXPECT_PRED_FORMAT4(SettingsEq, - &key1_val2, empty_dict_.get(), &set1_, storage_->Set(key1_, *val2_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), &key1_val2, &set12_, storage_->Set(*dict12_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), NULL, NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), NULL, NULL, storage_->Get()); + storage_->Set(key1_, *val2_); + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(key1_, val2_->DeepCopy(), val1_->DeepCopy())); + changes.push_back(ExtensionSettingChange(key2_, NULL, val2_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(*dict12_)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key3_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(list1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get(list12_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict1_, storage_->Get(list13_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *dict12_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, ClearWhenEmpty) { - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Clear()); - - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Clear()); + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, ClearWhenNotEmpty) { - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), empty_dict_.get(), &set12_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, dict12_.get(), &set12_, storage_->Clear()); - - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + storage_->Set(*dict12_); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val1_->DeepCopy(), NULL)); + changes.push_back(ExtensionSettingChange(key2_, val2_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Clear()); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(key1_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(empty_list_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(list123_)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } // Dots should be allowed in key names; they shouldn't be interpreted as @@ -355,56 +333,66 @@ TEST_P(ExtensionSettingsStorageTest, DotsInKeyNames) { StringValue dot_value("baz.qux"); std::vector<std::string> dot_list; dot_list.push_back(dot_key); - std::set<std::string> dot_set; - dot_set.insert(dot_key); DictionaryValue dot_dict; dot_dict.SetWithoutPathExpansion(dot_key, dot_value.DeepCopy()); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(dot_key)); - - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, empty_dict_.get(), &dot_set, - storage_->Set(dot_key, dot_value)); - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, &dot_dict, &empty_set_, - storage_->Set(dot_key, dot_value)); - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, NULL, NULL, storage_->Get(dot_key)); - - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, &dot_dict, &dot_set, storage_->Remove(dot_key)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Remove(dot_key)); - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, empty_dict_.get(), &dot_set, storage_->Set(dot_dict)); - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, NULL, NULL, storage_->Get(dot_list)); - EXPECT_PRED_FORMAT4(SettingsEq, - &dot_dict, NULL, NULL, storage_->Get()); - - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, &dot_dict, &dot_set, storage_->Remove(dot_list)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get(dot_key)); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get()); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(dot_key)); + + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(dot_key, NULL, dot_value.DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(dot_key, dot_value)); + } + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Set(dot_key, dot_value)); + + EXPECT_PRED_FORMAT2(SettingsEq, dot_dict, storage_->Get(dot_key)); + + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(dot_key, dot_value.DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(dot_key)); + } + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Remove(dot_key)); + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(dot_key, NULL, dot_value.DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(dot_dict)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, dot_dict, storage_->Get(dot_list)); + EXPECT_PRED_FORMAT2(SettingsEq, dot_dict, storage_->Get()); + + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(dot_key, dot_value.DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(dot_list)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get(dot_key)); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get()); } TEST_P(ExtensionSettingsStorageTest, DotsInKeyNamesWithDicts) { DictionaryValue outer_dict; DictionaryValue* inner_dict = new DictionaryValue(); outer_dict.Set("foo", inner_dict); - inner_dict->Set("bar", Value::CreateStringValue("baz")); - std::set<std::string> changed_keys; - changed_keys.insert("foo"); - - EXPECT_PRED_FORMAT4(SettingsEq, - &outer_dict, empty_dict_.get(), &changed_keys, storage_->Set(outer_dict)); - EXPECT_PRED_FORMAT4(SettingsEq, - &outer_dict, NULL, NULL, storage_->Get("foo")); - EXPECT_PRED_FORMAT4(SettingsEq, - empty_dict_.get(), NULL, NULL, storage_->Get("foo.bar")); + inner_dict->SetString("bar", "baz"); + + { + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange("foo", NULL, inner_dict->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(outer_dict)); + } + + EXPECT_PRED_FORMAT2(SettingsEq, outer_dict, storage_->Get("foo")); + EXPECT_PRED_FORMAT2(SettingsEq, *empty_dict_, storage_->Get("foo.bar")); } TEST_P(ExtensionSettingsStorageTest, ComplexChangedKeysScenarios) { @@ -413,84 +401,87 @@ TEST_P(ExtensionSettingsStorageTest, ComplexChangedKeysScenarios) { // - Removing over missing and present keys, combinations. // - Clearing. std::vector<std::string> complex_list; - std::set<std::string> complex_set; - DictionaryValue complex_dict; DictionaryValue complex_changed_dict; - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), empty_dict_.get(), &set1_, storage_->Set(key1_, *val1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), dict1_.get(), &empty_set_, storage_->Set(key1_, *val1_)); - - complex_dict.Clear(); - complex_dict.Set(key1_, val2_->DeepCopy()); - EXPECT_PRED_FORMAT4(SettingsEq, - &complex_dict, dict1_.get(), &set1_, storage_->Set(key1_, *val2_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, &complex_dict, &set1_, storage_->Remove(key1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict1_.get(), empty_dict_.get(), &set1_, storage_->Set(key1_, *val1_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, dict1_.get(), &set1_, storage_->Clear()); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Clear()); - - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), empty_dict_.get(), &set12_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict12_.get(), dict12_.get(), &empty_set_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - dict123_.get(), dict12_.get(), &set3_, storage_->Set(*dict123_)); - - complex_dict.Clear(); - complex_dict.Set(key1_, val2_->DeepCopy()); - complex_dict.Set(key2_, val2_->DeepCopy()); - complex_dict.Set("asdf", val1_->DeepCopy()); - complex_dict.Set("qwerty", val3_->DeepCopy()); - complex_changed_dict.Clear(); - complex_changed_dict.Set(key1_, val1_->DeepCopy()); - complex_changed_dict.Set(key2_, val2_->DeepCopy()); - complex_set.clear(); - complex_set.insert(key1_); - complex_set.insert("asdf"); - complex_set.insert("qwerty"); - EXPECT_PRED_FORMAT4(SettingsEq, - &complex_dict, &complex_changed_dict, &complex_set, - storage_->Set(complex_dict)); - - complex_changed_dict.Clear(); - complex_changed_dict.Set(key1_, val2_->DeepCopy()); - complex_changed_dict.Set(key2_, val2_->DeepCopy()); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, &complex_changed_dict, &set12_, storage_->Remove(list12_)); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Remove(list12_)); - - complex_list.clear(); - complex_list.push_back(key1_); - complex_list.push_back("asdf"); - complex_changed_dict.Clear(); - complex_changed_dict.Set("asdf", val1_->DeepCopy()); - complex_set.clear(); - complex_set.insert("asdf"); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, - &complex_changed_dict, - &complex_set, - storage_->Remove(complex_list)); - - complex_changed_dict.Clear(); - complex_changed_dict.Set(key3_, val3_->DeepCopy()); - complex_changed_dict.Set("qwerty", val3_->DeepCopy()); - complex_set.clear(); - complex_set.insert(key3_); - complex_set.insert("qwerty"); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, &complex_changed_dict, &complex_set, storage_->Clear()); - EXPECT_PRED_FORMAT4(SettingsEq, - NULL, empty_dict_.get(), &empty_set_, storage_->Clear()); + storage_->Set(key1_, *val1_); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Set(key1_, *val1_)); + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange( + key1_, val1_->DeepCopy(), val2_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(key1_, *val2_)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val2_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(key1_)); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Remove(key1_)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, NULL, val1_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(key1_, *val1_)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val1_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Clear()); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Clear()); + } + + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, NULL, val1_->DeepCopy())); + changes.push_back(ExtensionSettingChange(key2_, NULL, val2_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(*dict12_)); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Set(*dict12_)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key3_, NULL, val3_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(*dict123_)); + } + { + DictionaryValue to_set; + to_set.Set(key1_, val2_->DeepCopy()); + to_set.Set(key2_, val2_->DeepCopy()); + to_set.Set("asdf", val1_->DeepCopy()); + to_set.Set("qwerty", val3_->DeepCopy()); + + ExtensionSettingChangeList changes; + changes.push_back( + ExtensionSettingChange(key1_, val1_->DeepCopy(), val2_->DeepCopy())); + changes.push_back(ExtensionSettingChange("asdf", NULL, val1_->DeepCopy())); + changes.push_back( + ExtensionSettingChange("qwerty", NULL, val3_->DeepCopy())); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Set(to_set)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key1_, val2_->DeepCopy(), NULL)); + changes.push_back(ExtensionSettingChange(key2_, val2_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(list12_)); + } + { + std::vector<std::string> to_remove; + to_remove.push_back(key1_); + to_remove.push_back("asdf"); + + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange("asdf", val1_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Remove(to_remove)); + } + { + ExtensionSettingChangeList changes; + changes.push_back(ExtensionSettingChange(key3_, val3_->DeepCopy(), NULL)); + changes.push_back( + ExtensionSettingChange("qwerty", val3_->DeepCopy(), NULL)); + EXPECT_PRED_FORMAT2(ChangesEq, changes, storage_->Clear()); + EXPECT_PRED_FORMAT2(ChangesEq, + ExtensionSettingChangeList(), storage_->Clear()); + } } diff --git a/chrome/browser/extensions/extension_settings_sync_unittest.cc b/chrome/browser/extensions/extension_settings_sync_unittest.cc index 6068ade..88e0372 100644 --- a/chrome/browser/extensions/extension_settings_sync_unittest.cc +++ b/chrome/browser/extensions/extension_settings_sync_unittest.cc @@ -36,8 +36,7 @@ static std::string GetJson(const Value& value) { // Returns whether two Values are equal. testing::AssertionResult ValuesEq( - const char* expected_expr, - const char* actual_expr, + const char* _1, const char* _2, const Value* expected, const Value* actual) { if (expected == actual) { @@ -61,17 +60,15 @@ testing::AssertionResult ValuesEq( // Returns whether the result of a storage operation is an expected value. // Logs when different. testing::AssertionResult SettingsEq( - const char* expected_expr, - const char* actual_expr, - const DictionaryValue* expected, - const ExtensionSettingsStorage::Result actual) { + const char* _1, const char* _2, + const DictionaryValue& expected, + const ExtensionSettingsStorage::ReadResult& actual) { if (actual.HasError()) { return testing::AssertionFailure() << - "Expected: " << GetJson(*expected) << - ", actual has error: " << actual.GetError(); + "Expected: " << GetJson(expected) << + ", actual has error: " << actual.error(); } - return ValuesEq( - expected_expr, actual_expr, expected, actual.GetSettings()); + return ValuesEq(_1, _2, &expected, &actual.settings()); } // SyncChangeProcessor which just records the changes made, accessed after @@ -323,8 +320,8 @@ TEST_F(ExtensionSettingsSyncTest, AnySyncDataOverwritesLocalData) { ASSERT_EQ(0u, sync_.changes().size()); // Sync settings should have been pushed to local settings. - ASSERT_PRED_FORMAT2(SettingsEq, &expected1, storage1->Get()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected2, storage2->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected1, storage1->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected2, storage2->Get()); GetSyncableService(model_type)->StopSyncing(model_type); } @@ -366,8 +363,8 @@ TEST_F(ExtensionSettingsSyncTest, ProcessSyncChanges) { expected1.Set("bar", value2.DeepCopy()); expected2.Set("foo", value1.DeepCopy()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected1, storage1->Get()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected2, storage2->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected1, storage1->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected2, storage2->Get()); // Make sync update some settings, storage1 the new setting, storage2 the // initial setting. @@ -380,8 +377,8 @@ TEST_F(ExtensionSettingsSyncTest, ProcessSyncChanges) { expected1.Set("bar", value2.DeepCopy()); expected2.Set("bar", value1.DeepCopy()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected1, storage1->Get()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected2, storage2->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected1, storage1->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected2, storage2->Get()); // Make sync remove some settings, storage1 the initial setting, storage2 the // new setting. @@ -394,8 +391,8 @@ TEST_F(ExtensionSettingsSyncTest, ProcessSyncChanges) { expected1.Remove("foo", NULL); expected2.Remove("foo", NULL); - ASSERT_PRED_FORMAT2(SettingsEq, &expected1, storage1->Get()); - ASSERT_PRED_FORMAT2(SettingsEq, &expected2, storage2->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected1, storage1->Get()); + ASSERT_PRED_FORMAT2(SettingsEq, expected2, storage2->Get()); GetSyncableService(model_type)->StopSyncing(model_type); } diff --git a/chrome/browser/extensions/in_memory_extension_settings_storage.cc b/chrome/browser/extensions/in_memory_extension_settings_storage.cc index ee50721..3dff28f 100644 --- a/chrome/browser/extensions/in_memory_extension_settings_storage.cc +++ b/chrome/browser/extensions/in_memory_extension_settings_storage.cc @@ -4,6 +4,8 @@ #include "chrome/browser/extensions/in_memory_extension_settings_storage.h" +#include "base/logging.h" + namespace { std::vector<std::string> CreateVector(const std::string& string) { @@ -14,12 +16,12 @@ std::vector<std::string> CreateVector(const std::string& string) { } // namespace -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get( +ExtensionSettingsStorage::ReadResult InMemoryExtensionSettingsStorage::Get( const std::string& key) { return Get(CreateVector(key)); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get( +ExtensionSettingsStorage::ReadResult InMemoryExtensionSettingsStorage::Get( const std::vector<std::string>& keys) { DictionaryValue* settings = new DictionaryValue(); for (std::vector<std::string>::const_iterator it = keys.begin(); @@ -29,67 +31,63 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get( settings->SetWithoutPathExpansion(*it, value->DeepCopy()); } } - return Result(settings, NULL, NULL); + return ReadResult(settings); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get() { - return Result(storage_.DeepCopy(), NULL, NULL); +ExtensionSettingsStorage::ReadResult InMemoryExtensionSettingsStorage::Get() { + return ReadResult(storage_.DeepCopy()); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Set( +ExtensionSettingsStorage::WriteResult InMemoryExtensionSettingsStorage::Set( const std::string& key, const Value& value) { DictionaryValue settings; settings.SetWithoutPathExpansion(key, value.DeepCopy()); return Set(settings); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Set( +ExtensionSettingsStorage::WriteResult InMemoryExtensionSettingsStorage::Set( const DictionaryValue& settings) { - DictionaryValue* old_settings = new DictionaryValue(); - std::set<std::string>* changed_keys = new std::set<std::string>(); - for (DictionaryValue::key_iterator it = settings.begin_keys(); - it != settings.end_keys(); ++it) { + scoped_ptr<ExtensionSettingChangeList> changes( + new ExtensionSettingChangeList()); + for (DictionaryValue::Iterator it(settings); it.HasNext(); it.Advance()) { Value* old_value = NULL; - if (storage_.GetWithoutPathExpansion(*it, &old_value)) { - old_settings->SetWithoutPathExpansion(*it, old_value->DeepCopy()); - } - Value* new_value = NULL; - settings.GetWithoutPathExpansion(*it, &new_value); - if (old_value == NULL || !old_value->Equals(new_value)) { - changed_keys->insert(*it); - storage_.SetWithoutPathExpansion(*it, new_value->DeepCopy()); + storage_.GetWithoutPathExpansion(it.key(), &old_value); + if (!old_value || !old_value->Equals(&it.value())) { + changes->push_back( + ExtensionSettingChange( + it.key(), + old_value ? old_value->DeepCopy() : old_value, + it.value().DeepCopy())); + storage_.SetWithoutPathExpansion(it.key(), it.value().DeepCopy()); } } - return Result(settings.DeepCopy(), old_settings, changed_keys); + return WriteResult(changes.release()); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Remove( +ExtensionSettingsStorage::WriteResult InMemoryExtensionSettingsStorage::Remove( const std::string& key) { return Remove(CreateVector(key)); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Remove( +ExtensionSettingsStorage::WriteResult InMemoryExtensionSettingsStorage::Remove( const std::vector<std::string>& keys) { - DictionaryValue* old_settings = new DictionaryValue(); - std::set<std::string>* changed_keys = new std::set<std::string>(); + scoped_ptr<ExtensionSettingChangeList> changes( + new ExtensionSettingChangeList()); for (std::vector<std::string>::const_iterator it = keys.begin(); it != keys.end(); ++it) { Value* old_value = NULL; if (storage_.RemoveWithoutPathExpansion(*it, &old_value)) { - old_settings->SetWithoutPathExpansion(*it, old_value); - changed_keys->insert(*it); + changes->push_back(ExtensionSettingChange(*it, old_value, NULL)); } } - return Result(NULL, old_settings, changed_keys); + return WriteResult(changes.release()); } -ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Clear() { - std::set<std::string>* changed_keys = new std::set<std::string>(); - for (DictionaryValue::key_iterator it = storage_.begin_keys(); - it != storage_.end_keys(); ++it) { - changed_keys->insert(*it); +ExtensionSettingsStorage::WriteResult +InMemoryExtensionSettingsStorage::Clear() { + std::vector<std::string> keys; + for (DictionaryValue::Iterator it(storage_); it.HasNext(); it.Advance()) { + keys.push_back(it.key()); } - DictionaryValue* old_settings = new DictionaryValue(); - storage_.Swap(old_settings); - return Result(NULL, old_settings, changed_keys); + return Remove(keys); } diff --git a/chrome/browser/extensions/in_memory_extension_settings_storage.h b/chrome/browser/extensions/in_memory_extension_settings_storage.h index e879cfa..fcc95f1 100644 --- a/chrome/browser/extensions/in_memory_extension_settings_storage.h +++ b/chrome/browser/extensions/in_memory_extension_settings_storage.h @@ -15,14 +15,14 @@ class InMemoryExtensionSettingsStorage : public ExtensionSettingsStorage { InMemoryExtensionSettingsStorage() {} // ExtensionSettingsStorage implementation. - virtual Result Get(const std::string& key) OVERRIDE; - virtual Result Get(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Get() OVERRIDE; - virtual Result Set(const std::string& key, const Value& value) OVERRIDE; - virtual Result Set(const DictionaryValue& values) OVERRIDE; - virtual Result Remove(const std::string& key) OVERRIDE; - virtual Result Remove(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Clear() OVERRIDE; + virtual ReadResult Get(const std::string& key) OVERRIDE; + virtual ReadResult Get(const std::vector<std::string>& keys) OVERRIDE; + virtual ReadResult Get() OVERRIDE; + virtual WriteResult Set(const std::string& key, const Value& value) OVERRIDE; + virtual WriteResult Set(const DictionaryValue& values) OVERRIDE; + virtual WriteResult Remove(const std::string& key) OVERRIDE; + virtual WriteResult Remove(const std::vector<std::string>& keys) OVERRIDE; + virtual WriteResult Clear() OVERRIDE; private: DictionaryValue storage_; diff --git a/chrome/browser/extensions/syncable_extension_settings_storage.cc b/chrome/browser/extensions/syncable_extension_settings_storage.cc index 42152cc..772a99e 100644 --- a/chrome/browser/extensions/syncable_extension_settings_storage.cc +++ b/chrome/browser/extensions/syncable_extension_settings_storage.cc @@ -29,83 +29,84 @@ SyncableExtensionSettingsStorage::~SyncableExtensionSettingsStorage() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get( +ExtensionSettingsStorage::ReadResult SyncableExtensionSettingsStorage::Get( const std::string& key) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); return delegate_->Get(key); } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get( +ExtensionSettingsStorage::ReadResult SyncableExtensionSettingsStorage::Get( const std::vector<std::string>& keys) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); return delegate_->Get(keys); } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Get() { +ExtensionSettingsStorage::ReadResult SyncableExtensionSettingsStorage::Get() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); return delegate_->Get(); } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Set( +ExtensionSettingsStorage::WriteResult SyncableExtensionSettingsStorage::Set( const std::string& key, const Value& value) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - Result result = delegate_->Set(key, value); + WriteResult result = delegate_->Set(key, value); if (result.HasError()) { return result; } - if (sync_processor_ && !result.GetChangedKeys()->empty()) { - SendAddsOrUpdatesToSync(*result.GetChangedKeys(), *result.GetSettings()); + if (sync_processor_ && !result.changes().empty()) { + SendChangesToSync(result.changes()); } return result; } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Set( +ExtensionSettingsStorage::WriteResult SyncableExtensionSettingsStorage::Set( const DictionaryValue& values) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - Result result = delegate_->Set(values); + WriteResult result = delegate_->Set(values); if (result.HasError()) { return result; } - if (sync_processor_ && !result.GetChangedKeys()->empty()) { - SendAddsOrUpdatesToSync(*result.GetChangedKeys(), *result.GetSettings()); + if (sync_processor_ && !result.changes().empty()) { + SendChangesToSync(result.changes()); } return result; } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Remove( +ExtensionSettingsStorage::WriteResult SyncableExtensionSettingsStorage::Remove( const std::string& key) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - Result result = delegate_->Remove(key); + WriteResult result = delegate_->Remove(key); if (result.HasError()) { return result; } - if (sync_processor_ && !result.GetChangedKeys()->empty()) { - SendDeletesToSync(*result.GetChangedKeys()); + if (sync_processor_ && !result.changes().empty()) { + SendChangesToSync(result.changes()); } return result; } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Remove( +ExtensionSettingsStorage::WriteResult SyncableExtensionSettingsStorage::Remove( const std::vector<std::string>& keys) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - Result result = delegate_->Remove(keys); + WriteResult result = delegate_->Remove(keys); if (result.HasError()) { return result; } - if (sync_processor_ && !result.GetChangedKeys()->empty()) { - SendDeletesToSync(*result.GetChangedKeys()); + if (sync_processor_ && !result.changes().empty()) { + SendChangesToSync(result.changes()); } return result; } -ExtensionSettingsStorage::Result SyncableExtensionSettingsStorage::Clear() { +ExtensionSettingsStorage::WriteResult +SyncableExtensionSettingsStorage::Clear() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - Result result = delegate_->Clear(); + WriteResult result = delegate_->Clear(); if (result.HasError()) { return result; } - if (sync_processor_ && !result.GetChangedKeys()->empty()) { - SendDeletesToSync(*result.GetChangedKeys()); + if (sync_processor_ && !result.changes().empty()) { + SendChangesToSync(result.changes()); } return result; } @@ -126,19 +127,18 @@ SyncError SyncableExtensionSettingsStorage::StartSyncing( sync_type_ = type; sync_processor_ = sync_processor; - Result maybe_settings = delegate_->Get(); + ReadResult maybe_settings = delegate_->Get(); if (maybe_settings.HasError()) { return SyncError( FROM_HERE, - std::string("Failed to get settings: ") + maybe_settings.GetError(), + std::string("Failed to get settings: ") + maybe_settings.error(), type); } - const DictionaryValue* settings = maybe_settings.GetSettings(); if (sync_state.empty()) { - return SendLocalSettingsToSync(*settings); + return SendLocalSettingsToSync(maybe_settings.settings()); } - return OverwriteLocalSettingsWithSync(sync_state, *settings); + return OverwriteLocalSettingsWithSync(sync_state, maybe_settings.settings()); } SyncError SyncableExtensionSettingsStorage::SendLocalSettingsToSync( @@ -247,7 +247,7 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges( DCHECK(!sync_changes.empty()) << "No sync changes for " << extension_id_; std::vector<SyncError> errors; - ExtensionSettingChanges::Builder changes; + ExtensionSettingChangeList changes; for (ExtensionSettingSyncDataList::const_iterator it = sync_changes.begin(); it != sync_changes.end(); ++it) { @@ -258,21 +258,18 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges( scoped_ptr<Value> current_value; { - Result maybe_settings = Get(it->key()); + ReadResult maybe_settings = Get(it->key()); if (maybe_settings.HasError()) { errors.push_back(SyncError( FROM_HERE, std::string("Error getting current sync state for ") + - extension_id_ + "/" + key + ": " + maybe_settings.GetError(), + extension_id_ + "/" + key + ": " + maybe_settings.error(), sync_type_)); continue; } - const DictionaryValue* settings = maybe_settings.GetSettings(); - if (settings) { - Value* value = NULL; - if (settings->GetWithoutPathExpansion(key, &value)) { - current_value.reset(value->DeepCopy()); - } + Value* value = NULL; + if (maybe_settings.settings().GetWithoutPathExpansion(key, &value)) { + current_value.reset(value->DeepCopy()); } } @@ -325,76 +322,65 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges( observers_->Notify( &ExtensionSettingsObserver::OnSettingsChanged, - static_cast<Profile*>(NULL), extension_id_, - changes.Build()); + ExtensionSettingChange::GetEventJson(changes)); return errors; } -void SyncableExtensionSettingsStorage::SendAddsOrUpdatesToSync( - const std::set<std::string>& changed_keys, - const DictionaryValue& settings) { +void SyncableExtensionSettingsStorage::SendChangesToSync( + const ExtensionSettingChangeList& changes) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DCHECK(sync_processor_); - DCHECK(!changed_keys.empty()); - - SyncChangeList changes; - for (std::set<std::string>::const_iterator it = changed_keys.begin(); - it != changed_keys.end(); ++it) { - Value* value = NULL; - settings.GetWithoutPathExpansion(*it, &value); - DCHECK(value); - if (synced_keys_.count(*it)) { - // Key is synced, send ACTION_UPDATE to sync. - changes.push_back( - extension_settings_sync_util::CreateUpdate( - extension_id_, *it, *value)); - } else { - // Key is not synced, send ACTION_ADD to sync. - changes.push_back( - extension_settings_sync_util::CreateAdd(extension_id_, *it, *value)); - } - } + DCHECK(!changes.empty()); - SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes); - if (error.IsSet()) { - LOG(WARNING) << "Failed to send changes to sync: " << error.message(); - return; - } + SyncChangeList sync_changes; + std::set<std::string> added_keys; + std::set<std::string> deleted_keys; - synced_keys_.insert(changed_keys.begin(), changed_keys.end()); -} - -void SyncableExtensionSettingsStorage::SendDeletesToSync( - const std::set<std::string>& keys) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - DCHECK(sync_processor_); - DCHECK(!keys.empty()); - - SyncChangeList changes; - for (std::set<std::string>::const_iterator it = keys.begin(); - it != keys.end(); ++it) { - if (!synced_keys_.count(*it)) { - LOG(WARNING) << "Deleted " << *it << " but no entry in synced_keys"; - continue; + for (ExtensionSettingChangeList::const_iterator it = changes.begin(); + it != changes.end(); ++it) { + const std::string& key = it->key(); + const Value* value = it->new_value(); + if (value) { + if (synced_keys_.count(key)) { + // New value, key is synced; send ACTION_UPDATE. + sync_changes.push_back( + extension_settings_sync_util::CreateUpdate( + extension_id_, key, *value)); + } else { + // New value, key is not synced; send ACTION_ADD. + sync_changes.push_back( + extension_settings_sync_util::CreateAdd( + extension_id_, key, *value)); + added_keys.insert(key); + } + } else { + if (synced_keys_.count(key)) { + // Clearing value, key is synced; send ACTION_DELETE. + sync_changes.push_back( + extension_settings_sync_util::CreateDelete(extension_id_, key)); + deleted_keys.insert(key); + } else { + LOG(WARNING) << "Deleted " << key << " but not in synced_keys_"; + } } - changes.push_back( - extension_settings_sync_util::CreateDelete(extension_id_, *it)); } - if (changes.empty()) { + if (sync_changes.empty()) { return; } - SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes); + SyncError error = + sync_processor_->ProcessSyncChanges(FROM_HERE, sync_changes); if (error.IsSet()) { LOG(WARNING) << "Failed to send changes to sync: " << error.message(); return; } - for (std::set<std::string>::const_iterator it = keys.begin(); - it != keys.end(); ++it) { + synced_keys_.insert(added_keys.begin(), added_keys.end()); + for (std::set<std::string>::iterator it = deleted_keys.begin(); + it != deleted_keys.end(); ++it) { synced_keys_.erase(*it); } } @@ -402,18 +388,18 @@ void SyncableExtensionSettingsStorage::SendDeletesToSync( SyncError SyncableExtensionSettingsStorage::OnSyncAdd( const std::string& key, Value* new_value, - ExtensionSettingChanges::Builder* changes) { + ExtensionSettingChangeList* changes) { DCHECK(new_value); synced_keys_.insert(key); - Result result = delegate_->Set(key, *new_value); + WriteResult result = delegate_->Set(key, *new_value); if (result.HasError()) { return SyncError( FROM_HERE, std::string("Error pushing sync add to local settings: ") + - result.GetError(), + result.error(), sync_type_); } - changes->AppendChange(key, NULL, new_value); + changes->push_back(ExtensionSettingChange(key, NULL, new_value)); return SyncError(); } @@ -421,35 +407,35 @@ SyncError SyncableExtensionSettingsStorage::OnSyncUpdate( const std::string& key, Value* old_value, Value* new_value, - ExtensionSettingChanges::Builder* changes) { + ExtensionSettingChangeList* changes) { DCHECK(old_value); DCHECK(new_value); - Result result = delegate_->Set(key, *new_value); + WriteResult result = delegate_->Set(key, *new_value); if (result.HasError()) { return SyncError( FROM_HERE, std::string("Error pushing sync update to local settings: ") + - result.GetError(), + result.error(), sync_type_); } - changes->AppendChange(key, old_value, new_value); + changes->push_back(ExtensionSettingChange(key, old_value, new_value)); return SyncError(); } SyncError SyncableExtensionSettingsStorage::OnSyncDelete( const std::string& key, Value* old_value, - ExtensionSettingChanges::Builder* changes) { + ExtensionSettingChangeList* changes) { DCHECK(old_value); synced_keys_.erase(key); - Result result = delegate_->Remove(key); + WriteResult result = delegate_->Remove(key); if (result.HasError()) { return SyncError( FROM_HERE, std::string("Error pushing sync remove to local settings: ") + - result.GetError(), + result.error(), sync_type_); } - changes->AppendChange(key, old_value, NULL); + changes->push_back(ExtensionSettingChange(key, old_value, NULL)); return SyncError(); } diff --git a/chrome/browser/extensions/syncable_extension_settings_storage.h b/chrome/browser/extensions/syncable_extension_settings_storage.h index d222987..f5bf635 100644 --- a/chrome/browser/extensions/syncable_extension_settings_storage.h +++ b/chrome/browser/extensions/syncable_extension_settings_storage.h @@ -29,14 +29,14 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { virtual ~SyncableExtensionSettingsStorage(); // ExtensionSettingsStorage implementation. - virtual Result Get(const std::string& key) OVERRIDE; - virtual Result Get(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Get() OVERRIDE; - virtual Result Set(const std::string& key, const Value& value) OVERRIDE; - virtual Result Set(const DictionaryValue& settings) OVERRIDE; - virtual Result Remove(const std::string& key) OVERRIDE; - virtual Result Remove(const std::vector<std::string>& keys) OVERRIDE; - virtual Result Clear() OVERRIDE; + virtual ReadResult Get(const std::string& key) OVERRIDE; + virtual ReadResult Get(const std::vector<std::string>& keys) OVERRIDE; + virtual ReadResult Get() OVERRIDE; + virtual WriteResult Set(const std::string& key, const Value& value) OVERRIDE; + virtual WriteResult Set(const DictionaryValue& settings) OVERRIDE; + virtual WriteResult Remove(const std::string& key) OVERRIDE; + virtual WriteResult Remove(const std::vector<std::string>& keys) OVERRIDE; + virtual WriteResult Clear() OVERRIDE; // Sync-related methods, analogous to those on SyncableService (handled by // ExtensionSettings). @@ -51,14 +51,9 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { const ExtensionSettingSyncDataList& sync_changes); private: - // Either adds to sync or send updates to sync for some settings. - // Whether they're adds or updates depends on the state of |synced_keys_|. - void SendAddsOrUpdatesToSync( - const std::set<std::string>& changed_keys, - const DictionaryValue& settings); - - // Sends deletes to sync for some settings. - void SendDeletesToSync(const std::set<std::string>& keys); + // Propagates some changes to sync by sending an ADD/UPDATE/DELETE depending + // on the change and the current state of sync. + void SendChangesToSync(const ExtensionSettingChangeList& changes); // Sends all local settings to sync (synced settings assumed to be empty). SyncError SendLocalSettingsToSync( @@ -73,16 +68,16 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { SyncError OnSyncAdd( const std::string& key, Value* new_value, - ExtensionSettingChanges::Builder* changes); + ExtensionSettingChangeList* changes); SyncError OnSyncUpdate( const std::string& key, Value* old_value, Value* new_value, - ExtensionSettingChanges::Builder* changes); + ExtensionSettingChangeList* changes); SyncError OnSyncDelete( const std::string& key, Value* old_value, - ExtensionSettingChanges::Builder* changes); + ExtensionSettingChangeList* changes); // List of observers to settings changes. const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> > diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 2a075ab..a0d0f0c 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1131,8 +1131,8 @@ 'browser/extensions/extension_proxy_api_helpers.h', 'browser/extensions/extension_save_page_api.cc', 'browser/extensions/extension_save_page_api.h', - 'browser/extensions/extension_setting_changes.cc', - 'browser/extensions/extension_setting_changes.h', + 'browser/extensions/extension_setting_change.cc', + 'browser/extensions/extension_setting_change.h', 'browser/extensions/extension_setting_sync_data.cc', 'browser/extensions/extension_setting_sync_data.h', 'browser/extensions/extension_settings_api.cc', diff --git a/chrome/common/extensions/api/extension_api.json b/chrome/common/extensions/api/extension_api.json index 51c5428..60aecc8 100644 --- a/chrome/common/extensions/api/extension_api.json +++ b/chrome/common/extensions/api/extension_api.json @@ -1033,10 +1033,16 @@ { "name": "keys", "choices": [ - {"type": "string"}, - {"type": "array", "items": {"type": "string"}, "minItems": 1} + { "type": "string" }, + { "type": "array", "items": { "type": "string" } }, + { + "type": "object", + "description": "Settings object to return in the callback, where the values are replaced with those from settings if they exist.", + "properties": {}, + "additionalProperties": { "type": "any" } + } ], - "description": "A single key or a list of keys to get from settings. Leave empty to get the entire contents of settings; this should only be used for debugging.", + "description": "A single key to get, list of keys to get, or a dictionary specifying default values (see description of the object). An empty list or object will return an empty settings object. Pass in null or undefined to get the entire contents of settings; this should only be used for debugging.", "optional": true }, { @@ -1070,16 +1076,8 @@ { "name": "callback", "type": "function", - "description": "Callback with settings values, or on failure (in which case lastError will be set).", - "parameters": [ - { - "name": "settings", - "type": "object", - "properties": {}, - "additionalProperties": { "type": "any" }, - "description": "Object with given keys set to settings values." - } - ], + "description": "Callback on success, or on failure (in which case lastError will be set).", + "parameters": [], "optional": true } ] diff --git a/chrome/common/extensions/docs/experimental.settings.html b/chrome/common/extensions/docs/experimental.settings.html index 3c1b347..4f5ff9c 100644 --- a/chrome/common/extensions/docs/experimental.settings.html +++ b/chrome/common/extensions/docs/experimental.settings.html @@ -542,7 +542,7 @@ <div class="summary"><span style="display: none; ">void</span> <!-- Note: intentionally longer 80 columns --> - <span>chrome.experimental.settings.get</span>(<span class="optional"><span style="display: none; ">, </span><span>string or array of string</span> + <span>chrome.experimental.settings.get</span>(<span class="optional"><span style="display: none; ">, </span><span>string or array of string or object</span> <var><span>keys</span></var></span><span class="null"><span>, </span><span>function</span> <var><span>callback</span></var></span>)</div> @@ -572,7 +572,7 @@ <span style="display: none; "> array of <span><span></span></span> </span> - <span>string or array of string</span> + <span>string or array of string or object</span> <span style="display: none; "></span> </span> </span> @@ -584,7 +584,7 @@ <dd class="todo" style="display: none; "> Undocumented. </dd> - <dd>A single key or a list of keys to get from settings. Leave empty to get the entire contents of settings; this should only be used for debugging.</dd> + <dd>A single key to get, list of keys to get, or a dictionary specifying default values (see description of the object). An empty list or object will return an empty settings object. Pass in null or undefined to get the entire contents of settings; this should only be used for debugging.</dd> <dd style="display: none; "> This parameter was added in version <b><span></span></b>. @@ -1113,7 +1113,7 @@ <dd class="todo" style="display: none; "> Undocumented. </dd> - <dd>Callback with settings values, or on failure (in which case lastError will be set).</dd> + <dd>Callback on success, or on failure (in which case lastError will be set).</dd> <dd style="display: none; "> This parameter was added in version <b><span></span></b>. @@ -1176,76 +1176,11 @@ </p> <!-- Note: intentionally longer 80 columns --> - <pre>function(<span>object settings</span>) <span class="subdued">{...}</span>;</pre> + <pre>function(<span></span>) <span class="subdued">{...}</span>;</pre> <dl> - <div> + <div style="display: none; "> <div> - <dt> - <var>settings</var> - <em> - - <!-- TYPE --> - <div style="display:inline"> - ( - <span class="optional" style="display: none; ">optional</span> - <span class="enum" style="display: none; ">enumerated</span> - <span id="typeTemplate"> - <span style="display: none; "> - <a> Type</a> - </span> - <span> - <span style="display: none; "> - array of <span><span></span></span> - </span> - <span>object</span> - <span style="display: none; "></span> - </span> - </span> - ) - </div> - - </em> - </dt> - <dd class="todo" style="display: none; "> - Undocumented. - </dd> - <dd>Object with given keys set to settings values.</dd> - <dd style="display: none; "> - This parameter was added in version - <b><span></span></b>. - You must omit this parameter in earlier versions, - and you may omit it in any version. If you require this - parameter, the manifest key - <a href="manifest.html#minimum_chrome_version">minimum_chrome_version</a> - can ensure that your extension won't be run in an earlier browser version. - </dd> - - <!-- OBJECT PROPERTIES --> - <dd> - <dl> - <div style="display: none; "> - <div> - </div> - </div> - </dl> - </dd> - - <!-- OBJECT METHODS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- OBJECT EVENT FIELDS --> - <dd style="display: none; "> - <div></div> - </dd> - - <!-- FUNCTION PARAMETERS --> - <dd style="display: none; "> - <div></div> - </dd> - - </div> + </div> </div> </dl> </div> diff --git a/chrome/test/data/extensions/api_test/settings/simple_test/background.html b/chrome/test/data/extensions/api_test/settings/simple_test/background.html index 99ab542..fd42fe6 100644 --- a/chrome/test/data/extensions/api_test/settings/simple_test/background.html +++ b/chrome/test/data/extensions/api_test/settings/simple_test/background.html @@ -1,6 +1,15 @@ <script> +// Calls chrome.test.succeed after the settings have been cleared for the next +// test. +function succeed() { + chrome.experimental.settings.clear(chrome.test.succeed); +} + chrome.test.runTests([ function getWhenEmpty() { + function stage0() { + chrome.experimental.settings.get('foo', stage1); + } function stage1(settings) { chrome.test.assertEq({}, settings); chrome.experimental.settings.get(['foo', 'bar'], stage2); @@ -11,18 +20,20 @@ chrome.test.runTests([ } function stage3(settings) { chrome.test.assertEq({}, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.get('foo', stage1); + stage0(); }, function getWhenNonempty() { - function stage1(settings) { - chrome.test.assertEq({ + function stage0() { + chrome.experimental.settings.set({ 'foo' : 'bar', 'baz' : 'qux', 'hello': 'world' - }, settings); + }, stage1); + } + function stage1() { chrome.experimental.settings.get(['foo', 'baz'], stage2); } function stage2(settings) { @@ -46,38 +57,36 @@ chrome.test.runTests([ 'baz' : 'qux', 'hello': 'world' }, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.set({ - 'foo' : 'bar', - 'baz' : 'qux', - 'hello': 'world' - }, stage1); + stage0(); }, function removeWhenEmpty() { - function stage1(settings) { - chrome.test.assertEq(undefined, settings); + function stage0() { + chrome.experimental.settings.remove('foo', stage1); + } + function stage1() { chrome.experimental.settings.remove(['foo', 'bar'], stage2); } - function stage2(settings) { - chrome.test.assertEq(undefined, settings); - chrome.test.succeed(); + function stage2() { + succeed(); } - chrome.experimental.settings.remove('foo', stage1); + stage0(); }, function removeWhenNonempty() { - function stage1(settings) { - chrome.test.assertEq({ + function stage0() { + chrome.experimental.settings.set({ 'foo' : 'bar', 'baz' : 'qux', 'hello': 'world' - }, settings); + }, stage1); + } + function stage1() { chrome.experimental.settings.remove('foo', stage2); } - function stage2(settings) { - chrome.test.assertEq(undefined, settings); + function stage2() { chrome.experimental.settings.get(null, stage3); } function stage3(settings) { @@ -87,8 +96,7 @@ chrome.test.runTests([ }, settings); chrome.experimental.settings.remove(['baz', 'nothing'], stage4); } - function stage4(settings) { - chrome.test.assertEq(undefined, settings); + function stage4() { chrome.experimental.settings.get(null, stage5); } function stage5(settings) { @@ -97,38 +105,31 @@ chrome.test.runTests([ }, settings); chrome.experimental.settings.remove('hello', stage6); } - function stage6(settings) { - chrome.test.assertEq(undefined, settings); + function stage6() { chrome.experimental.settings.get(null, stage7); } function stage7(settings) { chrome.test.assertEq({}, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.set({ - 'foo' : 'bar', - 'baz' : 'qux', - 'hello': 'world' - }, stage1); + stage0(); }, function setWhenOverwriting() { - function stage1(settings) { - chrome.test.assertEq({ + function stage0() { + chrome.experimental.settings.set({ 'foo' : 'bar', 'baz' : 'qux', 'hello': 'world' - }, settings); + }, stage1); + } + function stage1() { chrome.experimental.settings.set({ 'foo' : 'otherBar', 'baz' : 'otherQux' }, stage2); } - function stage2(settings) { - chrome.test.assertEq({ - 'foo' : 'otherBar', - 'baz' : 'otherQux', - }, settings); + function stage2() { chrome.experimental.settings.get(null, stage3); } function stage3(settings) { @@ -143,12 +144,7 @@ chrome.test.runTests([ 'some' : 'value' }, stage4); } - function stage4(settings) { - chrome.test.assertEq({ - 'baz' : 'anotherQux', - 'hello': 'otherWorld', - 'some' : 'value' - }, settings); + function stage4() { chrome.experimental.settings.get(null, stage5); } function stage5(settings) { @@ -158,57 +154,54 @@ chrome.test.runTests([ 'hello': 'otherWorld', 'some' : 'value' }, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.set({ - 'foo' : 'bar', - 'baz' : 'qux', - 'hello': 'world' - }, stage1); + stage0(); }, function clearWhenEmpty() { - function stage1(settings) { - chrome.test.assertEq(undefined, settings); + function stage0() { + chrome.experimental.settings.clear(stage1); + } + function stage1() { chrome.experimental.settings.get(null, stage2); } function stage2(settings) { chrome.test.assertEq({}, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.clear(stage1); + stage0(); }, function clearWhenNonempty() { - function stage1(settings) { - chrome.test.assertEq({ + function stage0() { + chrome.experimental.settings.set({ 'foo' : 'bar', 'baz' : 'qux', 'hello': 'world' - }, settings); + }, stage1); + } + function stage1() { chrome.experimental.settings.clear(stage2); } - function stage2(settings) { - chrome.test.assertEq(undefined, settings); + function stage2() { chrome.experimental.settings.get(null, stage3); } function stage3(settings) { chrome.test.assertEq({}, settings); - chrome.test.succeed(); + succeed(); } - chrome.experimental.settings.set({ - 'foo' : 'bar', - 'baz' : 'qux', - 'hello': 'world' - }, stage1); + stage0(); }, function keysWithDots() { - function stage1(settings) { - chrome.test.assertEq({ + function stage0() { + chrome.experimental.settings.set({ 'foo.bar' : 'baz', 'one' : {'two': 'three'} - }, settings); + }, stage1); + } + function stage1() { chrome.experimental.settings.get(['foo.bar', 'one'], stage2); } function stage2(settings) { @@ -222,20 +215,76 @@ chrome.test.runTests([ chrome.test.assertEq({}, settings); chrome.experimental.settings.remove(['foo.bar', 'one.two'], stage4); } - function stage4(settings) { - chrome.test.assertEq(undefined, settings); + function stage4() { chrome.experimental.settings.get(null, stage5); } function stage5(settings) { chrome.test.assertEq({ 'one' : {'two': 'three'} }, settings); - chrome.test.succeed(); + succeed(); + } + stage0(); + }, + + function getWithDefaultValues() { + function stage0() { + chrome.experimental.settings.get({ + 'foo': 'defaultBar', + 'baz': [1, 2, 3] + }, stage1); + } + function stage1(settings) { + chrome.test.assertEq({ + 'foo': 'defaultBar', + 'baz': [1, 2, 3] + }, settings); + chrome.experimental.settings.get(null, stage2); + } + function stage2(settings) { + chrome.test.assertEq({}, settings); + chrome.experimental.settings.set({'foo': 'bar'}, stage3); + } + function stage3() { + chrome.experimental.settings.get({ + 'foo': 'defaultBar', + 'baz': [1, 2, 3] + }, stage4); + } + function stage4(settings) { + chrome.test.assertEq({ + 'foo': 'bar', + 'baz': [1, 2, 3] + }, settings); + chrome.experimental.settings.set({'baz': {}}, stage5); + } + function stage5() { + chrome.experimental.settings.get({ + 'foo': 'defaultBar', + 'baz': [1, 2, 3] + }, stage6); + } + function stage6(settings) { + chrome.test.assertEq({ + 'foo': 'bar', + 'baz': {} + }, settings); + chrome.experimental.settings.remove('foo', stage7); + } + function stage7() { + chrome.experimental.settings.get({ + 'foo': 'defaultBar', + 'baz': [1, 2, 3] + }, stage8); + } + function stage8(settings) { + chrome.test.assertEq({ + 'foo': 'defaultBar', + 'baz': {} + }, settings); + succeed(); } - chrome.experimental.settings.set({ - 'foo.bar' : 'baz', - 'one' : {'two': 'three'} - }, stage1); + stage0(); } ]); </script> diff --git a/chrome/test/data/extensions/api_test/settings/split_incognito/background.html b/chrome/test/data/extensions/api_test/settings/split_incognito/background.html index bc2ef60..ad65ef5 100644 --- a/chrome/test/data/extensions/api_test/settings/split_incognito/background.html +++ b/chrome/test/data/extensions/api_test/settings/split_incognito/background.html @@ -3,14 +3,6 @@ var settings = chrome.experimental.settings; var assertEq = chrome.test.assertEq; var inIncognitoContext = chrome.extension.inIncognitoContext; -// Returns a function that asserts a result is expected then runs a callback. -function assertResultEq(expected, callback) { - return function(actual) { - chrome.test.assertEq(expected, actual); - callback(); - }; -} - // All notifications received. var notifications = []; settings.onChanged.addListener(function(changes) { @@ -29,19 +21,25 @@ var testActions = { settings.get("", callback); }, assertEmpty: function(callback) { - settings.get(null, assertResultEq({}, callback)); + settings.get(null, function(settings) { + chrome.test.assertEq({}, settings); + callback(); + }); }, assertFoo: function(callback) { - settings.get(null, assertResultEq({foo: "bar"}, callback)); + settings.get(null, function(settings) { + chrome.test.assertEq({foo: "bar"}, settings); + callback(); + }); }, setFoo: function(callback) { - settings.set({foo: "bar"}, assertResultEq({foo: "bar"}, callback)); + settings.set({foo: "bar"}, callback); }, removeFoo: function(callback) { - settings.remove("foo", assertResultEq(undefined, callback)); + settings.remove("foo", callback); }, clear: function(callback) { - settings.clear(assertResultEq(undefined, callback)); + settings.clear(callback); }, assertNoNotifications: function(callback) { assertEq([], notifications); |