Profiles: Really fix refcounted services.

chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc

Index: chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
diff --git a/chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc b/chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
index 8bf694caa66478ba65ac84a2c408d0bf3d732bd3..5e170633eff0ad529887dea5e8789df81800ba76 100644
--- a/chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
+++ b/chrome/browser/profiles/refcounted_profile_keyed_service_factory.cc
@@ -5,12 +5,43 @@
 #include "chrome/browser/profiles/refcounted_profile_keyed_service_factory.h"
 
 #include "base/logging.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 #include "chrome/browser/profiles/refcounted_profile_keyed_service.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
+void RefcountedProfileKeyedServiceFactory::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;
+}
+
+scoped_refptr<RefcountedProfileKeyedService>
+RefcountedProfileKeyedServiceFactory::SetTestingFactoryAndUse(
+    Profile* profile,
+    FactoryFunction factory) {
+  DCHECK(factory);
+  SetTestingFactory(profile, factory);

[Miranda Callahan, 2012/03/15 16:55:35]

see my comment in PKSF -- SetTestingFactory(GetProfileToUse(profile), factory)?

+  return GetServiceForProfile(profile, true);
+}
+
 RefcountedProfileKeyedServiceFactory::RefcountedProfileKeyedServiceFactory(
     const char* name,
     ProfileDependencyManager* manager)
@@ -21,26 +52,44 @@ RefcountedProfileKeyedServiceFactory::~RefcountedProfileKeyedServiceFactory() {
   DCHECK(mapping_.empty());
 }
 
-void RefcountedProfileKeyedServiceFactory::Associate(Profile* profile,
-                                                     ProfileKeyedBase* base) {
-  DCHECK(mapping_.find(profile) == mapping_.end());
-  mapping_.insert(std::make_pair(
-      profile, make_scoped_refptr(
-          static_cast<RefcountedProfileKeyedService*>(base))));
-}
-
-bool RefcountedProfileKeyedServiceFactory::GetAssociation(
+scoped_refptr<RefcountedProfileKeyedService>
+RefcountedProfileKeyedServiceFactory::GetServiceForProfile(
     Profile* profile,
-    ProfileKeyedBase** out) const {
-  bool found = false;
+    bool create) {
+  profile = GetProfileToUse(profile);
+  if (!profile)
+    return NULL;
+
+  // NOTE: If you modify any of the logic below, make sure to update the
+  // non-refcounted version in profile_keyed_service_factory.cc!
+  scoped_refptr<RefcountedProfileKeyedService> service;
   RefCountedStorage::const_iterator it = mapping_.find(profile);
   if (it != mapping_.end()) {
-    found = true;
+    return it->second;
+  } else if (create) {
+    // not found but creation allowed

[Miranda Callahan, 2012/03/15 16:55:35]

nit as before: sentencing

 
-    if (out)
-      *out = it->second.get();
+    // Check to see if we have a per-Profile factory

[Miranda Callahan, 2012/03/15 16:55:35]

nit as before: append a period.

+    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 16:55:35]

nit: I sentence thee

+    return NULL;
   }
-  return found;
+
+  DCHECK(mapping_.find(profile) == mapping_.end());
+  mapping_.insert(std::make_pair(profile, service));

[Miranda Callahan, 2012/03/15 16:55:35]

These two lines are encapsulated in the "Associate" method in PKSF; either do
that here as well, or un-do-that in PKSF.

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

I only did that in PKSF because only in the non-refcounted version are there
cases where a factory subclass needs those details. Made symmetric though, since
that's The Right Thing.

+  return service;
 }
 
 void RefcountedProfileKeyedServiceFactory::ProfileShutdown(Profile* profile) {
@@ -57,5 +106,20 @@ void RefcountedProfileKeyedServiceFactory::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 RefcountedProfileKeyedServiceFactory::SetEmptyTestingFactory(
+    Profile* profile) {
+  SetTestingFactory(profile, NULL);
+}
+
+void RefcountedProfileKeyedServiceFactory::CreateServiceNow(Profile* profile) {
+  GetServiceForProfile(profile, true);
+}