From 0373fe1947fb05fbe421407dc7f393e430db6019 Mon Sep 17 00:00:00 2001 From: aelias Date: Tue, 21 Feb 2017 15:50:54 -0800 Subject: [PATCH] Revert of Add PrefStore::GetValues (patchset #8 id:140001 of https://codereview.chromium.org/2692203007/ ) Reason for revert: Breaks compile on Cronet proxy. BUG=694814 Original issue's description: > Add PrefStore::GetValues > > Currently there's no way to proxy a PrefStore, which we need to > in the new prefs service, because there's no way to get its > initial state. The existing observer interface can only be used > to get changed values and GetValue requires that you know all the > keys in advance. > > GetValues involve a deep copy, but the intended consumer needs to > take ownership to send the value over the wire anyway. > > Design doc: https://docs.google.com/document/d/1Fj013SXClTzk4Yfq2eoL9OkKfN0h-GLXPAokCXFkcTY/edit?usp=sharing > > BUG=654988 > > Review-Url: https://codereview.chromium.org/2692203007 > Cr-Commit-Position: refs/heads/master@{#451691} > Committed: https://chromium.googlesource.com/chromium/src/+/311d4a192239299e42e66b1319b19ee58a5a4b06 TBR=sammc@chromium.org,emaxx@chromium.org,bauerb@chromium.org,tibell@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=654988 Review-Url: https://codereview.chromium.org/2704133006 Cr-Commit-Position: refs/heads/master@{#451856} --- .../supervised_user_pref_store.cc | 5 --- .../supervised_user_pref_store.h | 1 - .../configuration_policy_pref_store.cc | 8 ---- .../browser/configuration_policy_pref_store.h | 1 - components/prefs/default_pref_store.cc | 4 -- components/prefs/default_pref_store.h | 2 - components/prefs/in_memory_pref_store.cc | 4 -- components/prefs/in_memory_pref_store.h | 1 - components/prefs/json_pref_store.cc | 4 -- components/prefs/json_pref_store.h | 1 - components/prefs/overlay_user_pref_store.cc | 18 -------- components/prefs/overlay_user_pref_store.h | 1 - .../prefs/overlay_user_pref_store_unittest.cc | 41 ------------------- components/prefs/pref_store.h | 5 --- components/prefs/pref_value_map.cc | 8 ---- components/prefs/pref_value_map.h | 4 -- components/prefs/testing_pref_store.cc | 4 -- components/prefs/testing_pref_store.h | 1 - components/prefs/value_map_pref_store.cc | 4 -- components/prefs/value_map_pref_store.h | 1 - .../tracked/segregated_pref_store.cc | 14 ------- .../tracked/segregated_pref_store.h | 1 - .../tracked/segregated_pref_store_unittest.cc | 30 -------------- 23 files changed, 163 deletions(-) diff --git a/chrome/browser/supervised_user/supervised_user_pref_store.cc b/chrome/browser/supervised_user/supervised_user_pref_store.cc index 565ee8c05ed6c4..546db70b9ce8d8 100644 --- a/chrome/browser/supervised_user/supervised_user_pref_store.cc +++ b/chrome/browser/supervised_user/supervised_user_pref_store.cc @@ -87,11 +87,6 @@ bool SupervisedUserPrefStore::GetValue(const std::string& key, return prefs_->GetValue(key, value); } -std::unique_ptr SupervisedUserPrefStore::GetValues() - const { - return prefs_->AsDictionaryValue(); -} - void SupervisedUserPrefStore::AddObserver(PrefStore::Observer* observer) { observers_.AddObserver(observer); } diff --git a/chrome/browser/supervised_user/supervised_user_pref_store.h b/chrome/browser/supervised_user/supervised_user_pref_store.h index 556093444b1038..592d61ec0a3d69 100644 --- a/chrome/browser/supervised_user/supervised_user_pref_store.h +++ b/chrome/browser/supervised_user/supervised_user_pref_store.h @@ -35,7 +35,6 @@ class SupervisedUserPrefStore : public PrefStore, // PrefStore overrides: bool GetValue(const std::string& key, const base::Value** value) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/policy/core/browser/configuration_policy_pref_store.cc b/components/policy/core/browser/configuration_policy_pref_store.cc index 7d1a70616644c8..17ca01b44385bd 100644 --- a/components/policy/core/browser/configuration_policy_pref_store.cc +++ b/components/policy/core/browser/configuration_policy_pref_store.cc @@ -10,7 +10,6 @@ #include "base/bind.h" #include "base/location.h" #include "base/logging.h" -#include "base/memory/ptr_util.h" #include "base/single_thread_task_runner.h" #include "base/strings/string16.h" #include "base/strings/utf_string_conversions.h" @@ -80,13 +79,6 @@ bool ConfigurationPolicyPrefStore::GetValue(const std::string& key, return true; } -std::unique_ptr ConfigurationPolicyPrefStore::GetValues() - const { - if (!prefs_) - return base::MakeUnique(); - return prefs_->AsDictionaryValue(); -} - void ConfigurationPolicyPrefStore::OnPolicyUpdated( const PolicyNamespace& ns, const PolicyMap& previous, diff --git a/components/policy/core/browser/configuration_policy_pref_store.h b/components/policy/core/browser/configuration_policy_pref_store.h index 87e1992aa695f7..3d1ef9a2579d46 100644 --- a/components/policy/core/browser/configuration_policy_pref_store.h +++ b/components/policy/core/browser/configuration_policy_pref_store.h @@ -44,7 +44,6 @@ class POLICY_EXPORT ConfigurationPolicyPrefStore bool IsInitializationComplete() const override; bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; // PolicyService::Observer methods: void OnPolicyUpdated(const PolicyNamespace& ns, diff --git a/components/prefs/default_pref_store.cc b/components/prefs/default_pref_store.cc index 0e9c87c08385c8..468c11c74835a5 100644 --- a/components/prefs/default_pref_store.cc +++ b/components/prefs/default_pref_store.cc @@ -17,10 +17,6 @@ bool DefaultPrefStore::GetValue(const std::string& key, return prefs_.GetValue(key, result); } -std::unique_ptr DefaultPrefStore::GetValues() const { - return prefs_.AsDictionaryValue(); -} - void DefaultPrefStore::AddObserver(PrefStore::Observer* observer) { observers_.AddObserver(observer); } diff --git a/components/prefs/default_pref_store.h b/components/prefs/default_pref_store.h index c06cd6c10de37c..3481713f8935af 100644 --- a/components/prefs/default_pref_store.h +++ b/components/prefs/default_pref_store.h @@ -5,7 +5,6 @@ #ifndef COMPONENTS_PREFS_DEFAULT_PREF_STORE_H_ #define COMPONENTS_PREFS_DEFAULT_PREF_STORE_H_ -#include #include #include "base/macros.h" @@ -25,7 +24,6 @@ class COMPONENTS_PREFS_EXPORT DefaultPrefStore : public PrefStore { // PrefStore implementation: bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/prefs/in_memory_pref_store.cc b/components/prefs/in_memory_pref_store.cc index a0a9c35d47b5ee..d0d35582523908 100644 --- a/components/prefs/in_memory_pref_store.cc +++ b/components/prefs/in_memory_pref_store.cc @@ -18,10 +18,6 @@ bool InMemoryPrefStore::GetValue(const std::string& key, return prefs_.GetValue(key, value); } -std::unique_ptr InMemoryPrefStore::GetValues() const { - return prefs_.AsDictionaryValue(); -} - bool InMemoryPrefStore::GetMutableValue(const std::string& key, base::Value** value) { return prefs_.GetValue(key, value); diff --git a/components/prefs/in_memory_pref_store.h b/components/prefs/in_memory_pref_store.h index 5529622fdd0afa..042137c3ce15a6 100644 --- a/components/prefs/in_memory_pref_store.h +++ b/components/prefs/in_memory_pref_store.h @@ -26,7 +26,6 @@ class COMPONENTS_PREFS_EXPORT InMemoryPrefStore : public PersistentPrefStore { // PrefStore implementation. bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/prefs/json_pref_store.cc b/components/prefs/json_pref_store.cc index a9a39227d3fef9..026606a7b762fb 100644 --- a/components/prefs/json_pref_store.cc +++ b/components/prefs/json_pref_store.cc @@ -190,10 +190,6 @@ bool JsonPrefStore::GetValue(const std::string& key, return true; } -std::unique_ptr JsonPrefStore::GetValues() const { - return prefs_->CreateDeepCopy(); -} - void JsonPrefStore::AddObserver(PrefStore::Observer* observer) { DCHECK(CalledOnValidThread()); diff --git a/components/prefs/json_pref_store.h b/components/prefs/json_pref_store.h index 8c1164ca8ba390..170ccb1161d405 100644 --- a/components/prefs/json_pref_store.h +++ b/components/prefs/json_pref_store.h @@ -84,7 +84,6 @@ class COMPONENTS_PREFS_EXPORT JsonPrefStore // PrefStore overrides: bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/prefs/overlay_user_pref_store.cc b/components/prefs/overlay_user_pref_store.cc index ddc47c8f351d58..2b2208ed821440 100644 --- a/components/prefs/overlay_user_pref_store.cc +++ b/components/prefs/overlay_user_pref_store.cc @@ -47,24 +47,6 @@ bool OverlayUserPrefStore::GetValue(const std::string& key, return underlay_->GetValue(GetUnderlayKey(key), result); } -std::unique_ptr OverlayUserPrefStore::GetValues() const { - auto values = underlay_->GetValues(); - auto overlay_values = overlay_.AsDictionaryValue(); - for (const auto& overlay_mapping : overlay_to_underlay_names_map_) { - const std::string& overlay_key = overlay_mapping.first; - const std::string& underlay_key = overlay_mapping.second; - std::unique_ptr out_value; - if (overlay_key != underlay_key) { - values->Remove(underlay_key, &out_value); - } - overlay_values->Remove(overlay_key, &out_value); - if (out_value) { - values->Set(overlay_key, std::move(out_value)); - } - } - return values; -} - bool OverlayUserPrefStore::GetMutableValue(const std::string& key, base::Value** result) { if (!ShallBeStoredInOverlay(key)) diff --git a/components/prefs/overlay_user_pref_store.h b/components/prefs/overlay_user_pref_store.h index 7d9eb3071a5d94..e3ad88e1668886 100644 --- a/components/prefs/overlay_user_pref_store.h +++ b/components/prefs/overlay_user_pref_store.h @@ -38,7 +38,6 @@ class COMPONENTS_PREFS_EXPORT OverlayUserPrefStore : public PersistentPrefStore, bool IsInitializationComplete() const override; bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; // Methods of PersistentPrefStore. bool GetMutableValue(const std::string& key, base::Value** result) override; diff --git a/components/prefs/overlay_user_pref_store_unittest.cc b/components/prefs/overlay_user_pref_store_unittest.cc index 20cdb1730fb3fd..d722aeab6e0100 100644 --- a/components/prefs/overlay_user_pref_store_unittest.cc +++ b/components/prefs/overlay_user_pref_store_unittest.cc @@ -19,11 +19,9 @@ namespace { const char kBrowserWindowPlacement[] = "browser.window_placement"; const char kShowBookmarkBar[] = "bookmark_bar.show_on_all_tabs"; -const char kSharedKey[] = "sync_promo.show_on_first_run_allowed"; const char* const overlay_key = kBrowserWindowPlacement; const char* const regular_key = kShowBookmarkBar; -const char* const shared_key = kSharedKey; // With the removal of the kWebKitGlobalXXX prefs, we'll no longer have real // prefs using the overlay pref store, so make up keys here. const char mapped_overlay_key[] = "test.per_tab.javascript_enabled"; @@ -37,7 +35,6 @@ class OverlayUserPrefStoreTest : public testing::Test { : underlay_(new TestingPrefStore()), overlay_(new OverlayUserPrefStore(underlay_.get())) { overlay_->RegisterOverlayPref(overlay_key); - overlay_->RegisterOverlayPref(shared_key); overlay_->RegisterOverlayPref(mapped_overlay_key, mapped_underlay_key); } @@ -312,42 +309,4 @@ TEST_F(OverlayUserPrefStoreTest, ClearMutableValues) { EXPECT_TRUE(base::FundamentalValue(42).Equals(value)); } -TEST_F(OverlayUserPrefStoreTest, GetValues) { - // To check merge behavior, create underlay and overlay so each has a key the - // other doesn't have and they have one key in common. - underlay_->SetValue(regular_key, base::WrapUnique(new FundamentalValue(42)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - overlay_->SetValue(overlay_key, base::WrapUnique(new FundamentalValue(43)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - underlay_->SetValue(shared_key, base::WrapUnique(new FundamentalValue(42)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - overlay_->SetValue(shared_key, base::WrapUnique(new FundamentalValue(43)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - underlay_->SetValue(mapped_underlay_key, - base::WrapUnique(new FundamentalValue(42)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - overlay_->SetValue(mapped_overlay_key, - base::WrapUnique(new FundamentalValue(43)), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - - auto values = overlay_->GetValues(); - const Value* value = nullptr; - // Check that an overlay preference is returned. - ASSERT_TRUE(values->Get(overlay_key, &value)); - EXPECT_TRUE(base::FundamentalValue(43).Equals(value)); - - // Check that an underlay preference is returned. - ASSERT_TRUE(values->Get(regular_key, &value)); - EXPECT_TRUE(base::FundamentalValue(42).Equals(value)); - - // Check that the overlay is preferred. - ASSERT_TRUE(values->Get(shared_key, &value)); - EXPECT_TRUE(base::FundamentalValue(43).Equals(value)); - - // Check that mapping works. - ASSERT_TRUE(values->Get(mapped_overlay_key, &value)); - EXPECT_TRUE(base::FundamentalValue(43).Equals(value)); - EXPECT_FALSE(values->Get(mapped_underlay_key, &value)); -} - } // namespace base diff --git a/components/prefs/pref_store.h b/components/prefs/pref_store.h index 7964f1f0093936..ef2bc494d150b8 100644 --- a/components/prefs/pref_store.h +++ b/components/prefs/pref_store.h @@ -5,7 +5,6 @@ #ifndef COMPONENTS_PREFS_PREF_STORE_H_ #define COMPONENTS_PREFS_PREF_STORE_H_ -#include #include #include "base/macros.h" @@ -13,7 +12,6 @@ #include "components/prefs/base_prefs_export.h" namespace base { -class DictionaryValue; class Value; } @@ -54,9 +52,6 @@ class COMPONENTS_PREFS_EXPORT PrefStore : public base::RefCounted { virtual bool GetValue(const std::string& key, const base::Value** result) const = 0; - // Get all the values. Never returns a null pointer. - virtual std::unique_ptr GetValues() const = 0; - protected: friend class base::RefCounted; virtual ~PrefStore() {} diff --git a/components/prefs/pref_value_map.cc b/components/prefs/pref_value_map.cc index ecacf4c73d4b55..f0392d25284319 100644 --- a/components/prefs/pref_value_map.cc +++ b/components/prefs/pref_value_map.cc @@ -157,11 +157,3 @@ void PrefValueMap::GetDifferingKeys( for ( ; other_pref != other_prefs.end(); ++other_pref) differing_keys->push_back(other_pref->first); } - -std::unique_ptr PrefValueMap::AsDictionaryValue() const { - auto dictionary = base::MakeUnique(); - for (const auto& value : prefs_) { - dictionary->Set(value.first, value.second->CreateDeepCopy()); - } - return dictionary; -} diff --git a/components/prefs/pref_value_map.h b/components/prefs/pref_value_map.h index bad10cf978487b..b0ffbd15fdda3e 100644 --- a/components/prefs/pref_value_map.h +++ b/components/prefs/pref_value_map.h @@ -14,7 +14,6 @@ #include "components/prefs/base_prefs_export.h" namespace base { -class DictionaryValue; class Value; } @@ -84,9 +83,6 @@ class COMPONENTS_PREFS_EXPORT PrefValueMap { void GetDifferingKeys(const PrefValueMap* other, std::vector* differing_keys) const; - // Copies the map into a dictionary value. - std::unique_ptr AsDictionaryValue() const; - private: Map prefs_; diff --git a/components/prefs/testing_pref_store.cc b/components/prefs/testing_pref_store.cc index d084c4a23fd4bd..1e775824a6a9ba 100644 --- a/components/prefs/testing_pref_store.cc +++ b/components/prefs/testing_pref_store.cc @@ -24,10 +24,6 @@ bool TestingPrefStore::GetValue(const std::string& key, return prefs_.GetValue(key, value); } -std::unique_ptr TestingPrefStore::GetValues() const { - return prefs_.AsDictionaryValue(); -} - bool TestingPrefStore::GetMutableValue(const std::string& key, base::Value** value) { return prefs_.GetValue(key, value); diff --git a/components/prefs/testing_pref_store.h b/components/prefs/testing_pref_store.h index 2e685e4fde6ae1..2bf2c8f72b113e 100644 --- a/components/prefs/testing_pref_store.h +++ b/components/prefs/testing_pref_store.h @@ -25,7 +25,6 @@ class TestingPrefStore : public PersistentPrefStore { // Overriden from PrefStore. bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/prefs/value_map_pref_store.cc b/components/prefs/value_map_pref_store.cc index c31d7c771cc935..8b828b10d5c628 100644 --- a/components/prefs/value_map_pref_store.cc +++ b/components/prefs/value_map_pref_store.cc @@ -17,10 +17,6 @@ bool ValueMapPrefStore::GetValue(const std::string& key, return prefs_.GetValue(key, value); } -std::unique_ptr ValueMapPrefStore::GetValues() const { - return prefs_.AsDictionaryValue(); -} - void ValueMapPrefStore::AddObserver(PrefStore::Observer* observer) { observers_.AddObserver(observer); } diff --git a/components/prefs/value_map_pref_store.h b/components/prefs/value_map_pref_store.h index 34d10959572313..8310a9bad14bc6 100644 --- a/components/prefs/value_map_pref_store.h +++ b/components/prefs/value_map_pref_store.h @@ -25,7 +25,6 @@ class COMPONENTS_PREFS_EXPORT ValueMapPrefStore : public WriteablePrefStore { // PrefStore overrides: bool GetValue(const std::string& key, const base::Value** value) const override; - std::unique_ptr GetValues() const override; void AddObserver(PrefStore::Observer* observer) override; void RemoveObserver(PrefStore::Observer* observer) override; bool HasObservers() const override; diff --git a/components/user_prefs/tracked/segregated_pref_store.cc b/components/user_prefs/tracked/segregated_pref_store.cc index 297e2a7e58fa6c..edb1f2059920bb 100644 --- a/components/user_prefs/tracked/segregated_pref_store.cc +++ b/components/user_prefs/tracked/segregated_pref_store.cc @@ -83,20 +83,6 @@ bool SegregatedPrefStore::GetValue(const std::string& key, return StoreForKey(key)->GetValue(key, result); } -std::unique_ptr SegregatedPrefStore::GetValues() const { - auto values = default_pref_store_->GetValues(); - auto selected_pref_store_values = selected_pref_store_->GetValues(); - for (const auto& key : selected_preference_names_) { - const base::Value* value = nullptr; - if (selected_pref_store_values->Get(key, &value)) { - values->Set(key, value->CreateDeepCopy()); - } else { - values->Remove(key, nullptr); - } - } - return values; -} - void SegregatedPrefStore::SetValue(const std::string& key, std::unique_ptr value, uint32_t flags) { diff --git a/components/user_prefs/tracked/segregated_pref_store.h b/components/user_prefs/tracked/segregated_pref_store.h index 40629768f018e3..016cca6d0490de 100644 --- a/components/user_prefs/tracked/segregated_pref_store.h +++ b/components/user_prefs/tracked/segregated_pref_store.h @@ -52,7 +52,6 @@ class SegregatedPrefStore : public PersistentPrefStore { bool IsInitializationComplete() const override; bool GetValue(const std::string& key, const base::Value** result) const override; - std::unique_ptr GetValues() const override; // WriteablePrefStore implementation void SetValue(const std::string& key, diff --git a/components/user_prefs/tracked/segregated_pref_store_unittest.cc b/components/user_prefs/tracked/segregated_pref_store_unittest.cc index 4c685cc319abf3..5d32d027d5e480 100644 --- a/components/user_prefs/tracked/segregated_pref_store_unittest.cc +++ b/components/user_prefs/tracked/segregated_pref_store_unittest.cc @@ -24,7 +24,6 @@ namespace { const char kSelectedPref[] = "selected_pref"; const char kUnselectedPref[] = "unselected_pref"; -const char kSharedPref[] = "shared_pref"; const char kValue1[] = "value1"; const char kValue2[] = "value2"; @@ -71,7 +70,6 @@ class SegregatedPrefStoreTest : public testing::Test { std::set selected_pref_names; selected_pref_names.insert(kSelectedPref); - selected_pref_names.insert(kSharedPref); segregated_store_ = new SegregatedPrefStore(default_store_, selected_store_, selected_pref_names); @@ -276,31 +274,3 @@ TEST_F(SegregatedPrefStoreTest, IsInitializationCompleteAsync) { default_store_->SetBlockAsyncRead(false); EXPECT_TRUE(segregated_store_->IsInitializationComplete()); } - -TEST_F(SegregatedPrefStoreTest, GetValues) { - // To check merge behavior, create selected and default stores so each has a - // key the other doesn't have and they have one key in common. - selected_store_->SetValue(kSelectedPref, - base::MakeUnique(kValue1), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - default_store_->SetValue(kUnselectedPref, - base::MakeUnique(kValue2), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - selected_store_->SetValue(kSharedPref, - base::MakeUnique(kValue1), - WriteablePrefStore::DEFAULT_PREF_WRITE_FLAGS); - - auto values = segregated_store_->GetValues(); - const base::Value* value = nullptr; - // Check that a selected preference is returned. - ASSERT_TRUE(values->Get(kSelectedPref, &value)); - EXPECT_TRUE(base::FundamentalValue(kValue1).Equals(value)); - - // Check that a a default preference is returned. - ASSERT_TRUE(values->Get(kUnselectedPref, &value)); - EXPECT_TRUE(base::FundamentalValue(kValue2).Equals(value)); - - // Check that the selected preference is preferred. - ASSERT_TRUE(values->Get(kSharedPref, &value)); - EXPECT_TRUE(base::FundamentalValue(kValue1).Equals(value)); -}