Profile shouldn't own DesktopNotificationService.

chrome/browser/profiles/profile_keyed_service_factory.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_PROFILES_PROFILE_KEYED_SERVICE_FACTORY_H_
 #define CHROME_BROWSER_PROFILES_PROFILE_KEYED_SERVICE_FACTORY_H_
 
 #include <map>
 
 class Profile;
 class ProfileDependencyManager;
 class ProfileKeyedService;
 
 // Base class for Factories that take a Profile object and return some service
 // on a one-to-one mapping. Each factory that derives from this class *must*
 // be a Singleton (only unit tests don't do that). See ThemeServiceFactory as
 // an example of how to derive from this class.
 //
 // We do this because services depend on each other and we need to control
 // shutdown/destruction order. In each derived classes' constructors, the
 // implementors must explicitly state which services are depended on.
 class ProfileKeyedServiceFactory {
+ 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:
-  friend class ProfileDependencyManager;
-  friend class ProfileDependencyManagerUnittests;
-
   // ProfileKeyedServiceFactories must communicate with a
   // ProfileDependencyManager. For all non-test code, write your subclass
   // constructors like this:
   //
   //   MyServiceFactory::MyServiceFactory()
   //     : ProfileKeyedServiceFactory(
   //         ProfileDependencyManager::GetInstance())
   //   {}
   explicit ProfileKeyedServiceFactory(ProfileDependencyManager* manager);
   virtual ~ProfileKeyedServiceFactory();
@@ skipping 30 matching lines @@
   // usually call ProfileKeyedService::Shutdown(), which gives each
   // ProfileKeyedService a chance to remove dependencies on other services that
   // it may be holding.
   //
   // Secondly, ProfileDestroyed() is called on every ServiceFactory and the
   // default implementation removes it from |mapping_| and deletes the pointer.
   virtual void ProfileShutdown(Profile* profile);
   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_;
 
   // Which ProfileDependencyManager we should communicate with. In real code,
   // 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_