Remove KeyedServiceBaseFactory::RegisterUserPrefsOnContextForTest
Remove obsolete method RegisterUserPrefsOnContextForTest from
KeyedServiceBaseFactory (tests should instead register the prefs
before creating instances of the services by using testing context
like TestingProfile or TestChromeBrowserState).
Also remove all supporting code present to ensure that the prefs
are not registered twice due to RegisterUserPrefsOnContextForTest
interaction with registration via DependencyManager.
Bug: 932534
Change-Id: Id49b802d9cc6818cb101b8b9697b274a9e5d2e3b
Reviewed-on: https://chromium-review.googlesource.com/c/1474938
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#633091}diff --git a/components/keyed_service/DEPS b/components/keyed_service/DEPS
index 4153fdb..b1e57475 100644
--- a/components/keyed_service/DEPS
+++ b/components/keyed_service/DEPS
@@ -5,5 +5,4 @@
"+components/pref_registry",
"+components/prefs",
- "+components/user_prefs",
]
diff --git a/components/keyed_service/content/BUILD.gn b/components/keyed_service/content/BUILD.gn
index deadc76..763dd15 100644
--- a/components/keyed_service/content/BUILD.gn
+++ b/components/keyed_service/content/BUILD.gn
@@ -32,7 +32,6 @@
deps = [
"//base/third_party/dynamic_annotations",
"//components/pref_registry",
- "//components/user_prefs",
"//content/public/browser",
"//content/public/common",
]
diff --git a/components/keyed_service/content/browser_context_keyed_base_factory.h b/components/keyed_service/content/browser_context_keyed_base_factory.h
index 33ea144c..940fad4 100644
--- a/components/keyed_service/content/browser_context_keyed_base_factory.h
+++ b/components/keyed_service/content/browser_context_keyed_base_factory.h
@@ -77,9 +77,8 @@
virtual void BrowserContextDestroyed(content::BrowserContext* context);
private:
- // Registers any user preferences on this service. This is called by
- // RegisterPrefsIfNecessaryForContext() and should be overriden by any service
- // that wants to register profile-specific preferences.
+ // Registers any user preferences on this service. This should be overriden by
+ // any service that wants to register profile-specific preferences.
virtual void RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {}
diff --git a/components/keyed_service/content/browser_context_keyed_service_factory.cc b/components/keyed_service/content/browser_context_keyed_service_factory.cc
index 3cbb7d2..21e9611 100644
--- a/components/keyed_service/content/browser_context_keyed_service_factory.cc
+++ b/components/keyed_service/content/browser_context_keyed_service_factory.cc
@@ -10,7 +10,6 @@
#include "components/keyed_service/content/browser_context_dependency_manager.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/pref_registry/pref_registry_syncable.h"
-#include "components/user_prefs/user_prefs.h"
#include "content/public/browser/browser_context.h"
void BrowserContextKeyedServiceFactory::SetTestingFactory(
diff --git a/components/keyed_service/content/browser_context_keyed_service_factory.h b/components/keyed_service/content/browser_context_keyed_service_factory.h
index e52c7d0..ab605769 100644
--- a/components/keyed_service/content/browser_context_keyed_service_factory.h
+++ b/components/keyed_service/content/browser_context_keyed_service_factory.h
@@ -115,9 +115,8 @@
private:
friend class BrowserContextDependencyManagerUnittests;
- // Registers any user preferences on this service. This is called by
- // RegisterPrefsIfNecessaryForContext() and should be overriden by any service
- // that wants to register profile-specific preferences.
+ // Registers any user preferences on this service. This should be overriden by
+ // any service that wants to register profile-specific preferences.
virtual void RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {}
diff --git a/components/keyed_service/content/refcounted_browser_context_keyed_service_factory.h b/components/keyed_service/content/refcounted_browser_context_keyed_service_factory.h
index 1580eda..3d2deb80 100644
--- a/components/keyed_service/content/refcounted_browser_context_keyed_service_factory.h
+++ b/components/keyed_service/content/refcounted_browser_context_keyed_service_factory.h
@@ -120,9 +120,8 @@
private:
friend class BrowserContextDependencyManagerUnittests;
- // Registers any user preferences on this service. This is called by
- // RegisterPrefsIfNecessaryForContext() and should be overriden by any service
- // that wants to register profile-specific preferences.
+ // Registers any user preferences on this service. This should be overriden by
+ // any service that wants to register profile-specific preferences.
virtual void RegisterProfilePrefs(
user_prefs::PrefRegistrySyncable* registry) {}
diff --git a/components/keyed_service/core/BUILD.gn b/components/keyed_service/core/BUILD.gn
index 863ce7f8..9a52d82 100644
--- a/components/keyed_service/core/BUILD.gn
+++ b/components/keyed_service/core/BUILD.gn
@@ -34,7 +34,6 @@
"//base",
"//components/pref_registry",
"//components/prefs",
- "//components/user_prefs",
]
}
diff --git a/components/keyed_service/core/dependency_manager.cc b/components/keyed_service/core/dependency_manager.cc
index 093f40a8..a55ca5c 100644
--- a/components/keyed_service/core/dependency_manager.cc
+++ b/components/keyed_service/core/dependency_manager.cc
@@ -45,7 +45,7 @@
for (auto* dependency_node : construction_order) {
KeyedServiceBaseFactory* factory =
static_cast<KeyedServiceBaseFactory*>(dependency_node);
- factory->RegisterPrefsIfNecessaryForContext(context, pref_registry);
+ factory->RegisterPrefs(pref_registry);
}
}
diff --git a/components/keyed_service/core/keyed_service_base_factory.cc b/components/keyed_service/core/keyed_service_base_factory.cc
index 19b7513..b9cf09c 100644
--- a/components/keyed_service/core/keyed_service_base_factory.cc
+++ b/components/keyed_service/core/keyed_service_base_factory.cc
@@ -9,39 +9,6 @@
#include "components/keyed_service/core/dependency_manager.h"
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_service.h"
-#include "components/user_prefs/user_prefs.h"
-
-void KeyedServiceBaseFactory::RegisterUserPrefsOnContextForTest(
- base::SupportsUserData* context) {
- TRACE_EVENT0("browser,startup",
- "KeyedServiceBaseFactory::RegisterUserPrefsOnContextForTest");
- // Safe timing for pref registration is hard. Previously, we made
- // context responsible for all pref registration on every service
- // that used contexts. Now we don't and there are timing issues.
- //
- // With normal contexts, prefs can simply be registered at
- // DependencyManager::RegisterProfilePrefsForServices time.
- // With incognito contexts, we just never register since incognito
- // contexts share the same pref services with their parent contexts.
- //
- // Testing contexts throw a wrench into the mix, in that some tests will
- // swap out the PrefService after we've registered user prefs on the original
- // PrefService. Test code that does this is responsible for either manually
- // invoking RegisterProfilePrefs() on the appropriate
- // ContextKeyedServiceFactory associated with the prefs they need,
- // or they can use SetTestingFactory() and create a service (since service
- // creation with a factory method causes registration to happen at
- // TestingProfile creation time).
- //
- // Now that services are responsible for declaring their preferences, we have
- // to enforce a uniquenes check here because some tests create one context and
- // multiple services of the same type attached to that context (serially, not
- // parallel) and we don't want to register multiple times on the same context.
- // This is the purpose of RegisterPrefsIfNecessaryForContext() which could be
- // replaced directly by RegisterPrefs() if this method is ever phased out.
- RegisterPrefsIfNecessaryForContext(context,
- GetAssociatedPrefRegistry(context));
-}
KeyedServiceBaseFactory::KeyedServiceBaseFactory(const char* service_name,
DependencyManager* manager)
@@ -58,25 +25,6 @@
dependency_manager_->AddEdge(rhs, this);
}
-void KeyedServiceBaseFactory::RegisterPrefsIfNecessaryForContext(
- base::SupportsUserData* context,
- user_prefs::PrefRegistrySyncable* registry) {
- if (!ArePreferencesSetOn(context)) {
- RegisterPrefs(registry);
- MarkPreferencesSetOn(context);
- }
-}
-
-user_prefs::PrefRegistrySyncable*
-KeyedServiceBaseFactory::GetAssociatedPrefRegistry(
- base::SupportsUserData* context) const {
- PrefService* prefs = user_prefs::UserPrefs::Get(context);
- user_prefs::PrefRegistrySyncable* registry =
- static_cast<user_prefs::PrefRegistrySyncable*>(
- prefs->DeprecatedGetPrefRegistry());
- return registry;
-}
-
void KeyedServiceBaseFactory::AssertContextWasntDestroyed(
base::SupportsUserData* context) const {
// TODO(crbug.com/701326): We should DCHECK(CalledOnValidThread()) here, but
@@ -103,16 +51,4 @@
// While object destruction can be customized in ways where the object is
// only dereferenced, this still must run on the UI thread.
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- registered_preferences_.erase(context);
-}
-
-bool KeyedServiceBaseFactory::ArePreferencesSetOn(
- base::SupportsUserData* context) const {
- return registered_preferences_.find(context) != registered_preferences_.end();
-}
-
-void KeyedServiceBaseFactory::MarkPreferencesSetOn(
- base::SupportsUserData* context) {
- DCHECK(!ArePreferencesSetOn(context));
- registered_preferences_.insert(context);
}
diff --git a/components/keyed_service/core/keyed_service_base_factory.h b/components/keyed_service/core/keyed_service_base_factory.h
index 48c3ca2..612395e2b 100644
--- a/components/keyed_service/core/keyed_service_base_factory.h
+++ b/components/keyed_service/core/keyed_service_base_factory.h
@@ -12,7 +12,6 @@
#include "components/keyed_service/core/keyed_service_export.h"
class DependencyManager;
-class PrefService;
namespace base {
class SupportsUserData;
@@ -37,15 +36,6 @@
KeyedServiceBaseFactory(const char* service_name, DependencyManager* manager);
virtual ~KeyedServiceBaseFactory();
- // Registers preferences used in this service on the pref service associated
- // with |context|. This is safe to be called multiple times because testing
- // code can have multiple services of the same type attached to a single
- // |context|. Only test code is allowed to call this method.
- //
- // TODO(gab): This method can be removed entirely when
- // PrefService::DeprecatedGetPrefRegistry() is phased out.
- void RegisterUserPrefsOnContextForTest(base::SupportsUserData* context);
-
// The main public interface for declaring dependencies between services
// created by factories.
void DependsOn(KeyedServiceBaseFactory* rhs);
@@ -61,19 +51,6 @@
// 0xWhatever).
void MarkContextLive(base::SupportsUserData* context);
- // Calls RegisterProfilePrefs() after doing house keeping required to work
- // alongside RegisterUserPrefsOnContextForTest().
- // TODO(gab): This method can be replaced by RegisterProfilePrefs() directly
- // once RegisterUserPrefsOnContextForTest() is phased out.
- void RegisterPrefsIfNecessaryForContext(
- base::SupportsUserData* context,
- user_prefs::PrefRegistrySyncable* registry);
-
- // Returns the |user_pref::PrefRegistrySyncable| associated with |context|.
- // The way they are associated is controlled by the embedder.
- user_prefs::PrefRegistrySyncable* GetAssociatedPrefRegistry(
- base::SupportsUserData* context) const;
-
// Finds which context (if any) to use.
virtual base::SupportsUserData* GetContextToUse(
base::SupportsUserData* context) const = 0;
@@ -102,12 +79,6 @@
virtual void ContextShutdown(base::SupportsUserData* context) = 0;
virtual void ContextDestroyed(base::SupportsUserData* context);
- // Returns whether the preferences have been registered on this context.
- bool ArePreferencesSetOn(base::SupportsUserData* context) const;
-
- // Mark context has having preferences registered.
- void MarkPreferencesSetOn(base::SupportsUserData* context);
-
SEQUENCE_CHECKER(sequence_checker_);
private:
@@ -117,7 +88,8 @@
// by all the factories of a given type. Unit tests will use their own copy.
DependencyManager* dependency_manager_;
- // Registers any preferences used by this service.
+ // Registers any preferences used by this service. This should be overriden by
+ // any services that want to register context-specific preferences.
virtual void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) {}
// Used by DependencyManager to disable creation of the service when the
@@ -130,9 +102,6 @@
// Create the service associated with |context|.
virtual void CreateServiceNow(base::SupportsUserData* context) = 0;
- // Contexts that have this service's preferences registered on them.
- std::set<base::SupportsUserData*> registered_preferences_;
-
// A static string passed in to the constructor. Should be unique across all
// services.
const char* service_name_;
diff --git a/components/keyed_service/core/keyed_service_factory.cc b/components/keyed_service/core/keyed_service_factory.cc
index 5fbfc03..f3629a59 100644
--- a/components/keyed_service/core/keyed_service_factory.cc
+++ b/components/keyed_service/core/keyed_service_factory.cc
@@ -23,12 +23,6 @@
void KeyedServiceFactory::SetTestingFactory(base::SupportsUserData* context,
TestingFactory testing_factory) {
- // Destroying the context may cause us to lose data about whether |context|
- // has our preferences registered on it (since the context object itself
- // isn't dead). See if we need to readd it once we've gone through normal
- // destruction.
- bool add_context = ArePreferencesSetOn(context);
-
// Ensure that |context| is not marked as stale (e.g., due to it aliasing an
// instance that was destroyed in an earlier test) in order to avoid accesses
// to |context| in |BrowserContextShutdown| from causing
@@ -41,9 +35,6 @@
ContextShutdown(context);
ContextDestroyed(context);
- if (add_context)
- MarkPreferencesSetOn(context);
-
testing_factories_.emplace(context, std::move(testing_factory));
}
@@ -81,8 +72,6 @@
auto factory_iterator = testing_factories_.find(context);
if (factory_iterator != testing_factories_.end()) {
if (factory_iterator->second) {
- if (!IsOffTheRecord(context))
- RegisterUserPrefsOnContextForTest(context);
service = factory_iterator->second.Run(context);
}
} else {
diff --git a/components/keyed_service/core/refcounted_keyed_service_factory.cc b/components/keyed_service/core/refcounted_keyed_service_factory.cc
index f7c4f9c6..2e7355d3 100644
--- a/components/keyed_service/core/refcounted_keyed_service_factory.cc
+++ b/components/keyed_service/core/refcounted_keyed_service_factory.cc
@@ -22,12 +22,6 @@
void RefcountedKeyedServiceFactory::SetTestingFactory(
base::SupportsUserData* context,
TestingFactory testing_factory) {
- // Destroying the context may cause us to lose data about whether |context|
- // has our preferences registered on it (since the context object itself
- // isn't dead). See if we need to readd it once we've gone through normal
- // destruction.
- bool add_context = ArePreferencesSetOn(context);
-
// Ensure that |context| is not marked as stale (e.g., due to it aliasing an
// instance that was destroyed in an earlier test) in order to avoid accesses
// to |context| in |ContextShutdown| from causing
@@ -40,9 +34,6 @@
ContextShutdown(context);
ContextDestroyed(context);
- if (add_context)
- MarkPreferencesSetOn(context);
-
testing_factories_.emplace(context, std::move(testing_factory));
}
@@ -80,8 +71,6 @@
auto factory_iterator = testing_factories_.find(context);
if (factory_iterator != testing_factories_.end()) {
if (factory_iterator->second) {
- if (!IsOffTheRecord(context))
- RegisterUserPrefsOnContextForTest(context);
service = factory_iterator->second.Run(context);
}
} else {
diff --git a/components/keyed_service/ios/browser_state_keyed_service_factory.h b/components/keyed_service/ios/browser_state_keyed_service_factory.h
index a8539ac..d891c113 100644
--- a/components/keyed_service/ios/browser_state_keyed_service_factory.h
+++ b/components/keyed_service/ios/browser_state_keyed_service_factory.h
@@ -110,9 +110,8 @@
virtual void BrowserStateDestroyed(web::BrowserState* context);
private:
- // Registers any user preferences on this service. This is called by
- // RegisterPrefsIfNecessaryForContext() and should be overriden by any service
- // that wants to register profile-specific preferences.
+ // Registers any user preferences on this service. This should be overriden by
+ // any service that wants to register profile-specific preferences.
virtual void RegisterBrowserStatePrefs(
user_prefs::PrefRegistrySyncable* registry) {}
diff --git a/components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.h b/components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.h
index aa07a95..56afdfe 100644
--- a/components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.h
+++ b/components/keyed_service/ios/refcounted_browser_state_keyed_service_factory.h
@@ -115,9 +115,8 @@
virtual void BrowserStateDestroyed(web::BrowserState* context);
private:
- // Registers any user preferences on this service. This is called by
- // RegisterPrefsIfNecessaryForContext() and should be overriden by any service
- // that wants to register profile-specific preferences.
+ // Registers any user preferences on this service. This should be overriden by
+ // any service that wants to register profile-specific preferences.
virtual void RegisterBrowserStatePrefs(
user_prefs::PrefRegistrySyncable* registry) {}
