Profile shouldn't own DesktopNotificationService.

chrome/browser/profiles/profile_keyed_service_factory.h

Index: chrome/browser/profiles/profile_keyed_service_factory.h
diff --git a/chrome/browser/profiles/profile_keyed_service_factory.h b/chrome/browser/profiles/profile_keyed_service_factory.h
index fd33b150b11854d5d33a9f822efc9d124b5bb46a..632dc46bc048db6f2690952848b24c278da3906e 100644
--- a/chrome/browser/profiles/profile_keyed_service_factory.h
+++ b/chrome/browser/profiles/profile_keyed_service_factory.h
@@ -20,10 +20,16 @@ class ProfileKeyedService;
 // shutdown/destruction order. In each derived classes' constructors, the
 // implementors must explicitly state which services are depended on.
 class ProfileKeyedServiceFactory {
- protected:
-  friend class ProfileDependencyManager;
-  friend class ProfileDependencyManagerUnittests;
+ public:
+  typedef ProfileKeyedService* (*FactoryFunction)(Profile* profile);
 
+#if defined(UNIT_TEST)
+  // Sets the factory which will be used by |GetServiceFunction|. A value of
+  // NULL (the default) results in a call to |BuildServiceInstanceFor|.
+  void set_factory_function(FactoryFunction factory) { factory_ = factory; }

[Elliot Glaysher, 2011/04/08 17:43:32]

This is fine, but I'm worried that there isn't an association between a Profile
and the factory function.

Could this be modified to be something like set_factory_function_for(Profile* p,
FactoryFunction factory);? I know there are places in our unit tests where we
override the construction of one of the Profile::GetBlah() methods with a mock
only for one or two tests. Since ProfileKeyedServiceFactory is also a Singleton
in most cases, I'm also worried about one test setting state that will bleed
into another.

After doing that, could you please update the unit tests in theme_util_test.cc
to use your new system instead of the previous one I wrote?

[Torne, 2011/04/11 11:52:51]

I was assuming that test code does not create normal profiles, and thus having
TestingProfile's changes be global was reasonable (and is certainly easier...).
I don't actually know if this is true; I considered having it only apply to
things which were TestingProfiles... 

I wasn't intending that this should replace the mechanism you used for the theme
service. I don't see any benefit to the added complexity of installing a factory
function rather than just a constructed instance of the service in the case
where the test is doing it as a one-off thing; the reason to have a factory
function for stuff that was being replaced in TestingProfile is because of the
issues we previously discussed in this review (eager creation being a problem
for tests that use setups where that's not a legitimate thing to do).

I'll have a think about this a bit more...

+#endif
+
+ protected:
   // ProfileKeyedServiceFactories must communicate with a
   // ProfileDependencyManager. For all non-test code, write your subclass
   // constructors like this:
@@ -74,6 +80,9 @@ class ProfileKeyedServiceFactory {
   virtual void ProfileDestroyed(Profile* profile);
 
  private:
+  friend class ProfileDependencyManager;
+  friend class ProfileDependencyManagerUnittests;
+
   // The mapping between a Profile and its service.
   std::map<Profile*, ProfileKeyedService*> mapping_;
 
@@ -81,6 +90,9 @@ class ProfileKeyedServiceFactory {
   // this will always be ProfileDependencyManager::GetInstance(), but unit
   // tests will want to use their own copy.
   ProfileDependencyManager* dependency_manager_;
+
+  // An alternative function to use to create the service, used by tests.
+  FactoryFunction factory_;
 };
 
 #endif  // CHROME_BROWSER_PROFILES_PROFILE_KEYED_SERVICE_FACTORY_H_