Profiles: Really fix refcounted services.

chrome/browser/profiles/profile_keyed_base_factory.cc

Index: chrome/browser/profiles/profile_keyed_base_factory.cc
diff --git a/chrome/browser/profiles/profile_keyed_base_factory.cc b/chrome/browser/profiles/profile_keyed_base_factory.cc
index 1b63cb9691281dfec262991f4e88f5e3cd992c03..eacfad4dfaf9fd76b92827bdf4a07759d5508202 100644
--- a/chrome/browser/profiles/profile_keyed_base_factory.cc
+++ b/chrome/browser/profiles/profile_keyed_base_factory.cc
@@ -7,35 +7,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_dependency_manager.h"
 
-void ProfileKeyedBaseFactory::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 = registered_preferences_.find(profile) !=
-                     registered_preferences_.end();
-
-  // 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)
-    registered_preferences_.insert(profile);
-
-  factories_[profile] = factory;
-}
-
-ProfileKeyedBase* ProfileKeyedBaseFactory::SetTestingFactoryAndUse(
-    Profile* profile,
-    FactoryFunction factory) {
-  DCHECK(factory);
-  SetTestingFactory(profile, factory);
-  return GetBaseForProfile(profile, true);
-}
-
 ProfileKeyedBaseFactory::ProfileKeyedBaseFactory(
     const char* name, ProfileDependencyManager* manager)
     : dependency_manager_(manager)
@@ -54,6 +25,31 @@ void ProfileKeyedBaseFactory::DependsOn(ProfileKeyedBaseFactory* rhs) {
   dependency_manager_->AddEdge(rhs, this);
 }
 
+Profile* ProfileKeyedBaseFactory::GetProfileToUse(Profile* profile) {
+  DCHECK(CalledOnValidThread());
+
+#ifndef NDEBUG
+  dependency_manager_->AssertProfileWasntDestroyed(profile);
+#endif
+
+  // Possibly handle Incognito mode.
+  if (profile->IsOffTheRecord()) {
+    if (ServiceRedirectedInIncognito()) {
+      profile = profile->GetOriginalProfile();
+
+#ifndef NDEBUG
+      dependency_manager_->AssertProfileWasntDestroyed(profile);

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

These assertions that the profile wasn't destroyed twice in one method call
scare me and make me think we need some more clever way of making sure that
profile destruction timing doesn't interfere with factory methods. This isn't to
say that this should be done right now; just a comment.

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

While suboptimal, we're checking a different Profile object in this case.

[Miranda Callahan, 2012/03/15 18:11:21]

Good point!

+#endif
+    } else if (ServiceHasOwnInstanceInIncognito()) {
+      // No-op; the pointers are already set correctly.
+    } else {
+      return NULL;
+    }
+  }
+
+  return profile;
+}
+
 void ProfileKeyedBaseFactory::RegisterUserPrefsOnProfile(Profile* profile) {
   // Safe timing for pref registration is hard. Previously, we made Profile
   // responsible for all pref registration on every service that used
@@ -105,67 +101,19 @@ bool ProfileKeyedBaseFactory::ServiceIsNULLWhileTesting() {
   return false;
 }
 
-ProfileKeyedBase* ProfileKeyedBaseFactory::GetBaseForProfile(
-    Profile* profile,
-    bool create) {
-  DCHECK(CalledOnValidThread());
-
-#ifndef NDEBUG
-  dependency_manager_->AssertProfileWasntDestroyed(profile);
-#endif
-
-  // Possibly handle Incognito mode.
-  if (profile->IsOffTheRecord()) {
-    if (ServiceRedirectedInIncognito()) {
-      profile = profile->GetOriginalProfile();
-
-#ifndef NDEBUG
-      dependency_manager_->AssertProfileWasntDestroyed(profile);
-#endif
-    } else if (ServiceHasOwnInstanceInIncognito()) {
-      // No-op; the pointers are already set correctly.
-    } else {
-      return NULL;
-    }
-  }
-
-  ProfileKeyedBase* service = NULL;
-  if (GetAssociation(profile, &service)) {
-    return service;
-  } else if (create) {
-    // not found but creation allowed
-
-    // Check to see if we have a per-Profile 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
-    return NULL;
-  }
-
-  Associate(profile, service);
-  return service;
-}
-
 void ProfileKeyedBaseFactory::ProfileDestroyed(Profile* profile) {
   // While object destruction can be customized in ways where the object is
   // only dereferenced, this still must run on the UI thread.
   DCHECK(CalledOnValidThread());
 
-  // 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);
   registered_preferences_.erase(profile);
 }
+
+bool ProfileKeyedBaseFactory::ArePreferencesSetOn(Profile* profile) {
+  return registered_preferences_.find(profile) !=
+      registered_preferences_.end();
+}
+
+void ProfileKeyedBaseFactory::MarkPreferencesSetOn(Profile* profile) {

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

Should we add a DCHECK(!ArePreferencesSetOn(profile)) here?

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

Done.

+  registered_preferences_.insert(profile);
+}