Profiles: Really fix refcounted services.

chrome/browser/profiles/profile_keyed_service_factory.cc

Index: chrome/browser/profiles/profile_keyed_service_factory.cc
diff --git a/chrome/browser/profiles/profile_keyed_service_factory.cc b/chrome/browser/profiles/profile_keyed_service_factory.cc
index 10e14599ffa0bf25f410f659c08fc7a40031cef5..3669ea6463f36055f03c3f4c1dd8a304a2487625 100644
--- a/chrome/browser/profiles/profile_keyed_service_factory.cc
+++ b/chrome/browser/profiles/profile_keyed_service_factory.cc
@@ -11,6 +11,34 @@
 #include "chrome/browser/profiles/profile_dependency_manager.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 
+void ProfileKeyedServiceFactory::SetTestingFactory(Profile* profile,
+                                                   FactoryFunction factory) {
+  // Destroying the profile may cause us to lose data about whether |profile|
+  // has our preferences registered on it (since the profile object itself
+  // isn't dead). See if we need to readd it once we've gone through normal
+  // destruction.
+  bool add_profile = ArePreferencesSetOn(profile);
+
+  // We have to go through the shutdown and destroy mechanisms because there
+  // are unit tests that create a service on a profile and then change the
+  // testing service mid-test.
+  ProfileShutdown(profile);
+  ProfileDestroyed(profile);
+
+  if (add_profile)
+    MarkPreferencesSetOn(profile);
+
+  factories_[profile] = factory;
+}
+
+ProfileKeyedService* ProfileKeyedServiceFactory::SetTestingFactoryAndUse(
+    Profile* profile,
+    FactoryFunction factory) {
+  DCHECK(factory);
+  SetTestingFactory(profile, factory);

[Miranda Callahan, 2012/03/15 15:31:07]

Do we actually need to call SetTestingFactory(GetProfileToUse(profile), factory)
or does this not matter?

[Elliot Glaysher, 2012/03/15 18:02:41]

IIRC, that would be incorrect. There are a few tests that set different mocks on
the incognito/normal profile.

+  return GetServiceForProfile(profile, true);
+}
+
 ProfileKeyedServiceFactory::ProfileKeyedServiceFactory(
     const char* name, ProfileDependencyManager* manager)
     : ProfileKeyedBaseFactory(name, manager) {
@@ -23,28 +51,46 @@ ProfileKeyedServiceFactory::~ProfileKeyedServiceFactory() {
 ProfileKeyedService* ProfileKeyedServiceFactory::GetServiceForProfile(
     Profile* profile,
     bool create) {
-  return static_cast<ProfileKeyedService*>(GetBaseForProfile(profile, create));
-}
-
-void ProfileKeyedServiceFactory::Associate(Profile* profile,
-                                           ProfileKeyedBase* service) {
-  DCHECK(mapping_.find(profile) == mapping_.end());
-  mapping_.insert(std::make_pair(
-      profile, static_cast<ProfileKeyedService*>(service)));
-}
+  profile = GetProfileToUse(profile);
+  if (!profile)
+    return NULL;
 
-bool ProfileKeyedServiceFactory::GetAssociation(
-    Profile* profile,
-    ProfileKeyedBase** out) const {
-  bool found = false;
+  // NOTE: If you modify any of the logic below, make sure to update the
+  // refcounted version in refcounted_profile_keyed_service_factory.cc!
+  ProfileKeyedService* service = NULL;
   std::map<Profile*, ProfileKeyedService*>::const_iterator it =
       mapping_.find(profile);
   if (it != mapping_.end()) {
-    found = true;
-    if (out)
-      *out = it->second;
+    return it->second;
+  } else if (create) {
+    // not found but creation allowed
+

[Miranda Callahan, 2012/03/15 15:31:07]

nit: remove blank line, and sentence-ify the "not found but creation allowed"
line above it.

+    // Check to see if we have a per-Profile factory

[Miranda Callahan, 2012/03/15 15:31:07]

nit: s/factory/factory:/

+    std::map<Profile*, FactoryFunction>::iterator jt = factories_.find(profile);
+    if (jt != factories_.end()) {
+      if (jt->second) {
+        if (!profile->IsOffTheRecord())
+          RegisterUserPrefsOnProfile(profile);
+        service = jt->second(profile);
+      } else {
+        service = NULL;
+      }
+    } else {
+      service = BuildServiceInstanceFor(profile);
+    }
+  } else {
+    // not found, creation forbidden

[Miranda Callahan, 2012/03/15 15:31:07]

nit: sentence-ify comment above.

+    return NULL;
   }
-  return found;
+
+  Associate(profile, service);
+  return service;
+}
+
+void ProfileKeyedServiceFactory::Associate(Profile* profile,
+                                           ProfileKeyedService* service) {
+  DCHECK(mapping_.find(profile) == mapping_.end());
+  mapping_.insert(std::make_pair(profile, service));
 }
 
 void ProfileKeyedServiceFactory::ProfileShutdown(Profile* profile) {
@@ -62,5 +108,20 @@ void ProfileKeyedServiceFactory::ProfileDestroyed(Profile* profile) {
     mapping_.erase(it);
   }
 
+  // For unit tests, we also remove the factory function both so we don't
+  // maintain a big map of dead pointers, but also since we may have a second
+  // object that lives at the same address (see other comments about unit tests
+  // in this file).
+  factories_.erase(profile);
+
   ProfileKeyedBaseFactory::ProfileDestroyed(profile);
 }
+
+void ProfileKeyedServiceFactory::SetEmptyTestingFactory(
+    Profile* profile) {
+  SetTestingFactory(profile, NULL);
+}
+
+void ProfileKeyedServiceFactory::CreateServiceNow(Profile* profile) {
+  GetServiceForProfile(profile, true);
+}