diff options
Diffstat (limited to 'chromium/components/sync_preferences')
8 files changed, 298 insertions, 145 deletions
diff --git a/chromium/components/sync_preferences/BUILD.gn b/chromium/components/sync_preferences/BUILD.gn index a3ae9d2535d..49ca9514a71 100644 --- a/chromium/components/sync_preferences/BUILD.gn +++ b/chromium/components/sync_preferences/BUILD.gn @@ -63,6 +63,7 @@ source_set("unit_tests") { ":test_support", "//components/pref_registry", "//components/prefs", + "//components/prefs:test_support", "//components/sync", "//components/sync:test_support_model", "//testing/gtest", diff --git a/chromium/components/sync_preferences/pref_model_associator.cc b/chromium/components/sync_preferences/pref_model_associator.cc index 401d614defd..2aeff8dbbe1 100644 --- a/chromium/components/sync_preferences/pref_model_associator.cc +++ b/chromium/components/sync_preferences/pref_model_associator.cc @@ -375,7 +375,7 @@ syncer::SyncError PrefModelAssociator::ProcessSyncChanges( // Windows client, the Windows client does not support // kConfirmToQuitEnabled. Ignore updates from these preferences. std::string pref_name = pref_specifics.name(); - if (!IsPrefRegistered(pref_name.c_str())) + if (!IsPrefRegistered(pref_name)) continue; if (iter->change_type() == syncer::SyncChange::ACTION_DELETE) { @@ -406,6 +406,7 @@ syncer::SyncError PrefModelAssociator::ProcessSyncChanges( return syncer::SyncError(); } +// static base::Value* PrefModelAssociator::ReadPreferenceSpecifics( const sync_pb::PreferenceSpecifics& preference) { base::JSONReader reader; @@ -425,10 +426,10 @@ bool PrefModelAssociator::IsPrefSynced(const std::string& name) const { void PrefModelAssociator::AddSyncedPrefObserver(const std::string& name, SyncedPrefObserver* observer) { - std::unique_ptr<SyncedPrefObserverList>& observers = + std::unique_ptr<base::ObserverList<SyncedPrefObserver>>& observers = synced_pref_observers_[name]; if (!observers) - observers = std::make_unique<SyncedPrefObserverList>(); + observers = std::make_unique<base::ObserverList<SyncedPrefObserver>>(); observers->AddObserver(observer); } @@ -439,18 +440,7 @@ void PrefModelAssociator::RemoveSyncedPrefObserver( auto observer_iter = synced_pref_observers_.find(name); if (observer_iter == synced_pref_observers_.end()) return; - SyncedPrefObserverList* observers = observer_iter->second.get(); - observers->RemoveObserver(observer); -} - -void PrefModelAssociator::SetPrefModelAssociatorClientForTesting( - const PrefModelAssociatorClient* client) { - DCHECK(!client_); - client_ = client; -} - -std::set<std::string> PrefModelAssociator::registered_preferences() const { - return registered_preferences_; + observer_iter->second->RemoveObserver(observer); } void PrefModelAssociator::RegisterPref(const char* name) { @@ -458,7 +448,7 @@ void PrefModelAssociator::RegisterPref(const char* name) { registered_preferences_.insert(name); } -bool PrefModelAssociator::IsPrefRegistered(const char* name) { +bool PrefModelAssociator::IsPrefRegistered(const std::string& name) const { return registered_preferences_.count(name) > 0; } @@ -475,7 +465,7 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) { if (!preference) return; - if (!IsPrefRegistered(name.c_str())) + if (!IsPrefRegistered(name)) return; // We are not syncing this preference. syncer::SyncChangeList changes; @@ -522,8 +512,7 @@ void PrefModelAssociator::NotifySyncedPrefObservers(const std::string& path, auto observer_iter = synced_pref_observers_.find(path); if (observer_iter == synced_pref_observers_.end()) return; - SyncedPrefObserverList* observers = observer_iter->second.get(); - for (auto& observer : *observers) + for (auto& observer : *observer_iter->second) observer.OnSyncedPrefChanged(path, from_sync); } diff --git a/chromium/components/sync_preferences/pref_model_associator.h b/chromium/components/sync_preferences/pref_model_associator.h index 8832be194e7..d47a36b4a64 100644 --- a/chromium/components/sync_preferences/pref_model_associator.h +++ b/chromium/components/sync_preferences/pref_model_associator.h @@ -61,10 +61,6 @@ class PrefModelAssociator : public syncer::SyncableService { std::unique_ptr<syncer::SyncErrorFactory> sync_error_factory) override; void StopSyncing(syncer::ModelType type) override; - // Returns the list of preference names that are registered as syncable, and - // hence should be monitored for changes. - std::set<std::string> registered_preferences() const; - // Register a preference with the specified name for syncing. We do not care // about the type at registration time, but when changes arrive from the // syncer, we check if they can be applied and if not drop them. @@ -72,9 +68,6 @@ class PrefModelAssociator : public syncer::SyncableService { // begins). virtual void RegisterPref(const char* name); - // Returns true if the specified preference is registered for syncing. - virtual bool IsPrefRegistered(const char* name); - // Process a local preference change. This can trigger new SyncChanges being // sent to the syncer. virtual void ProcessPrefChange(const std::string& name); @@ -95,15 +88,13 @@ class PrefModelAssociator : public syncer::SyncableService { bool CreatePrefSyncData(const std::string& name, const base::Value& value, syncer::SyncData* sync_data) const; - - // Extract preference value from sync specifics. - base::Value* ReadPreferenceSpecifics( - const sync_pb::PreferenceSpecifics& specifics); - // Returns true if the pref under the given name is pulled down from sync. // Note this does not refer to SYNCABLE_PREF. bool IsPrefSynced(const std::string& name) const; + // Returns true if the specified preference is registered for syncing. + bool IsPrefRegistered(const std::string& name) const; + // Adds a SyncedPrefObserver to watch for changes to a specific pref. void AddSyncedPrefObserver(const std::string& name, SyncedPrefObserver* observer); @@ -115,19 +106,13 @@ class PrefModelAssociator : public syncer::SyncableService { // Returns the PrefModelAssociatorClient for this object. const PrefModelAssociatorClient* client() const { return client_; } - // Set the PrefModelAssociatorClient to use for that object during tests. - void SetPrefModelAssociatorClientForTesting( - const PrefModelAssociatorClient* client); - // Register callback method which will get called at the end of // PrefModelAssociator::MergeDataAndStartSyncing(). void RegisterMergeDataFinishedCallback(const base::Closure& callback); - protected: + private: friend class PrefServiceSyncableTest; - typedef std::map<std::string, syncer::SyncData> SyncDataMap; - // Create an association for a given preference. If |sync_pref| is valid, // signifying that sync has data for this preference, we reconcile their data // with ours and append a new UPDATE SyncChange to |sync_changes|. If @@ -143,9 +128,14 @@ class PrefModelAssociator : public syncer::SyncableService { static std::unique_ptr<base::Value> MergeListValues( const base::Value& from_value, const base::Value& to_value); + static base::Value MergeDictionaryValues(const base::Value& from_value, const base::Value& to_value); + // Extract preference value from sync specifics. + static base::Value* ReadPreferenceSpecifics( + const sync_pb::PreferenceSpecifics& specifics); + // Do we have an active association between the preferences and sync models? // Set when start syncing, reset in StopSyncing. While this is not set, we // ignore any local preference changes (when we start syncing we will look @@ -184,17 +174,14 @@ class PrefModelAssociator : public syncer::SyncableService { // PRIORITY_PREFERENCES. syncer::ModelType type_; - private: + void NotifySyncedPrefObservers(const std::string& path, bool from_sync) const; + // Map prefs to lists of observers. Observers will receive notification when // a pref changes, including the detail of whether or not the change came // from sync. - using SyncedPrefObserverList = base::ObserverList<SyncedPrefObserver>; - using SyncedPrefObserverMap = - base::hash_map<std::string, std::unique_ptr<SyncedPrefObserverList>>; - - void NotifySyncedPrefObservers(const std::string& path, bool from_sync) const; - - SyncedPrefObserverMap synced_pref_observers_; + base::hash_map<std::string, + std::unique_ptr<base::ObserverList<SyncedPrefObserver>>> + synced_pref_observers_; const PrefModelAssociatorClient* client_; // Weak. std::vector<base::Closure> callback_list_; diff --git a/chromium/components/sync_preferences/pref_service_syncable.cc b/chromium/components/sync_preferences/pref_service_syncable.cc index 96e192dbf69..478d278aa3e 100644 --- a/chromium/components/sync_preferences/pref_service_syncable.cc +++ b/chromium/components/sync_preferences/pref_service_syncable.cc @@ -174,15 +174,6 @@ void PrefServiceSyncable::RegisterMergeDataFinishedCallback( pref_sync_associator_.RegisterMergeDataFinishedCallback(callback); } -// Set the PrefModelAssociatorClient to use for that object during tests. -void PrefServiceSyncable::SetPrefModelAssociatorClientForTesting( - const PrefModelAssociatorClient* pref_model_associator_client) { - pref_sync_associator_.SetPrefModelAssociatorClientForTesting( - pref_model_associator_client); - priority_pref_sync_associator_.SetPrefModelAssociatorClientForTesting( - pref_model_associator_client); -} - void PrefServiceSyncable::AddRegisteredSyncablePreference( const std::string& path, uint32_t flags) { diff --git a/chromium/components/sync_preferences/pref_service_syncable.h b/chromium/components/sync_preferences/pref_service_syncable.h index f425965a986..656b10e8201 100644 --- a/chromium/components/sync_preferences/pref_service_syncable.h +++ b/chromium/components/sync_preferences/pref_service_syncable.h @@ -40,7 +40,7 @@ class PrefServiceSyncable : public PrefService { std::unique_ptr<PrefValueStore> pref_value_store, scoped_refptr<PersistentPrefStore> user_prefs, scoped_refptr<user_prefs::PrefRegistrySyncable> pref_registry, - const PrefModelAssociatorClient* pref_model_associato_client, + const PrefModelAssociatorClient* pref_model_associator_client, base::RepeatingCallback<void(PersistentPrefStore::PrefReadError)> read_error_callback, bool async); @@ -91,11 +91,6 @@ class PrefServiceSyncable : public PrefService { void RemoveSyncedPrefObserver(const std::string& name, SyncedPrefObserver* observer); - protected: - // Set the PrefModelAssociatorClient to use for that object during tests. - void SetPrefModelAssociatorClientForTesting( - const PrefModelAssociatorClient* pref_model_associator_client); - private: friend class PrefModelAssociator; diff --git a/chromium/components/sync_preferences/pref_service_syncable_unittest.cc b/chromium/components/sync_preferences/pref_service_syncable_unittest.cc index d79abb4c914..1129b294554 100644 --- a/chromium/components/sync_preferences/pref_service_syncable_unittest.cc +++ b/chromium/components/sync_preferences/pref_service_syncable_unittest.cc @@ -13,10 +13,11 @@ #include "base/json/json_writer.h" #include "base/macros.h" #include "base/memory/ptr_util.h" -#include "base/message_loop/message_loop.h" #include "base/strings/utf_string_conversions.h" #include "components/pref_registry/pref_registry_syncable.h" +#include "components/prefs/pref_notifier_impl.h" #include "components/prefs/scoped_user_pref_update.h" +#include "components/prefs/testing_pref_store.h" #include "components/sync/model/sync_change.h" #include "components/sync/model/sync_data.h" #include "components/sync/model/sync_error_factory_mock.h" @@ -30,6 +31,7 @@ using syncer::SyncChange; using syncer::SyncData; +using testing::NotNull; namespace sync_preferences { @@ -40,6 +42,7 @@ const char kExampleUrl1[] = "http://example.com/1"; const char kExampleUrl2[] = "http://example.com/2"; const char kStringPrefName[] = "string_pref_name"; const char kListPrefName[] = "list_pref_name"; +const char kDictPrefName[] = "dict_pref_name"; const char kUnsyncedPreferenceName[] = "nonsense_pref_name"; const char kUnsyncedPreferenceDefaultValue[] = "default"; const char kDefaultCharsetPrefName[] = "default_charset"; @@ -50,25 +53,6 @@ void Increment(int* num) { (*num)++; } -class TestPrefModelAssociatorClient : public PrefModelAssociatorClient { - public: - TestPrefModelAssociatorClient() {} - ~TestPrefModelAssociatorClient() override {} - - // PrefModelAssociatorClient implementation. - bool IsMergeableListPreference(const std::string& pref_name) const override { - return pref_name == kListPrefName; - } - - bool IsMergeableDictionaryPreference( - const std::string& pref_name) const override { - return false; - } - - private: - DISALLOW_COPY_AND_ASSIGN(TestPrefModelAssociatorClient); -}; - class TestSyncProcessorStub : public syncer::SyncChangeProcessor { public: explicit TestSyncProcessorStub(syncer::SyncChangeList* output) @@ -101,11 +85,9 @@ class PrefServiceSyncableTest : public testing::Test { public: PrefServiceSyncableTest() : pref_sync_service_(nullptr), - test_processor_(nullptr), next_pref_remote_sync_node_id_(0) {} void SetUp() override { - prefs_.SetPrefModelAssociatorClientForTesting(&client_); prefs_.registry()->RegisterStringPref(kUnsyncedPreferenceName, kUnsyncedPreferenceDefaultValue); prefs_.registry()->RegisterStringPref( @@ -117,7 +99,7 @@ class PrefServiceSyncableTest : public testing::Test { kDefaultCharsetPrefName, kDefaultCharsetValue, user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); - pref_sync_service_ = reinterpret_cast<PrefModelAssociator*>( + pref_sync_service_ = static_cast<PrefModelAssociator*>( prefs_.GetSyncableService(syncer::PREFERENCES)); ASSERT_TRUE(pref_sync_service_); } @@ -155,9 +137,9 @@ class PrefServiceSyncableTest : public testing::Test { void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data, syncer::SyncChangeList* output) { - test_processor_ = new TestSyncProcessorStub(output); syncer::SyncMergeResult r = pref_sync_service_->MergeDataAndStartSyncing( - syncer::PREFERENCES, initial_data, base::WrapUnique(test_processor_), + syncer::PREFERENCES, initial_data, + std::make_unique<TestSyncProcessorStub>(output), std::make_unique<syncer::SyncErrorFactoryMock>()); EXPECT_FALSE(r.error().IsSet()); } @@ -185,22 +167,20 @@ class PrefServiceSyncableTest : public testing::Test { } bool IsSynced(const std::string& pref_name) { - return pref_sync_service_->registered_preferences().count(pref_name) > 0; + return pref_sync_service_->IsPrefSynced(pref_name); } - bool HasSyncData(const std::string& pref_name) { - return pref_sync_service_->IsPrefSynced(pref_name); + bool IsRegistered(const std::string& pref_name) { + return pref_sync_service_->IsPrefRegistered(pref_name.c_str()); } PrefService* GetPrefs() { return &prefs_; } TestingPrefServiceSyncable* GetTestingPrefService() { return &prefs_; } protected: - TestPrefModelAssociatorClient client_; TestingPrefServiceSyncable prefs_; PrefModelAssociator* pref_sync_service_; - TestSyncProcessorStub* test_processor_; int next_pref_remote_sync_node_id_; }; @@ -229,7 +209,7 @@ TEST_F(PrefServiceSyncableTest, ModelAssociationDoNotSyncDefaults) { syncer::SyncChangeList out; InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out); - EXPECT_TRUE(IsSynced(kStringPrefName)); + EXPECT_TRUE(IsRegistered(kStringPrefName)); EXPECT_TRUE(pref->IsDefaultValue()); EXPECT_FALSE(FindValue(kStringPrefName, out).get()); } @@ -259,7 +239,6 @@ TEST_F(PrefServiceSyncableTest, ModelAssociationCloudHasData) { ListPrefUpdate update(GetPrefs(), kListPrefName); base::ListValue* url_list = update.Get(); url_list->AppendString(kExampleUrl0); - url_list->AppendString(kExampleUrl1); } syncer::SyncDataList in; @@ -267,7 +246,6 @@ TEST_F(PrefServiceSyncableTest, ModelAssociationCloudHasData) { AddToRemoteDataList(kStringPrefName, base::Value(kExampleUrl1), &in); base::ListValue urls_to_restore; urls_to_restore.AppendString(kExampleUrl1); - urls_to_restore.AppendString(kExampleUrl2); AddToRemoteDataList(kListPrefName, urls_to_restore, &in); AddToRemoteDataList(kDefaultCharsetPrefName, base::Value(kNonDefaultCharsetValue), &in); @@ -278,15 +256,259 @@ TEST_F(PrefServiceSyncableTest, ModelAssociationCloudHasData) { EXPECT_EQ(kExampleUrl1, prefs_.GetString(kStringPrefName)); + // No associator client is registered, so lists and dictionaries should not + // get merged (remote write wins). + auto expected_urls = std::make_unique<base::ListValue>(); + expected_urls->AppendString(kExampleUrl1); + EXPECT_FALSE(FindValue(kListPrefName, out)); + EXPECT_TRUE(GetPreferenceValue(kListPrefName).Equals(expected_urls.get())); + EXPECT_EQ(kNonDefaultCharsetValue, prefs_.GetString(kDefaultCharsetPrefName)); +} + +class TestPrefModelAssociatorClient : public PrefModelAssociatorClient { + public: + TestPrefModelAssociatorClient() {} + ~TestPrefModelAssociatorClient() override {} + + // PrefModelAssociatorClient implementation. + bool IsMergeableListPreference(const std::string& pref_name) const override { + return pref_name == kListPrefName; + } + + bool IsMergeableDictionaryPreference( + const std::string& pref_name) const override { + return true; + } + + private: + DISALLOW_COPY_AND_ASSIGN(TestPrefModelAssociatorClient); +}; + +class PrefServiceSyncableMergeTest : public testing::Test { + public: + PrefServiceSyncableMergeTest() + : pref_registry_( + base::MakeRefCounted<user_prefs::PrefRegistrySyncable>()), + pref_notifier_(new PrefNotifierImpl), + managed_prefs_(base::MakeRefCounted<TestingPrefStore>()), + user_prefs_(base::MakeRefCounted<TestingPrefStore>()), + prefs_( + std::unique_ptr<PrefNotifierImpl>(pref_notifier_), + std::make_unique<PrefValueStore>(managed_prefs_.get(), + new TestingPrefStore, + new TestingPrefStore, + new TestingPrefStore, + user_prefs_.get(), + new TestingPrefStore, + pref_registry_->defaults().get(), + pref_notifier_), + user_prefs_, + pref_registry_, + &client_, + base::BindRepeating(&PrefServiceSyncableMergeTest::HandleReadError), + /*async=*/false), + pref_sync_service_(nullptr), + next_pref_remote_sync_node_id_(0) {} + + void SetUp() override { + pref_registry_->RegisterStringPref(kUnsyncedPreferenceName, + kUnsyncedPreferenceDefaultValue); + pref_registry_->RegisterStringPref( + kStringPrefName, std::string(), + user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); + pref_registry_->RegisterListPref( + kListPrefName, user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); + pref_registry_->RegisterDictionaryPref( + kDictPrefName, user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); + pref_registry_->RegisterStringPref( + kDefaultCharsetPrefName, kDefaultCharsetValue, + user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); + + // Downcast to PrefModelAssociator so that tests can access its' specific + // behavior. This is a smell. The roles between PrefServiceSyncable and + // PrefModelAssociator are not clearly separated (and this test should only + // test against the SyncableService interface). + pref_sync_service_ = // static_cast<PrefModelAssociator*>( + prefs_.GetSyncableService(syncer::PREFERENCES); //); + ASSERT_THAT(pref_sync_service_, NotNull()); + } + + /// Empty stub for prefs_ error handling. + static void HandleReadError(PersistentPrefStore::PrefReadError error) {} + + syncer::SyncChange MakeRemoteChange(int64_t id, + const std::string& name, + const base::Value& value, + SyncChange::SyncChangeType type) { + std::string serialized; + JSONStringValueSerializer json(&serialized); + CHECK(json.Serialize(value)); + sync_pb::EntitySpecifics entity; + sync_pb::PreferenceSpecifics* pref_one = entity.mutable_preference(); + pref_one->set_name(name); + pref_one->set_value(serialized); + return syncer::SyncChange( + FROM_HERE, type, + syncer::SyncData::CreateRemoteData(id, entity, base::Time())); + } + + void AddToRemoteDataList(const std::string& name, + const base::Value& value, + syncer::SyncDataList* out) { + std::string serialized; + JSONStringValueSerializer json(&serialized); + ASSERT_TRUE(json.Serialize(value)); + sync_pb::EntitySpecifics one; + sync_pb::PreferenceSpecifics* pref_one = one.mutable_preference(); + pref_one->set_name(name); + pref_one->set_value(serialized); + out->push_back(SyncData::CreateRemoteData(++next_pref_remote_sync_node_id_, + one, base::Time())); + } + + void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data, + syncer::SyncChangeList* output) { + syncer::SyncMergeResult r = pref_sync_service_->MergeDataAndStartSyncing( + syncer::PREFERENCES, initial_data, + std::make_unique<TestSyncProcessorStub>(output), + std::make_unique<syncer::SyncErrorFactoryMock>()); + EXPECT_FALSE(r.error().IsSet()); + } + + const base::Value& GetPreferenceValue(const std::string& name) { + const PrefService::Preference* preference = + prefs_.FindPreference(name.c_str()); + return *preference->GetValue(); + } + + std::unique_ptr<base::Value> FindValue(const std::string& name, + const syncer::SyncChangeList& list) { + syncer::SyncChangeList::const_iterator it = list.begin(); + for (; it != list.end(); ++it) { + if (syncer::SyncDataLocal(it->sync_data()).GetTag() == name) { + return base::JSONReader::Read( + it->sync_data().GetSpecifics().preference().value()); + } + } + return nullptr; + } + + protected: + scoped_refptr<user_prefs::PrefRegistrySyncable> pref_registry_; + // Owned by prefs_; + PrefNotifierImpl* pref_notifier_; + scoped_refptr<TestingPrefStore> managed_prefs_; + scoped_refptr<TestingPrefStore> user_prefs_; + TestPrefModelAssociatorClient client_; + PrefServiceSyncable prefs_; + syncer::SyncableService* pref_sync_service_; + int next_pref_remote_sync_node_id_; +}; + +TEST_F(PrefServiceSyncableMergeTest, ShouldMergeSelectedListValues) { + { + ListPrefUpdate update(&prefs_, kListPrefName); + base::ListValue* url_list = update.Get(); + url_list->AppendString(kExampleUrl0); + url_list->AppendString(kExampleUrl1); + } + + base::ListValue urls_to_restore; + urls_to_restore.AppendString(kExampleUrl1); + urls_to_restore.AppendString(kExampleUrl2); + syncer::SyncDataList in; + AddToRemoteDataList(kListPrefName, urls_to_restore, &in); + + syncer::SyncChangeList out; + InitWithSyncDataTakeOutput(in, &out); + std::unique_ptr<base::ListValue> expected_urls(new base::ListValue); expected_urls->AppendString(kExampleUrl1); expected_urls->AppendString(kExampleUrl2); expected_urls->AppendString(kExampleUrl0); std::unique_ptr<base::Value> value(FindValue(kListPrefName, out)); ASSERT_TRUE(value.get()); - EXPECT_TRUE(value->Equals(expected_urls.get())); + EXPECT_TRUE(value->Equals(expected_urls.get())) << *value; EXPECT_TRUE(GetPreferenceValue(kListPrefName).Equals(expected_urls.get())); - EXPECT_EQ(kNonDefaultCharsetValue, prefs_.GetString(kDefaultCharsetPrefName)); +} + +// List preferences have special handling at association time due to our ability +// to merge the local and sync value. Make sure the merge logic doesn't merge +// managed preferences. +TEST_F(PrefServiceSyncableMergeTest, ManagedListPreferences) { + // Make the list of urls to restore on startup managed. + base::ListValue managed_value; + managed_value.AppendString(kExampleUrl0); + managed_value.AppendString(kExampleUrl1); + managed_prefs_->SetValue(kListPrefName, managed_value.CreateDeepCopy(), + WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); + + // Set a cloud version. + syncer::SyncDataList in; + base::ListValue urls_to_restore; + urls_to_restore.AppendString(kExampleUrl1); + urls_to_restore.AppendString(kExampleUrl2); + AddToRemoteDataList(kListPrefName, urls_to_restore, &in); + + // Start sync and verify the synced value didn't get merged. + { + syncer::SyncChangeList out; + InitWithSyncDataTakeOutput(in, &out); + EXPECT_FALSE(FindValue(kListPrefName, out).get()); + } + + // Changing the user's urls to restore on startup pref should not sync + // anything. + { + syncer::SyncChangeList out; + base::ListValue user_value; + user_value.AppendString("http://chromium.org"); + prefs_.Set(kListPrefName, user_value); + EXPECT_FALSE(FindValue(kListPrefName, out).get()); + } + + // An incoming sync transaction should change the user value, not the managed + // value. + base::ListValue sync_value; + sync_value.AppendString("http://crbug.com"); + syncer::SyncChangeList list; + list.push_back(MakeRemoteChange(1, kListPrefName, sync_value, + SyncChange::ACTION_UPDATE)); + pref_sync_service_->ProcessSyncChanges(FROM_HERE, list); + + const base::Value* managed_prefs_result; + ASSERT_TRUE(managed_prefs_->GetValue(kListPrefName, &managed_prefs_result)); + EXPECT_TRUE(managed_value.Equals(managed_prefs_result)); + // Get should return the managed value, too. + EXPECT_TRUE(managed_value.Equals(prefs_.Get(kListPrefName))); + // Verify the user pref value has the change. + EXPECT_TRUE(sync_value.Equals(prefs_.GetUserPrefValue(kListPrefName))); +} + +TEST_F(PrefServiceSyncableMergeTest, ShouldMergeSelectedDictionaryValues) { + { + DictionaryPrefUpdate update(&prefs_, kDictPrefName); + base::DictionaryValue* dict_value = update.Get(); + dict_value->Set("my_key1", std::make_unique<base::Value>("my_value1")); + dict_value->Set("my_key3", std::make_unique<base::Value>("my_value3")); + } + + base::DictionaryValue remote_update; + remote_update.Set("my_key2", std::make_unique<base::Value>("my_value2")); + syncer::SyncDataList in; + AddToRemoteDataList(kDictPrefName, remote_update, &in); + + syncer::SyncChangeList out; + InitWithSyncDataTakeOutput(in, &out); + + base::DictionaryValue expected_dict; + expected_dict.Set("my_key1", std::make_unique<base::Value>("my_value1")); + expected_dict.Set("my_key2", std::make_unique<base::Value>("my_value2")); + expected_dict.Set("my_key3", std::make_unique<base::Value>("my_value3")); + std::unique_ptr<base::Value> value(FindValue(kDictPrefName, out)); + ASSERT_TRUE(value.get()); + EXPECT_TRUE(value->Equals(&expected_dict)); + EXPECT_TRUE(GetPreferenceValue(kDictPrefName).Equals(&expected_dict)); } TEST_F(PrefServiceSyncableTest, FailModelAssociation) { @@ -354,8 +576,7 @@ TEST_F(PrefServiceSyncableTest, UpdatedSyncNodeActionAdd) { const base::Value& actual = GetPreferenceValue(kStringPrefName); EXPECT_TRUE(expected.Equals(&actual)); - EXPECT_EQ( - 1U, pref_sync_service_->registered_preferences().count(kStringPrefName)); + EXPECT_TRUE(pref_sync_service_->IsPrefSynced(kStringPrefName)); } TEST_F(PrefServiceSyncableTest, UpdatedSyncNodeUnknownPreference) { @@ -395,49 +616,6 @@ TEST_F(PrefServiceSyncableTest, ManagedPreferences) { EXPECT_TRUE(sync_value.Equals(prefs_.GetUserPref(kStringPrefName))); } -// List preferences have special handling at association time due to our ability -// to merge the local and sync value. Make sure the merge logic doesn't merge -// managed preferences. -TEST_F(PrefServiceSyncableTest, ManagedListPreferences) { - // Make the list of urls to restore on startup managed. - base::ListValue managed_value; - managed_value.AppendString(kExampleUrl0); - managed_value.AppendString(kExampleUrl1); - prefs_.SetManagedPref(kListPrefName, managed_value.CreateDeepCopy()); - - // Set a cloud version. - syncer::SyncDataList in; - syncer::SyncChangeList out; - base::ListValue urls_to_restore; - urls_to_restore.AppendString(kExampleUrl1); - urls_to_restore.AppendString(kExampleUrl2); - AddToRemoteDataList(kListPrefName, urls_to_restore, &in); - - // Start sync and verify the synced value didn't get merged. - InitWithSyncDataTakeOutput(in, &out); - EXPECT_FALSE(FindValue(kListPrefName, out).get()); - out.clear(); - - // Changing the user's urls to restore on startup pref should not sync - // anything. - base::ListValue user_value; - user_value.AppendString("http://chromium.org"); - prefs_.SetUserPref(kListPrefName, user_value.CreateDeepCopy()); - EXPECT_FALSE(FindValue(kListPrefName, out).get()); - - // An incoming sync transaction should change the user value, not the managed - // value. - base::ListValue sync_value; - sync_value.AppendString("http://crbug.com"); - syncer::SyncChangeList list; - list.push_back(MakeRemoteChange(1, kListPrefName, sync_value, - SyncChange::ACTION_UPDATE)); - pref_sync_service_->ProcessSyncChanges(FROM_HERE, list); - - EXPECT_TRUE(managed_value.Equals(prefs_.GetManagedPref(kListPrefName))); - EXPECT_TRUE(sync_value.Equals(prefs_.GetUserPref(kListPrefName))); -} - TEST_F(PrefServiceSyncableTest, DynamicManagedPreferences) { syncer::SyncChangeList out; InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out); @@ -501,7 +679,7 @@ TEST_F(PrefServiceSyncableTest, DynamicManagedDefaultPreferences) { syncer::SyncChangeList out; InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out); - EXPECT_TRUE(IsSynced(kStringPrefName)); + EXPECT_TRUE(IsRegistered(kStringPrefName)); EXPECT_TRUE(pref->IsDefaultValue()); EXPECT_FALSE(FindValue(kStringPrefName, out).get()); out.clear(); diff --git a/chromium/components/sync_preferences/testing_pref_service_syncable.cc b/chromium/components/sync_preferences/testing_pref_service_syncable.cc index 92fa102683d..e42f5020b72 100644 --- a/chromium/components/sync_preferences/testing_pref_service_syncable.cc +++ b/chromium/components/sync_preferences/testing_pref_service_syncable.cc @@ -4,6 +4,8 @@ #include "components/sync_preferences/testing_pref_service_syncable.h" +#include <memory> + #include "base/bind.h" #include "components/pref_registry/pref_registry_syncable.h" #include "components/prefs/pref_notifier_impl.h" @@ -31,7 +33,7 @@ TestingPrefServiceBase<sync_preferences::PrefServiceSyncable, pref_notifier), user_prefs, pref_registry, - nullptr, // pref_model_associator_client + /*pref_model_associator_client=*/nullptr, base::Bind(&TestingPrefServiceBase< PrefServiceSyncable, user_prefs::PrefRegistrySyncable>::HandleReadError), diff --git a/chromium/components/sync_preferences/testing_pref_service_syncable.h b/chromium/components/sync_preferences/testing_pref_service_syncable.h index cf83573fcdf..ee59cc1dfad 100644 --- a/chromium/components/sync_preferences/testing_pref_service_syncable.h +++ b/chromium/components/sync_preferences/testing_pref_service_syncable.h @@ -16,6 +16,18 @@ class PrefRegistrySyncable; namespace sync_preferences { // Test version of PrefServiceSyncable. +// This class hierarchy has a flaw: TestingPrefServiceBase is inheriting from +// the first template parameter (PrefServiceSyncable in this case). This means, +// all of the supported parameter types must support the same constructor +// signatures -- which they don't. Hence, it's not possible to properly inject +// a PrefModelAssociatorClient. +// TODO(tschumann) The whole purpose of TestingPrefServiceBase is questionable +// and I'd be in favor of removing it completely: +// -- it hides the dependency injetion of the different stores +// -- just to later offer ways to manipulate speficic stores. +// -- if tests just dependency injects the individual stores directly, they +// already have full control and won't need that indirection at all. +// See PrefServiceSyncableMergeTest as an example of a cleaner way. class TestingPrefServiceSyncable : public TestingPrefServiceBase<PrefServiceSyncable, user_prefs::PrefRegistrySyncable> { @@ -34,8 +46,6 @@ class TestingPrefServiceSyncable // a PrefRegistry via its constructor (or via e.g. PrefServiceFactory). user_prefs::PrefRegistrySyncable* registry(); - using PrefServiceSyncable::SetPrefModelAssociatorClientForTesting; - private: DISALLOW_COPY_AND_ASSIGN(TestingPrefServiceSyncable); }; |