diff options
Diffstat (limited to 'chrome/browser/extensions')
26 files changed, 1202 insertions, 415 deletions
diff --git a/chrome/browser/extensions/extension_event_names.cc b/chrome/browser/extensions/extension_event_names.cc index 8c67a4d..bc63d34 100644 --- a/chrome/browser/extensions/extension_event_names.cc +++ b/chrome/browser/extensions/extension_event_names.cc @@ -35,4 +35,6 @@ const char kOnInputMethodChanged[] = "inputMethodPrivate.onChanged"; const char kOnDownloadCreated[] = "experimental.downloads.onCreated"; const char kOnDownloadChanged[] = "experimental.downloads.onChanged"; const char kOnDownloadErased[] = "experimental.downloads.onErased"; + +const char kOnSettingsChanged[] = "experimental.settings.onChanged"; } // namespace extension_event_names diff --git a/chrome/browser/extensions/extension_event_names.h b/chrome/browser/extensions/extension_event_names.h index 67b3dd2..3955276 100644 --- a/chrome/browser/extensions/extension_event_names.h +++ b/chrome/browser/extensions/extension_event_names.h @@ -45,6 +45,9 @@ extern const char kOnDownloadCreated[]; extern const char kOnDownloadChanged[]; extern const char kOnDownloadErased[]; +// Settings. +extern const char kOnSettingsChanged[]; + }; // namespace extension_event_names #endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_EVENT_NAMES_H_ diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc index 91fb896..c3c6e16 100644 --- a/chrome/browser/extensions/extension_service.cc +++ b/chrome/browser/extensions/extension_service.cc @@ -591,8 +591,7 @@ ExtensionService::ExtensionService(Profile* profile, : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), profile_(profile), extension_prefs_(extension_prefs), - extension_settings_frontend_( - profile->GetPath().AppendASCII(kSettingsDirectoryName)), + extension_settings_frontend_(profile), pending_extension_manager_(*ALLOW_THIS_IN_INITIALIZER_LIST(this)), install_directory_(install_directory), extensions_enabled_(extensions_enabled), diff --git a/chrome/browser/extensions/extension_setting_changes.cc b/chrome/browser/extensions/extension_setting_changes.cc new file mode 100644 index 0000000..ce1a95c --- /dev/null +++ b/chrome/browser/extensions/extension_setting_changes.cc @@ -0,0 +1,38 @@ +// 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 new file mode 100644 index 0000000..af9f363 --- /dev/null +++ b/chrome/browser/extensions/extension_setting_changes.h @@ -0,0 +1,66 @@ +// 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 5da832f..64526d7 100644 --- a/chrome/browser/extensions/extension_settings_api.cc +++ b/chrome/browser/extensions/extension_settings_api.cc @@ -25,7 +25,7 @@ bool SettingsFunction::RunImpl() { void SettingsFunction::RunWithBackendOnFileThread( ExtensionSettingsBackend* backend) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - bool success = RunWithStorage(backend->GetStorage(extension_id())); + bool success = RunWithStorage(backend, backend->GetStorage(extension_id())); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, @@ -33,13 +33,35 @@ void SettingsFunction::RunWithBackendOnFileThread( } bool SettingsFunction::UseResult( + ExtensionSettingsBackend* backend, const ExtensionSettingsStorage::Result& storage_result) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); if (storage_result.HasError()) { error_ = storage_result.GetError(); return false; } - DictionaryValue* settings = storage_result.GetSettings(); - result_.reset(settings == NULL ? NULL : settings->DeepCopy()); + + const DictionaryValue* settings = storage_result.GetSettings(); + if (settings) { + result_.reset(settings->DeepCopy()); + } + + 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); + } + backend->TriggerOnSettingsChanged( + profile(), extension_id(), changes.Build()); + } + return true; } @@ -58,47 +80,57 @@ static void AddAllStringValues( } bool GetSettingsFunction::RunWithStorage( + ExtensionSettingsBackend* backend, 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(storage->Get()); + return UseResult(backend, storage->Get()); } else if (input->GetAsString(&as_string)) { - return UseResult(storage->Get(as_string)); + return UseResult(backend, storage->Get(as_string)); } else if (input->GetAsList(&as_list)) { std::vector<std::string> string_list; AddAllStringValues(*as_list, &string_list); - return UseResult(storage->Get(string_list)); + return UseResult(backend, storage->Get(string_list)); } - return UseResult(ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); + return UseResult( + backend, ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); } bool SetSettingsFunction::RunWithStorage( + ExtensionSettingsBackend* backend, ExtensionSettingsStorage* storage) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DictionaryValue *input; EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(0, &input)); - return UseResult(storage->Set(*input)); + return UseResult(backend, storage->Set(*input)); } bool RemoveSettingsFunction::RunWithStorage( + ExtensionSettingsBackend* backend, 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->GetAsString(&as_string)) { - return UseResult(storage->Remove(as_string)); + return UseResult(backend, storage->Remove(as_string)); } else if (input->GetAsList(&as_list)) { std::vector<std::string> string_list; AddAllStringValues(*as_list, &string_list); - return UseResult(storage->Remove(string_list)); + return UseResult(backend, storage->Remove(string_list)); } - return UseResult(ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); + return UseResult( + backend, ExtensionSettingsStorage::Result(kUnsupportedArgumentType)); } bool ClearSettingsFunction::RunWithStorage( + ExtensionSettingsBackend* backend, ExtensionSettingsStorage* storage) { - return UseResult(storage->Clear()); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + return UseResult(backend, storage->Clear()); } diff --git a/chrome/browser/extensions/extension_settings_api.h b/chrome/browser/extensions/extension_settings_api.h index 886319b..15f3fcb 100644 --- a/chrome/browser/extensions/extension_settings_api.h +++ b/chrome/browser/extensions/extension_settings_api.h @@ -21,11 +21,15 @@ class SettingsFunction : public AsyncExtensionFunction { // // Implementations should fill in args themselves, though (like RunImpl) // may return false to imply failure. - virtual bool RunWithStorage(ExtensionSettingsStorage* storage) = 0; + virtual bool RunWithStorage( + ExtensionSettingsBackend* backend, + 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(const ExtensionSettingsStorage::Result& storage_result); + bool UseResult( + ExtensionSettingsBackend* backend, + const ExtensionSettingsStorage::Result& storage_result); private: // Called via PostTask from RunImpl. Calls RunWithStorage and then @@ -38,7 +42,9 @@ class GetSettingsFunction : public SettingsFunction { DECLARE_EXTENSION_FUNCTION_NAME("experimental.settings.get"); protected: - virtual bool RunWithStorage(ExtensionSettingsStorage* storage) OVERRIDE; + virtual bool RunWithStorage( + ExtensionSettingsBackend* backend, + ExtensionSettingsStorage* storage) OVERRIDE; }; class SetSettingsFunction : public SettingsFunction { @@ -46,7 +52,9 @@ class SetSettingsFunction : public SettingsFunction { DECLARE_EXTENSION_FUNCTION_NAME("experimental.settings.set"); protected: - virtual bool RunWithStorage(ExtensionSettingsStorage* storage) OVERRIDE; + virtual bool RunWithStorage( + ExtensionSettingsBackend* backend, + ExtensionSettingsStorage* storage) OVERRIDE; }; class RemoveSettingsFunction : public SettingsFunction { @@ -54,7 +62,9 @@ class RemoveSettingsFunction : public SettingsFunction { DECLARE_EXTENSION_FUNCTION_NAME("experimental.settings.remove"); protected: - virtual bool RunWithStorage(ExtensionSettingsStorage* storage) OVERRIDE; + virtual bool RunWithStorage( + ExtensionSettingsBackend* backend, + ExtensionSettingsStorage* storage) OVERRIDE; }; class ClearSettingsFunction : public SettingsFunction { @@ -62,7 +72,9 @@ class ClearSettingsFunction : public SettingsFunction { DECLARE_EXTENSION_FUNCTION_NAME("experimental.settings.clear"); protected: - virtual bool RunWithStorage(ExtensionSettingsStorage* storage) OVERRIDE; + virtual bool RunWithStorage( + ExtensionSettingsBackend* backend, + ExtensionSettingsStorage* storage) OVERRIDE; }; #endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_API_H_ diff --git a/chrome/browser/extensions/extension_settings_apitest.cc b/chrome/browser/extensions/extension_settings_apitest.cc index 213d921..6b7f8a7 100644 --- a/chrome/browser/extensions/extension_settings_apitest.cc +++ b/chrome/browser/extensions/extension_settings_apitest.cc @@ -2,15 +2,36 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/bind.h" #include "base/command_line.h" #include "base/json/json_writer.h" #include "chrome/browser/extensions/extension_apitest.h" +#include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/extension_settings_backend.h" +#include "chrome/browser/extensions/extension_settings_sync_util.h" #include "chrome/browser/extensions/extension_test_message_listener.h" #include "chrome/browser/profiles/profile.h" +#include "chrome/browser/sync/api/sync_change.h" +#include "chrome/browser/sync/api/sync_change_processor.h" #include "chrome/browser/ui/browser.h" #include "chrome/common/chrome_switches.h" #include "chrome/test/base/ui_test_utils.h" +namespace { + +class NoopSyncChangeProcessor : public SyncChangeProcessor { + public: + virtual SyncError ProcessSyncChanges( + const tracked_objects::Location& from_here, + const SyncChangeList& change_list) OVERRIDE { + return SyncError(); + } + + virtual ~NoopSyncChangeProcessor() {}; +}; + +} // namespace + class ExtensionSettingsApiTest : public ExtensionApiTest { protected: void ReplyWhenSatisfied( @@ -20,11 +41,11 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { normal_action, incognito_action, NULL, false); } - void LoadAndReplyWhenSatisfied( + const Extension* LoadAndReplyWhenSatisfied( const std::string& normal_action, const std::string& incognito_action, const std::string& extension_dir) { - MaybeLoadAndReplyWhenSatisfied( + return MaybeLoadAndReplyWhenSatisfied( normal_action, incognito_action, &extension_dir, false); } @@ -34,8 +55,26 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { MaybeLoadAndReplyWhenSatisfied(normal_action, incognito_action, NULL, true); } + void InitSync(SyncChangeProcessor* sync_processor) { + browser()->profile()->GetExtensionService()-> + extension_settings_frontend()->RunWithBackend(base::Bind( + &ExtensionSettingsApiTest::InitSyncWithBackend, + this, + sync_processor)); + MessageLoop::current()->RunAllPending(); + } + + void SendChanges(const SyncChangeList& change_list) { + browser()->profile()->GetExtensionService()-> + extension_settings_frontend()->RunWithBackend(base::Bind( + &ExtensionSettingsApiTest::SendChangesToBackend, + this, + change_list)); + MessageLoop::current()->RunAllPending(); + } + private: - void MaybeLoadAndReplyWhenSatisfied( + const Extension* MaybeLoadAndReplyWhenSatisfied( const std::string& normal_action, const std::string& incognito_action, // May be NULL to imply not loading the extension. @@ -46,9 +85,11 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { // Only load the extension after the listeners have been set up, to avoid // initialisation race conditions. + const Extension* extension = NULL; if (extension_dir) { - ASSERT_TRUE(LoadExtensionIncognito(test_data_dir_ - .AppendASCII("settings").AppendASCII(*extension_dir))); + extension = LoadExtensionIncognito( + test_data_dir_.AppendASCII("settings").AppendASCII(*extension_dir)); + EXPECT_TRUE(extension); } EXPECT_TRUE(listener.WaitUntilSatisfied()); @@ -56,6 +97,7 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { listener.Reply(CreateMessage(normal_action, is_final_action)); listener_incognito.Reply(CreateMessage(incognito_action, is_final_action)); + return extension; } std::string CreateMessage(const std::string& action, bool is_final_action) { @@ -66,6 +108,19 @@ class ExtensionSettingsApiTest : public ExtensionApiTest { base::JSONWriter::Write(message.get(), false, &message_json); return message_json; } + + void InitSyncWithBackend( + SyncChangeProcessor* sync_processor, ExtensionSettingsBackend* backend) { + EXPECT_FALSE(backend->MergeDataAndStartSyncing( + syncable::EXTENSION_SETTINGS, + SyncDataList(), + sync_processor).IsSet()); + } + + void SendChangesToBackend( + const SyncChangeList& change_list, ExtensionSettingsBackend* backend) { + EXPECT_FALSE(backend->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); + } }; IN_PROC_BROWSER_TEST_F(ExtensionSettingsApiTest, SimpleTest) { @@ -90,15 +145,84 @@ IN_PROC_BROWSER_TEST_F(ExtensionSettingsApiTest, SplitModeIncognito) { browser()->profile()->GetOffTheRecordProfile()); LoadAndReplyWhenSatisfied("assertEmpty", "assertEmpty", "split_incognito"); - ReplyWhenSatisfied("noop", "setFoobar"); - ReplyWhenSatisfied("assertFoobar", "assertFoobar"); + ReplyWhenSatisfied("noop", "setFoo"); + ReplyWhenSatisfied("assertFoo", "assertFoo"); ReplyWhenSatisfied("clear", "noop"); ReplyWhenSatisfied("assertEmpty", "assertEmpty"); - ReplyWhenSatisfied("setFoobar", "noop"); - ReplyWhenSatisfied("assertFoobar", "assertFoobar"); + ReplyWhenSatisfied("setFoo", "noop"); + ReplyWhenSatisfied("assertFoo", "assertFoo"); ReplyWhenSatisfied("noop", "removeFoo"); FinalReplyWhenSatisfied("assertEmpty", "assertEmpty"); EXPECT_TRUE(catcher.GetNextResult()) << catcher.message(); EXPECT_TRUE(catcher_incognito.GetNextResult()) << catcher.message(); } + +IN_PROC_BROWSER_TEST_F(ExtensionSettingsApiTest, + OnChangedNotificationsBetweenBackgroundPages) { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableExperimentalExtensionApis); + + // We need 2 ResultCatchers because we'll be running the same test in both + // regular and incognito mode. + ResultCatcher catcher, catcher_incognito; + catcher.RestrictToProfile(browser()->profile()); + catcher_incognito.RestrictToProfile( + browser()->profile()->GetOffTheRecordProfile()); + + LoadAndReplyWhenSatisfied( + "assertNoNotifications", "assertNoNotifications", "split_incognito"); + ReplyWhenSatisfied("noop", "setFoo"); + ReplyWhenSatisfied("assertAddFooNotification", "assertNoNotifications"); + ReplyWhenSatisfied("clearNotifications", "noop"); + ReplyWhenSatisfied("removeFoo", "noop"); + FinalReplyWhenSatisfied( + "assertNoNotifications", "assertDeleteFooNotification"); + + EXPECT_TRUE(catcher.GetNextResult()) << catcher.message(); + EXPECT_TRUE(catcher_incognito.GetNextResult()) << catcher.message(); +} + +// Disabled, see crbug.com/101110 +IN_PROC_BROWSER_TEST_F(ExtensionSettingsApiTest, + DISABLED_OnChangedNotificationsFromSync) { + CommandLine::ForCurrentProcess()->AppendSwitch( + switches::kEnableExperimentalExtensionApis); + + // We need 2 ResultCatchers because we'll be running the same test in both + // regular and incognito mode. + ResultCatcher catcher, catcher_incognito; + catcher.RestrictToProfile(browser()->profile()); + catcher_incognito.RestrictToProfile( + browser()->profile()->GetOffTheRecordProfile()); + + const Extension* extension = + LoadAndReplyWhenSatisfied( + "assertNoNotifications", "assertNoNotifications", "split_incognito"); + const std::string& extension_id = extension->id(); + + NoopSyncChangeProcessor sync_processor; + InitSync(&sync_processor); + + // Set "foo" to "bar" via sync. + SyncChangeList sync_changes; + StringValue bar("bar"); + sync_changes.push_back(extension_settings_sync_util::CreateAdd( + extension_id, "foo", bar)); + SendChanges(sync_changes); + + ReplyWhenSatisfied("assertAddFooNotification", "assertAddFooNotification"); + ReplyWhenSatisfied("clearNotifications", "clearNotifications"); + + // Remove "foo" via sync. + sync_changes.clear(); + sync_changes.push_back(extension_settings_sync_util::CreateDelete( + extension_id, "foo")); + SendChanges(sync_changes); + + FinalReplyWhenSatisfied( + "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 ef97825..56bd944 100644 --- a/chrome/browser/extensions/extension_settings_backend.cc +++ b/chrome/browser/extensions/extension_settings_backend.cc @@ -36,8 +36,12 @@ const size_t kMaxSettingKeys = 512; } // namespace -ExtensionSettingsBackend::ExtensionSettingsBackend(const FilePath& base_path) +ExtensionSettingsBackend::ExtensionSettingsBackend( + const FilePath& base_path, + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >& + observers) : base_path_(base_path), + observers_(observers), sync_processor_(NULL) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); } @@ -83,7 +87,10 @@ ExtensionSettingsBackend::GetOrCreateStorageWithSyncData( syncable_storage = linked_ptr<SyncableExtensionSettingsStorage>( - new SyncableExtensionSettingsStorage(extension_id, storage)); + new SyncableExtensionSettingsStorage( + observers_, + extension_id, + storage)); if (sync_processor_) { // TODO(kalman): do something if StartSyncing fails. ignore_result(syncable_storage->StartSyncing(sync_data, sync_processor_)); @@ -108,6 +115,17 @@ void ExtensionSettingsBackend::DeleteExtensionData( 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; @@ -161,7 +179,7 @@ SyncDataList ExtensionSettingsBackend::GetAllSyncData( continue; } - DictionaryValue* settings = maybe_settings.GetSettings(); + 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; diff --git a/chrome/browser/extensions/extension_settings_backend.h b/chrome/browser/extensions/extension_settings_backend.h index 28b0e8a..81c1067 100644 --- a/chrome/browser/extensions/extension_settings_backend.h +++ b/chrome/browser/extensions/extension_settings_backend.h @@ -10,7 +10,9 @@ #include "base/file_path.h" #include "base/memory/linked_ptr.h" #include "base/memory/ref_counted.h" +#include "base/observer_list_threadsafe.h" #include "base/task.h" +#include "chrome/browser/extensions/extension_settings_observer.h" #include "chrome/browser/extensions/syncable_extension_settings_storage.h" #include "chrome/browser/sync/api/syncable_service.h" @@ -19,9 +21,13 @@ // Lives entirely on the FILE thread. class ExtensionSettingsBackend : public SyncableService { public: - // File path is the base of the extension settings directory. - // The databases will be at base_path/extension_id. - explicit ExtensionSettingsBackend(const FilePath& base_path); + // |base_path| is the base of the extension settings directory, so the + // databases will be at base_path/extension_id. + // |observers| is the list of observers to settings changes. + explicit ExtensionSettingsBackend( + const FilePath& base_path, + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >& + observers); virtual ~ExtensionSettingsBackend(); @@ -37,6 +43,13 @@ class ExtensionSettingsBackend : public SyncableService { // Deletes all setting data for an extension. Call on the FILE thread. void DeleteExtensionData(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( @@ -64,6 +77,10 @@ class ExtensionSettingsBackend : public SyncableService { // The base file path to create any leveldb databases at. const FilePath base_path_; + // The list of observers to settings changes. + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> > + observers_; + // A cache of ExtensionSettingsStorage objects that have already been created. // Ensure that there is only ever one created per extension. typedef std::map<std::string, linked_ptr<SyncableExtensionSettingsStorage> > diff --git a/chrome/browser/extensions/extension_settings_frontend.cc b/chrome/browser/extensions/extension_settings_frontend.cc index 4963e92..84bfaac 100644 --- a/chrome/browser/extensions/extension_settings_frontend.cc +++ b/chrome/browser/extensions/extension_settings_frontend.cc @@ -6,20 +6,117 @@ #include "base/bind.h" #include "base/file_path.h" +#include "chrome/browser/extensions/extension_event_names.h" +#include "chrome/browser/extensions/extension_event_router.h" +#include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_settings_backend.h" +#include "chrome/browser/profiles/profile.h" #include "content/browser/browser_thread.h" +#include "content/public/browser/notification_service.h" -ExtensionSettingsFrontend::ExtensionSettingsFrontend( - const FilePath& base_path) - : core_(new ExtensionSettingsFrontend::Core()) { +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()); + } + } + + private: + Profile* target_profile_; +}; + +class ExtensionSettingsFrontend::Core + : public base::RefCountedThreadSafe<Core> { + public: + explicit Core( + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >& + observers) + : observers_(observers), backend_(NULL) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + } + + // Does any FILE thread specific initialization, such as construction of + // |backend_|. Must be called before any call to + // RunWithBackendOnFileThread(). + void InitOnFileThread(const FilePath& base_path) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(!backend_); + backend_ = new ExtensionSettingsBackend(base_path, observers_); + } + + // Runs |callback| with the extension backend. + void RunWithBackendOnFileThread(const BackendCallback& callback) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + DCHECK(backend_); + callback.Run(backend_); + } + + private: + virtual ~Core() { + if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) { + delete backend_; + } else if (BrowserThread::CurrentlyOn(BrowserThread::UI)) { + BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, backend_); + } else { + NOTREACHED(); + } + } + + friend class base::RefCountedThreadSafe<Core>; + + // Observers to settings changes (thread safe). + scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> > + observers_; + + // Lives on the FILE thread. + ExtensionSettingsBackend* backend_; + + DISALLOW_COPY_AND_ASSIGN(Core); +}; + +ExtensionSettingsFrontend::ExtensionSettingsFrontend(Profile* profile) + : profile_(profile), + observers_(new ObserverListThreadSafe<ExtensionSettingsObserver>()), + core_(new ExtensionSettingsFrontend::Core(observers_.get())) { 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); + BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind( &ExtensionSettingsFrontend::Core::InitOnFileThread, core_.get(), - base_path)); + profile->GetPath().AppendASCII( + ExtensionService::kSettingsDirectoryName))); +} + +ExtensionSettingsFrontend::~ExtensionSettingsFrontend() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); } void ExtensionSettingsFrontend::RunWithBackend( @@ -34,34 +131,66 @@ void ExtensionSettingsFrontend::RunWithBackend( callback)); } -ExtensionSettingsFrontend::~ExtensionSettingsFrontend() { +void ExtensionSettingsFrontend::AddObserver( + ExtensionSettingsObserver* observer) { + observers_->AddObserver(observer); +} + +void ExtensionSettingsFrontend::RemoveObserver( + ExtensionSettingsObserver* observer) { + observers_->RemoveObserver(observer); +} + +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(); + } } -ExtensionSettingsFrontend::Core::Core() : backend_(NULL) { +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::Core::InitOnFileThread( - const FilePath& base_path) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - DCHECK(!backend_); - backend_ = new ExtensionSettingsBackend(base_path); +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::Core::RunWithBackendOnFileThread( - const BackendCallback& callback) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); - DCHECK(backend_); - callback.Run(backend_); +void ExtensionSettingsFrontend::SetDefaultObserver( + Profile* profile, scoped_ptr<DefaultObserver>* observer) { + DCHECK(!observer->get()); + observer->reset(new DefaultObserver(profile)); + AddObserver(observer->get()); } -ExtensionSettingsFrontend::Core::~Core() { - if (BrowserThread::CurrentlyOn(BrowserThread::FILE)) { - delete backend_; - } else if (BrowserThread::CurrentlyOn(BrowserThread::UI)) { - BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, backend_); - } else { - NOTREACHED(); +void ExtensionSettingsFrontend::ClearDefaultObserver( + scoped_ptr<DefaultObserver>* observer) { + if (observer->get()) { + RemoveObserver(observer->get()); + observer->reset(); } } diff --git a/chrome/browser/extensions/extension_settings_frontend.h b/chrome/browser/extensions/extension_settings_frontend.h index 7fe3dc0..104da18 100644 --- a/chrome/browser/extensions/extension_settings_frontend.h +++ b/chrome/browser/extensions/extension_settings_frontend.h @@ -6,56 +6,83 @@ #define CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_FRONTEND_H_ #pragma once +#include "base/callback.h" #include "base/compiler_specific.h" #include "base/memory/ref_counted.h" -#include "base/callback.h" +#include "base/memory/scoped_ptr.h" +#include "base/observer_list_threadsafe.h" +#include "chrome/browser/extensions/extension_settings_observer.h" #include "chrome/browser/sync/api/syncable_service.h" +#include "content/public/browser/notification_observer.h" +#include "content/public/browser/notification_registrar.h" class FilePath; +class Profile; class ExtensionSettingsBackend; class ExtensionSettingsStorage; // The component of extension settings which runs on the UI thread, as opposed // to ExtensionSettingsBackend which lives on the FILE thread. -class ExtensionSettingsFrontend { +class ExtensionSettingsFrontend : public content::NotificationObserver { public: - explicit ExtensionSettingsFrontend(const FilePath& base_path); + explicit ExtensionSettingsFrontend(Profile* profile); + virtual ~ExtensionSettingsFrontend(); typedef base::Callback<void(ExtensionSettingsBackend*)> BackendCallback; // Runs |callback| on the FILE thread with the extension settings. void RunWithBackend(const BackendCallback& callback); - ~ExtensionSettingsFrontend(); + // Adds an observer to settings changes. + void AddObserver(ExtensionSettingsObserver* observer); + + // Removes an observer to settings changes. + void RemoveObserver(ExtensionSettingsObserver* observer); + + // NotificationObserver implementation. + virtual void Observe( + int type, + const content::NotificationSource& source, + const content::NotificationDetails& details) OVERRIDE; private: - // Ref-counted container for the ExtensionSettingsBackend object. - class Core : public base::RefCountedThreadSafe<Core> { - public: - // Constructed on UI thread. - Core(); + // Observer which sends events to a target profile iff the profile isn't the + // originating profile for the event. + class DefaultObserver; - // Does any FILE thread specific initialization, such as construction of - // |backend_|. Must be called before any call to - // RunWithBackendOnFileThread(). - void InitOnFileThread(const FilePath& base_path); + // Called when a profile is created. + void OnProfileCreated(Profile* profile); - // Runs |callback| with the extension backend. - void RunWithBackendOnFileThread(const BackendCallback& callback); + // Called when a profile is destroyed. + void OnProfileDestroyed(Profile* profile); - private: - // Can be destroyed on either the UI or FILE thread. - virtual ~Core(); - friend class base::RefCountedThreadSafe<Core>; + // Creates a scoped DefaultObserver and adds it as an Observer. + void SetDefaultObserver( + Profile* profile, scoped_ptr<DefaultObserver>* observer); - // Lives on the FILE thread. - ExtensionSettingsBackend* backend_; + // If a scoped DefaultObserver exists, clears it and removes it as an + // Observer. + void ClearDefaultObserver(scoped_ptr<DefaultObserver>* observer); - DISALLOW_COPY_AND_ASSIGN(Core); - }; + // 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. + Profile* const profile_; + // List of observers to settings changes. + scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> > observers_; + + // The default original and incognito mode profile observers. + scoped_ptr<DefaultObserver> original_profile_observer; + scoped_ptr<DefaultObserver> incognito_profile_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 076eda3..d245850 100644 --- a/chrome/browser/extensions/extension_settings_frontend_unittest.cc +++ b/chrome/browser/extensions/extension_settings_frontend_unittest.cc @@ -23,13 +23,13 @@ class ExtensionSettingsFrontendTest : public testing::Test { virtual void SetUp() OVERRIDE { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); - frontend_.reset(new ExtensionSettingsFrontend(temp_dir_.path())); profile_.reset(new TestingProfile(temp_dir_.path())); + frontend_.reset(new ExtensionSettingsFrontend(profile_.get())); } virtual void TearDown() OVERRIDE { - profile_.reset(); frontend_.reset(); + profile_.reset(); } protected: @@ -45,8 +45,8 @@ class ExtensionSettingsFrontendTest : public testing::Test { } ScopedTempDir temp_dir_; - scoped_ptr<ExtensionSettingsFrontend> frontend_; scoped_ptr<TestingProfile> profile_; + scoped_ptr<ExtensionSettingsFrontend> frontend_; private: // Intended as a ExtensionSettingsFrontend::BackendCallback from GetBackend. @@ -77,7 +77,7 @@ TEST_F(ExtensionSettingsFrontendTest, SettingsPreservedAcrossReconstruction) { ASSERT_FALSE(result.HasError()); EXPECT_FALSE(result.GetSettings()->empty()); - frontend_.reset(new ExtensionSettingsFrontend(temp_dir_.path())); + frontend_.reset(new ExtensionSettingsFrontend(profile_.get())); GetBackend(&backend); storage = backend->GetStorage(id); diff --git a/chrome/browser/extensions/extension_settings_leveldb_storage.cc b/chrome/browser/extensions/extension_settings_leveldb_storage.cc index 5c86c02..647c232 100644 --- a/chrome/browser/extensions/extension_settings_leveldb_storage.cc +++ b/chrome/browser/extensions/extension_settings_leveldb_storage.cc @@ -100,7 +100,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( if (setting.get()) { settings->SetWithoutPathExpansion(key, setting.release()); } - return Result(settings, NULL); + return Result(settings, NULL, NULL); } ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( @@ -124,7 +124,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get( } } - return Result(settings.release(), NULL); + return Result(settings.release(), NULL, NULL); } ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() { @@ -154,7 +154,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Get() { return Result(kGenericOnFailureMessage); } - return Result(settings.release(), NULL); + return Result(settings.release(), NULL, NULL); } ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( @@ -168,24 +168,29 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( ExtensionSettingsStorage::Result 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; for (DictionaryValue::key_iterator it = settings.begin_keys(); it != settings.end_keys(); ++it) { - scoped_ptr<Value> original_value; - if (!ReadFromDb(leveldb::ReadOptions(), *it, &original_value)) { + scoped_ptr<Value> old_value; + if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) { return Result(kGenericOnFailureMessage); } Value* new_value = NULL; settings.GetWithoutPathExpansion(*it, &new_value); - if (!original_value.get() || !original_value->Equals(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()); + } } leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch); @@ -194,7 +199,8 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Set( return Result(kGenericOnFailureMessage); } - return Result(settings.DeepCopy(), changed_keys.release()); + return Result( + settings.DeepCopy(), old_settings.release(), changed_keys.release()); } ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( @@ -208,18 +214,20 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( ExtensionSettingsStorage::Result 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; for (std::vector<std::string>::const_iterator it = keys.begin(); it != keys.end(); ++it) { - scoped_ptr<Value> original_value; - if (!ReadFromDb(leveldb::ReadOptions(), *it, &original_value)) { + scoped_ptr<Value> old_value; + if (!ReadFromDb(leveldb::ReadOptions(), *it, &old_value)) { return Result(kGenericOnFailureMessage); } - if (original_value.get()) { + if (old_value.get()) { changed_keys->insert(*it); + old_settings->SetWithoutPathExpansion(*it, old_value.release()); batch.Delete(*it); } } @@ -230,11 +238,12 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Remove( return Result(kGenericOnFailureMessage); } - return Result(NULL, changed_keys.release()); + return Result(NULL, old_settings.release(), changed_keys.release()); } ExtensionSettingsStorage::Result 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 @@ -245,8 +254,17 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() { options.snapshot = snapshot.get(); scoped_ptr<leveldb::Iterator> it(db_->NewIterator(options)); for (it->SeekToFirst(); it->Valid(); it->Next()) { - changed_keys->insert(it->key().ToString()); - batch.Delete(it->key()); + const std::string key = it->key().ToString(); + const std::string old_value_as_json = it->value().ToString(); + changed_keys->insert(key); + Value* old_value = + base::JSONReader().JsonToValue(old_value_as_json, false, false); + if (old_value) { + old_settings->SetWithoutPathExpansion(key, old_value); + } else { + LOG(ERROR) << "Invalid JSON in database: " << old_value_as_json; + } + batch.Delete(key); } if (!it->status().ok()) { @@ -260,7 +278,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsLeveldbStorage::Clear() { return Result(kGenericOnFailureMessage); } - return Result(NULL, changed_keys.release()); + return Result(NULL, old_settings.release(), changed_keys.release()); } bool ExtensionSettingsLeveldbStorage::ReadFromDb( diff --git a/chrome/browser/extensions/extension_settings_observer.cc b/chrome/browser/extensions/extension_settings_observer.cc new file mode 100644 index 0000000..d25ae17 --- /dev/null +++ b/chrome/browser/extensions/extension_settings_observer.cc @@ -0,0 +1,7 @@ +// 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_settings_observer.h" + +ExtensionSettingsObserver::~ExtensionSettingsObserver() {} diff --git a/chrome/browser/extensions/extension_settings_observer.h b/chrome/browser/extensions/extension_settings_observer.h new file mode 100644 index 0000000..7cc2461 --- /dev/null +++ b/chrome/browser/extensions/extension_settings_observer.h @@ -0,0 +1,31 @@ +// 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_SETTINGS_OBSERVER_H_ +#define CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_OBSERVER_H_ +#pragma once + +#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; + + protected: + virtual ~ExtensionSettingsObserver(); +}; + +#endif // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_OBSERVER_H_ diff --git a/chrome/browser/extensions/extension_settings_storage.cc b/chrome/browser/extensions/extension_settings_storage.cc index 882efe8..3259748 100644 --- a/chrome/browser/extensions/extension_settings_storage.cc +++ b/chrome/browser/extensions/extension_settings_storage.cc @@ -8,27 +8,57 @@ // Implementation of ExtensionSettingsStorage::Result ExtensionSettingsStorage::Result::Result( - DictionaryValue* settings, std::set<std::string>* changed_keys) - : inner_(new Inner(settings, changed_keys, std::string())) {} + DictionaryValue* settings, + DictionaryValue* old_settings, + std::set<std::string>* changed_keys) + : inner_(new Inner(settings, old_settings, changed_keys, std::string())) {} ExtensionSettingsStorage::Result::Result(const std::string& error) - : inner_(new Inner(NULL, new std::set<std::string>(), error)) { + : inner_(new Inner(NULL, NULL, new std::set<std::string>(), error)) { DCHECK(!error.empty()); } ExtensionSettingsStorage::Result::~Result() {} -DictionaryValue* ExtensionSettingsStorage::Result::GetSettings() const { +const DictionaryValue* ExtensionSettingsStorage::Result::GetSettings() const { DCHECK(!HasError()); return inner_->settings_.get(); } -std::set<std::string>* +const std::set<std::string>* ExtensionSettingsStorage::Result::GetChangedKeys() const { DCHECK(!HasError()); return inner_->changed_keys_.get(); } +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 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; +} + bool ExtensionSettingsStorage::Result::HasError() const { return !inner_->error_.empty(); } @@ -40,8 +70,12 @@ const std::string& ExtensionSettingsStorage::Result::GetError() const { ExtensionSettingsStorage::Result::Inner::Inner( DictionaryValue* settings, + DictionaryValue* old_settings, std::set<std::string>* changed_keys, const std::string& error) - : settings_(settings), changed_keys_(changed_keys), error_(error) {} + : settings_(settings), + old_settings_(old_settings), + changed_keys_(changed_keys), + error_(error) {} ExtensionSettingsStorage::Result::Inner::~Inner() {} diff --git a/chrome/browser/extensions/extension_settings_storage.h b/chrome/browser/extensions/extension_settings_storage.h index ba728bc..106f8fb 100644 --- a/chrome/browser/extensions/extension_settings_storage.h +++ b/chrome/browser/extensions/extension_settings_storage.h @@ -23,25 +23,37 @@ class ExtensionSettingsStorage { // Supports lightweight copying. class Result { public: - // Ownership of |settings| and |changed_keys| taken. + // 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| may be NULL when the result is for an operation which - // cannot change settings (e.g. Get()). - Result(DictionaryValue* settings, std::set<std::string>* changed_keys); + // |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 with the Result. - DictionaryValue* GetSettings() const; + // Ownership remains with this. + const DictionaryValue* GetSettings() const; - // Gets the list of setting keys which changed as a result of the + // 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(). - std::set<std::string>* GetChangedKeys() const; + // 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. @@ -55,11 +67,13 @@ class ExtensionSettingsStorage { // Empty error implies no error. Inner( DictionaryValue* settings, + DictionaryValue* old_settings, std::set<std::string>* changed_keys, const std::string& error); ~Inner(); const scoped_ptr<DictionaryValue> settings_; + const scoped_ptr<DictionaryValue> old_settings_; const scoped_ptr<std::set<std::string> > changed_keys_; const std::string error_; }; diff --git a/chrome/browser/extensions/extension_settings_storage_cache.cc b/chrome/browser/extensions/extension_settings_storage_cache.cc index f20f068..2df99ed 100644 --- a/chrome/browser/extensions/extension_settings_storage_cache.cc +++ b/chrome/browser/extensions/extension_settings_storage_cache.cc @@ -15,7 +15,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( if (GetFromCache(key, &value)) { DictionaryValue* settings = new DictionaryValue(); settings->SetWithoutPathExpansion(key, value); - return Result(settings, NULL); + return Result(settings, NULL, NULL); } Result result = delegate_->Get(key); @@ -43,7 +43,7 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( } if (missing_keys.empty()) { - return Result(from_cache.release(), NULL); + return Result(from_cache.release(), NULL, NULL); } Result result = delegate_->Get(keys); @@ -52,8 +52,10 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get( } cache_.MergeDictionary(result.GetSettings()); - result.GetSettings()->MergeDictionary(from_cache.get()); - return result; + + DictionaryValue* settings_with_cache = result.GetSettings()->DeepCopy(); + settings_with_cache->MergeDictionary(from_cache.get()); + return Result(settings_with_cache, NULL, NULL); } ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Get() { @@ -80,9 +82,9 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Set( return result; } - std::set<std::string>* changed_keys = result.GetChangedKeys(); + const std::set<std::string>* changed_keys = result.GetChangedKeys(); DCHECK(changed_keys); - for (std::set<std::string>::iterator it = changed_keys->begin(); + 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); @@ -108,9 +110,9 @@ ExtensionSettingsStorage::Result ExtensionSettingsStorageCache::Remove( return result; } - std::set<std::string>* changed_keys = result.GetChangedKeys(); + const std::set<std::string>* changed_keys = result.GetChangedKeys(); DCHECK(changed_keys); - for (std::set<std::string>::iterator it = changed_keys->begin(); + for (std::set<std::string>::const_iterator it = changed_keys->begin(); it != changed_keys->end(); ++it) { cache_.RemoveWithoutPathExpansion(*it, NULL); } diff --git a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc index 91a62ce..6186c87 100644 --- a/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc +++ b/chrome/browser/extensions/extension_settings_storage_quota_enforcer.cc @@ -90,7 +90,8 @@ ExtensionSettingsStorageQuotaEnforcer::ExtensionSettingsStorageQuotaEnforcer( return; } - DictionaryValue* initial_settings = maybe_initial_settings.GetSettings(); + 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; diff --git a/chrome/browser/extensions/extension_settings_storage_unittest.cc b/chrome/browser/extensions/extension_settings_storage_unittest.cc index a36bed9..f3080ac 100644 --- a/chrome/browser/extensions/extension_settings_storage_unittest.cc +++ b/chrome/browser/extensions/extension_settings_storage_unittest.cc @@ -33,48 +33,86 @@ std::string ToString(const std::set<std::string>& strings) { } // namespace +// Compares two possibly NULL values for equality, filling |error| with an +// appropriate error message if they're different. +bool ValuesEqual( + const Value* expected, const Value* actual, std::string* error) { + if (expected == actual) { + return true; + } + if (expected && !actual) { + *error = "Expected: " + GetJSON(*expected) + ", actual: NULL"; + return false; + } + if (actual && !expected) { + *error = "Expected: NULL, actual: " + GetJSON(*actual); + return false; + } + if (!expected->Equals(actual)) { + *error = + "Expected: " + GetJSON(*expected) + ", actual: " + GetJSON(*actual); + return false; + } + return true; +} + // Returns whether the result of a storage operation has the expected settings // and changed keys. testing::AssertionResult SettingsEq( - const char* _1, const char* _2, const char* _3, + 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()) { return testing::AssertionFailure() << "Expected: " << GetJSON(*expected_settings) << ", " << ToString(*expected_changed_keys) << "\n" << ", actual has error: " << actual.GetError(); } - if (expected_settings == NULL && actual.GetSettings() != NULL) { - return testing::AssertionFailure() << - "Expected NULL settings, actual: " << GetJSON(*actual.GetSettings()); + + if (!ValuesEqual(expected_settings, actual.GetSettings(), &error)) { + return testing::AssertionFailure() << "For settings, " << error; } - if (expected_changed_keys == NULL && actual.GetChangedKeys() != NULL) { + + if (!expected_changed_keys && actual.GetChangedKeys()) { return testing::AssertionFailure() << "Expected NULL changed keys, actual: " << ToString(*actual.GetChangedKeys()); } - if (expected_settings != NULL && actual.GetSettings() == NULL) { - return testing::AssertionFailure() << - "Expected: " << GetJSON(*expected_settings) << ", actual NULL"; - } - if (expected_changed_keys != NULL && actual.GetChangedKeys() == NULL) { + if (expected_changed_keys && !actual.GetChangedKeys()) { return testing::AssertionFailure() << "Expected: " << ToString(*expected_changed_keys) << ", actual NULL"; } - if (expected_settings != actual.GetSettings() && - !expected_settings->Equals(actual.GetSettings())) { - return testing::AssertionFailure() << - "Expected: " << GetJSON(*expected_settings) << - ", actual: " << GetJSON(*actual.GetSettings()); - } 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; + } + } + } + return testing::AssertionSuccess(); } @@ -82,10 +120,11 @@ ExtensionSettingsStorageTest::ExtensionSettingsStorageTest() : key1_("foo"), key2_("bar"), key3_("baz"), - empty_dict_(new DictionaryValue), - dict1_(new DictionaryValue), - dict12_(new DictionaryValue), - dict123_(new DictionaryValue), + empty_dict_(new DictionaryValue()), + dict1_(new DictionaryValue()), + dict3_(new DictionaryValue()), + dict12_(new DictionaryValue()), + dict123_(new DictionaryValue()), ui_thread_(BrowserThread::UI, MessageLoop::current()), file_thread_(BrowserThread::FILE, MessageLoop::current()) { val1_.reset(Value::CreateStringValue(key1_ + "Value")); @@ -111,6 +150,7 @@ ExtensionSettingsStorageTest::ExtensionSettingsStorageTest() set123_.insert(list123_.begin(), list123_.end()); dict1_->Set(key1_, val1_->DeepCopy()); + dict3_->Set(key3_, val3_->DeepCopy()); dict12_->Set(key1_, val1_->DeepCopy()); dict12_->Set(key2_, val2_->DeepCopy()); dict123_->Set(key1_, val1_->DeepCopy()); @@ -131,179 +171,179 @@ void ExtensionSettingsStorageTest::TearDown() { } TEST_P(ExtensionSettingsStorageTest, GetWhenEmpty) { - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, GetWithSingleValue) { - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), &set1_, storage_->Set(key1_, *val1_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key2_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, GetWithMultipleValues) { - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), &set12_, storage_->Set(*dict12_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, RemoveWhenEmpty) { - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, RemoveWithSingleValue) { - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), &set1_, storage_->Set(*dict1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set1_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key2_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, RemoveWithMultipleValues) { - EXPECT_PRED_FORMAT3(SettingsEq, - dict123_.get(), &set123_, storage_->Set(*dict123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set3_, storage_->Remove(key3_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get()); - - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set12_, storage_->Remove(list12_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, SetWhenOverwriting) { DictionaryValue key1_val2; key1_val2.Set(key1_, val2_->DeepCopy()); - EXPECT_PRED_FORMAT3(SettingsEq, - &key1_val2, &set1_, storage_->Set(key1_, *val2_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), &set12_, storage_->Set(*dict12_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key3_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(list1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get(list12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), NULL, storage_->Get(list13_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, ClearWhenEmpty) { - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Clear()); - - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, ClearWhenNotEmpty) { - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), &set12_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set12_, storage_->Clear()); - - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(empty_list_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(list123_)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } // Dots should be allowed in key names; they shouldn't be interpreted as @@ -318,33 +358,35 @@ TEST_P(ExtensionSettingsStorageTest, DotsInKeyNames) { DictionaryValue dot_dict; dot_dict.SetWithoutPathExpansion(dot_key, dot_value.DeepCopy()); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(dot_key)); - - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, &dot_set, storage_->Set(dot_key, dot_value)); - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, &empty_set_, storage_->Set(dot_key, dot_value)); - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, NULL, storage_->Get(dot_key)); - - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &dot_set, storage_->Remove(dot_key)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Remove(dot_key)); - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, &dot_set, storage_->Set(dot_dict)); - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, NULL, storage_->Get(dot_list)); - EXPECT_PRED_FORMAT3(SettingsEq, - &dot_dict, NULL, storage_->Get()); - - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &dot_set, storage_->Remove(dot_list)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get(dot_key)); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get()); + 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()); } TEST_P(ExtensionSettingsStorageTest, DotsInKeyNamesWithDicts) { @@ -355,12 +397,12 @@ TEST_P(ExtensionSettingsStorageTest, DotsInKeyNamesWithDicts) { std::set<std::string> changed_keys; changed_keys.insert("foo"); - EXPECT_PRED_FORMAT3(SettingsEq, - &outer_dict, &changed_keys, storage_->Set(outer_dict)); - EXPECT_PRED_FORMAT3(SettingsEq, - &outer_dict, NULL, storage_->Get("foo")); - EXPECT_PRED_FORMAT3(SettingsEq, - empty_dict_.get(), NULL, storage_->Get("foo.bar")); + 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")); } TEST_P(ExtensionSettingsStorageTest, ComplexChangedKeysScenarios) { @@ -371,66 +413,82 @@ TEST_P(ExtensionSettingsStorageTest, ComplexChangedKeysScenarios) { std::vector<std::string> complex_list; std::set<std::string> complex_set; DictionaryValue complex_dict; + DictionaryValue complex_changed_dict; - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), &set1_, storage_->Set(key1_, *val1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), &empty_set_, storage_->Set(key1_, *val1_)); + 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_FORMAT3(SettingsEq, - &complex_dict, &set1_, storage_->Set(key1_, *val2_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set1_, storage_->Remove(key1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Remove(key1_)); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict1_.get(), &set1_, storage_->Set(key1_, *val1_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set1_, storage_->Clear()); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Clear()); - - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), &set12_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict12_.get(), &empty_set_, storage_->Set(*dict12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - dict123_.get(), &set3_, storage_->Set(*dict123_)); + 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_FORMAT3(SettingsEq, - &complex_dict, &complex_set, storage_->Set(complex_dict)); - - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &set12_, storage_->Remove(list12_)); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Remove(list12_)); + 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_FORMAT3(SettingsEq, - NULL, &complex_set, storage_->Remove(complex_list)); - + 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_FORMAT3(SettingsEq, - NULL, &complex_set, storage_->Clear()); - EXPECT_PRED_FORMAT3(SettingsEq, - NULL, &empty_set_, storage_->Clear()); + EXPECT_PRED_FORMAT4(SettingsEq, + NULL, &complex_changed_dict, &complex_set, storage_->Clear()); + EXPECT_PRED_FORMAT4(SettingsEq, + NULL, empty_dict_.get(), &empty_set_, storage_->Clear()); } diff --git a/chrome/browser/extensions/extension_settings_storage_unittest.h b/chrome/browser/extensions/extension_settings_storage_unittest.h index 8f306ed..b78975e 100644 --- a/chrome/browser/extensions/extension_settings_storage_unittest.h +++ b/chrome/browser/extensions/extension_settings_storage_unittest.h @@ -8,12 +8,15 @@ #include "testing/gtest/include/gtest/gtest.h" +#include "base/bind.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop.h" #include "base/scoped_temp_dir.h" #include "base/task.h" #include "chrome/browser/extensions/extension_settings_backend.h" +#include "chrome/browser/extensions/extension_settings_frontend.h" +#include "chrome/test/base/testing_profile.h" #include "content/browser/browser_thread.h" // Parameter type for the value-parameterized tests. @@ -61,6 +64,7 @@ class ExtensionSettingsStorageTest scoped_ptr<DictionaryValue> empty_dict_; scoped_ptr<DictionaryValue> dict1_; + scoped_ptr<DictionaryValue> dict3_; scoped_ptr<DictionaryValue> dict12_; scoped_ptr<DictionaryValue> dict123_; diff --git a/chrome/browser/extensions/extension_settings_sync_unittest.cc b/chrome/browser/extensions/extension_settings_sync_unittest.cc index f9b34f9..7073475 100644 --- a/chrome/browser/extensions/extension_settings_sync_unittest.cc +++ b/chrome/browser/extensions/extension_settings_sync_unittest.cc @@ -12,10 +12,12 @@ #include "base/scoped_temp_dir.h" #include "base/task.h" #include "chrome/browser/extensions/extension_settings_backend.h" +#include "chrome/browser/extensions/extension_settings_frontend.h" #include "chrome/browser/extensions/extension_settings_storage_cache.h" #include "chrome/browser/extensions/extension_settings_sync_util.h" #include "chrome/browser/extensions/syncable_extension_settings_storage.h" #include "chrome/browser/sync/api/sync_change_processor.h" +#include "chrome/test/base/testing_profile.h" #include "content/browser/browser_thread.h" // TODO(kalman): Integration tests for sync. @@ -103,7 +105,8 @@ class MockSyncChangeProcessor : public SyncChangeProcessor { if (matching_changes.empty()) { ADD_FAILURE() << "No matching changes for " << extension_id << "/" << key << " (out of " << changes_.size() << ")"; - return ExtensionSettingSyncData(SyncChange::ACTION_INVALID, "", "", NULL); + return ExtensionSettingSyncData( + SyncChange::ACTION_INVALID, "", "", new DictionaryValue()); } if (matching_changes.size() != 1u) { ADD_FAILURE() << matching_changes.size() << " matching changes for " << @@ -116,25 +119,27 @@ class MockSyncChangeProcessor : public SyncChangeProcessor { ExtensionSettingSyncDataList changes_; }; +// To be called as a callback from ExtensionSettingsFrontend::RunWithSettings. +void AssignSettings( + ExtensionSettingsBackend** dst, ExtensionSettingsBackend* src) { + *dst = src; +} + } // namespace class ExtensionSettingsSyncTest : public testing::Test { public: ExtensionSettingsSyncTest() : ui_thread_(BrowserThread::UI, MessageLoop::current()), - file_thread_(BrowserThread::FILE, MessageLoop::current()) {} - - virtual void SetUp() OVERRIDE { - ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); - // TODO(kalman): Use ExtensionSettingsFrontend here? It would test more - // code paths. - backend_.reset(new ExtensionSettingsBackend(temp_dir_.path())); + file_thread_(BrowserThread::FILE, MessageLoop::current()), + frontend_(&profile_), + backend_(NULL) { } - virtual void TearDown() OVERRIDE { - // Must do this explicitly here so that it's destroyed before the - // message loops are. - backend_.reset(); + virtual void SetUp() OVERRIDE { + frontend_.RunWithBackend(base::Bind(&AssignSettings, &backend_)); + MessageLoop::current()->RunAllPending(); + ASSERT_TRUE(backend_ != NULL); } protected: @@ -146,10 +151,7 @@ class ExtensionSettingsSyncTest : public testing::Test { backend_->GetStorage(extension_id)); } - MockSyncChangeProcessor sync_; - scoped_ptr<ExtensionSettingsBackend> backend_; - - // Gets all the sync data from backend_ as a map from extension id to its + // Gets all the sync data from |backend_| as a map from extension id to its // sync data. std::map<std::string, ExtensionSettingSyncDataList> GetAllSyncData() { SyncDataList as_list = @@ -163,13 +165,17 @@ class ExtensionSettingsSyncTest : public testing::Test { return as_map; } - private: - ScopedTempDir temp_dir_; - // Need these so that the DCHECKs for running on FILE or UI threads pass. MessageLoop message_loop_; BrowserThread ui_thread_; BrowserThread file_thread_; + + MockSyncChangeProcessor sync_; + TestingProfile profile_; + ExtensionSettingsFrontend frontend_; + + // Get from frontend_->RunWithBackend, so weak reference. + ExtensionSettingsBackend* backend_; }; TEST_F(ExtensionSettingsSyncTest, NoDataDoesNotInvokeSync) { diff --git a/chrome/browser/extensions/in_memory_extension_settings_storage.cc b/chrome/browser/extensions/in_memory_extension_settings_storage.cc index 1cb58d5..ee50721 100644 --- a/chrome/browser/extensions/in_memory_extension_settings_storage.cc +++ b/chrome/browser/extensions/in_memory_extension_settings_storage.cc @@ -29,11 +29,11 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get( settings->SetWithoutPathExpansion(*it, value->DeepCopy()); } } - return Result(settings, NULL); + return Result(settings, NULL, NULL); } ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Get() { - return Result(storage_.DeepCopy(), NULL); + return Result(storage_.DeepCopy(), NULL, NULL); } ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Set( @@ -45,11 +45,14 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Set( ExtensionSettingsStorage::Result 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) { Value* old_value = NULL; - storage_.GetWithoutPathExpansion(*it, &old_value); + 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)) { @@ -57,7 +60,7 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Set( storage_.SetWithoutPathExpansion(*it, new_value->DeepCopy()); } } - return Result(settings.DeepCopy(), changed_keys); + return Result(settings.DeepCopy(), old_settings, changed_keys); } ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Remove( @@ -67,14 +70,17 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Remove( ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Remove( const std::vector<std::string>& keys) { + DictionaryValue* old_settings = new DictionaryValue(); std::set<std::string>* changed_keys = new std::set<std::string>(); for (std::vector<std::string>::const_iterator it = keys.begin(); it != keys.end(); ++it) { - if (storage_.RemoveWithoutPathExpansion(*it, NULL)) { + Value* old_value = NULL; + if (storage_.RemoveWithoutPathExpansion(*it, &old_value)) { + old_settings->SetWithoutPathExpansion(*it, old_value); changed_keys->insert(*it); } } - return Result(NULL, changed_keys); + return Result(NULL, old_settings, changed_keys); } ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Clear() { @@ -83,6 +89,7 @@ ExtensionSettingsStorage::Result InMemoryExtensionSettingsStorage::Clear() { it != storage_.end_keys(); ++it) { changed_keys->insert(*it); } - storage_.Clear(); - return Result(NULL, changed_keys); + DictionaryValue* old_settings = new DictionaryValue(); + storage_.Swap(old_settings); + return Result(NULL, old_settings, changed_keys); } diff --git a/chrome/browser/extensions/syncable_extension_settings_storage.cc b/chrome/browser/extensions/syncable_extension_settings_storage.cc index f9820f4..8ec5d17 100644 --- a/chrome/browser/extensions/syncable_extension_settings_storage.cc +++ b/chrome/browser/extensions/syncable_extension_settings_storage.cc @@ -11,8 +11,14 @@ #include "content/browser/browser_thread.h" SyncableExtensionSettingsStorage::SyncableExtensionSettingsStorage( - std::string extension_id, ExtensionSettingsStorage* delegate) - : extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) { + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >& + observers, + const std::string& extension_id, + ExtensionSettingsStorage* delegate) + : observers_(observers), + extension_id_(extension_id), + delegate_(delegate), + sync_processor_(NULL) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); } @@ -118,7 +124,7 @@ SyncError SyncableExtensionSettingsStorage::StartSyncing( syncable::EXTENSION_SETTINGS); } - DictionaryValue* settings = maybe_settings.GetSettings(); + const DictionaryValue* settings = maybe_settings.GetSettings(); if (sync_state.empty()) { return SendLocalSettingsToSync(*settings); } @@ -224,43 +230,88 @@ std::vector<SyncError> SyncableExtensionSettingsStorage::ProcessSyncChanges( DCHECK(!sync_changes.empty()) << "No sync changes for " << extension_id_; std::vector<SyncError> errors; + ExtensionSettingChanges::Builder changes; for (ExtensionSettingSyncDataList::const_iterator it = sync_changes.begin(); it != sync_changes.end(); ++it) { DCHECK_EQ(extension_id_, it->extension_id()); + + const std::string& key = it->key(); + const Value& value = it->value(); + + scoped_ptr<Value> current_value; + { + Result 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(), + syncable::EXTENSION_SETTINGS)); + continue; + } + const DictionaryValue* settings = maybe_settings.GetSettings(); + if (settings) { + Value* value = NULL; + if (settings->GetWithoutPathExpansion(key, &value)) { + current_value.reset(value->DeepCopy()); + } + } + } + + SyncError error; + switch (it->change_type()) { case SyncChange::ACTION_ADD: - case SyncChange::ACTION_UPDATE: { - synced_keys_.insert(it->key()); - Result result = delegate_->Set(it->key(), it->value()); - if (result.HasError()) { - errors.push_back(SyncError( - FROM_HERE, - std::string("Error pushing sync change to local settings: ") + - result.GetError(), - syncable::EXTENSION_SETTINGS)); + if (!current_value.get()) { + error = OnSyncAdd(key, value.DeepCopy(), &changes); + } else { + // Already a value; hopefully a local change has beaten sync in a + // race and it's not a bug, so pretend it's an update. + LOG(WARNING) << "Got add from sync for existing setting " << + extension_id_ << "/" << key; + error = OnSyncUpdate( + key, current_value.release(), value.DeepCopy(), &changes); } break; - } - case SyncChange::ACTION_DELETE: { - synced_keys_.erase(it->key()); - Result result = delegate_->Remove(it->key()); - if (result.HasError()) { - errors.push_back(SyncError( - FROM_HERE, - std::string("Error removing sync change from local settings: ") + - result.GetError(), - syncable::EXTENSION_SETTINGS)); + case SyncChange::ACTION_UPDATE: + if (current_value.get()) { + error = OnSyncUpdate( + key, current_value.release(), value.DeepCopy(), &changes); + } else { + // Similarly, pretend it's an add. + LOG(WARNING) << "Got update from sync for nonexistent setting" << + extension_id_ << "/" << key; + error = OnSyncAdd(key, value.DeepCopy(), &changes); + } + break; + + case SyncChange::ACTION_DELETE: + if (current_value.get()) { + error = OnSyncDelete(key, current_value.release(), &changes); + } else { + // Similarly, ignore it. + LOG(WARNING) << "Got delete from sync for nonexistent setting " << + extension_id_ << "/" << key; } break; - } default: NOTREACHED(); } + + if (error.IsSet()) { + errors.push_back(error); + } } + observers_->Notify( + &ExtensionSettingsObserver::OnSettingsChanged, + static_cast<Profile*>(NULL), + extension_id_, + changes.Build()); + return errors; } @@ -307,11 +358,18 @@ void SyncableExtensionSettingsStorage::SendDeletesToSync( SyncChangeList changes; for (std::set<std::string>::const_iterator it = keys.begin(); it != keys.end(); ++it) { - DCHECK(synced_keys_.count(*it)); + if (!synced_keys_.count(*it)) { + LOG(WARNING) << "Deleted " << *it << " but no entry in synced_keys"; + continue; + } changes.push_back( extension_settings_sync_util::CreateDelete(extension_id_, *it)); } + if (changes.empty()) { + return; + } + SyncError error = sync_processor_->ProcessSyncChanges(FROM_HERE, changes); if (error.IsSet()) { LOG(WARNING) << "Failed to send changes to sync: " << error.message(); @@ -323,3 +381,58 @@ void SyncableExtensionSettingsStorage::SendDeletesToSync( synced_keys_.erase(*it); } } + +SyncError SyncableExtensionSettingsStorage::OnSyncAdd( + const std::string& key, + Value* new_value, + ExtensionSettingChanges::Builder* changes) { + DCHECK(new_value); + synced_keys_.insert(key); + Result result = delegate_->Set(key, *new_value); + if (result.HasError()) { + return SyncError( + FROM_HERE, + std::string("Error pushing sync add to local settings: ") + + result.GetError(), + syncable::EXTENSION_SETTINGS); + } + changes->AppendChange(key, NULL, new_value); + return SyncError(); +} + +SyncError SyncableExtensionSettingsStorage::OnSyncUpdate( + const std::string& key, + Value* old_value, + Value* new_value, + ExtensionSettingChanges::Builder* changes) { + DCHECK(old_value); + DCHECK(new_value); + Result result = delegate_->Set(key, *new_value); + if (result.HasError()) { + return SyncError( + FROM_HERE, + std::string("Error pushing sync update to local settings: ") + + result.GetError(), + syncable::EXTENSION_SETTINGS); + } + changes->AppendChange(key, old_value, new_value); + return SyncError(); +} + +SyncError SyncableExtensionSettingsStorage::OnSyncDelete( + const std::string& key, + Value* old_value, + ExtensionSettingChanges::Builder* changes) { + DCHECK(old_value); + synced_keys_.erase(key); + Result result = delegate_->Remove(key); + if (result.HasError()) { + return SyncError( + FROM_HERE, + std::string("Error pushing sync remove to local settings: ") + + result.GetError(), + syncable::EXTENSION_SETTINGS); + } + changes->AppendChange(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 39f4607..499d89c 100644 --- a/chrome/browser/extensions/syncable_extension_settings_storage.h +++ b/chrome/browser/extensions/syncable_extension_settings_storage.h @@ -8,7 +8,9 @@ #include "base/compiler_specific.h" #include "base/memory/weak_ptr.h" +#include "base/observer_list_threadsafe.h" #include "base/values.h" +#include "chrome/browser/extensions/extension_settings_observer.h" #include "chrome/browser/extensions/extension_settings_storage.h" #include "chrome/browser/extensions/extension_setting_sync_data.h" #include "chrome/browser/sync/api/syncable_service.h" @@ -18,7 +20,9 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { public: explicit SyncableExtensionSettingsStorage( - std::string extension_id, + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> >& + observers, + const std::string& extension_id, // Ownership taken. ExtensionSettingsStorage* delegate); @@ -62,6 +66,26 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { SyncError OverwriteLocalSettingsWithSync( const DictionaryValue& sync_state, const DictionaryValue& settings); + // Called when an Add/Update/Remove comes from sync. Ownership of Value*s + // are taken. + SyncError OnSyncAdd( + const std::string& key, + Value* new_value, + ExtensionSettingChanges::Builder* changes); + SyncError OnSyncUpdate( + const std::string& key, + Value* old_value, + Value* new_value, + ExtensionSettingChanges::Builder* changes); + SyncError OnSyncDelete( + const std::string& key, + Value* old_value, + ExtensionSettingChanges::Builder* changes); + + // List of observers to settings changes. + const scoped_refptr<ObserverListThreadSafe<ExtensionSettingsObserver> > + observers_; + // Id of the extension these settings are for. std::string const extension_id_; @@ -72,7 +96,8 @@ class SyncableExtensionSettingsStorage : public ExtensionSettingsStorage { // StartSyncing and StopSyncing). SyncChangeProcessor* sync_processor_; - // Keys of the settings that are currently being synced. + // Keys of the settings that are currently being synced. Only used to decide + // whether an ACTION_ADD or ACTION_UPDATE should be sent to sync on a Set(). std::set<std::string> synced_keys_; DISALLOW_COPY_AND_ASSIGN(SyncableExtensionSettingsStorage); |