Profiles: Really fix refcounted services.

chrome/browser/profiles/profile_keyed_service_factory.h

 // Copyright (c) 2012 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>
 
 #include "base/compiler_specific.h"
+#include "base/basictypes.h"

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

nit: b before c except after e. actually, also after e. 

 #include "chrome/browser/profiles/profile_keyed_base_factory.h"
 #include "chrome/browser/profiles/profile_keyed_service.h"
 
 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 ProfileKeyedBaseFactory {
+ public:
+  // A function that replaces the (possibly internal) object used by this
+  // factory. For the majority of cases, this is the object returned to users.
+  typedef ProfileKeyedService* (*FactoryFunction)(Profile* profile);

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

Shouldn't FactoryFunction be described as "A function that takes a Profile* and
returns the PKS associated with that Profile"? I'm not sure what you mean by
"replaces the (possibly internal) object used by this factory" wrt this function
definition.

+
+  // Associates |factory| with |profile| so that |factory| is used to create
+  // the ProfileKeyedService when requested.
+  //

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

nit: remove blank line.

+  // |factory| can be NULL to signal that ProfileKeyedService should be NULL. A
+  // second call to SetTestingFactory() is allowed. If the FactoryFunction is

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

instead of "a second call", maybe "multiple calls ... are allowed." 

+  // changed AND an instance of the PKSF already exists for |profile|, that
+  // service is destroyed.
+  void SetTestingFactory(Profile* profile, FactoryFunction factory);
+
+  // Associates |factory| with |profile| and immediately returns the created
+  // ProfileKeyedService. Since the factory will be used immediately, it may
+  // not be NULL;

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

nit: s/;/.

+  ProfileKeyedService* SetTestingFactoryAndUse(Profile* profile,
+                                               FactoryFunction factory);
+
  protected:
   // ProfileKeyedServiceFactories must communicate with a
   // ProfileDependencyManager. For all non-test code, write your subclass
   // constructors like this:
   //
   //   MyServiceFactory::MyServiceFactory()
   //     : ProfileKeyedServiceFactory(
   //         "MyService",
   //         ProfileDependencyManager::GetInstance())
   //   {}
   ProfileKeyedServiceFactory(const char* name,
                              ProfileDependencyManager* manager);
   virtual ~ProfileKeyedServiceFactory();
 
   // Common implementation that maps |profile| to some service object. Deals
   // with incognito profiles per subclass instructions with
   // ServiceRedirectedInIncognito() and ServiceHasOwnInstanceInIncognito().

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

Change comment to reflect the fact that we're using GetProfileToUse to deal with
incognito selection; otherwise we're referencing internal details of a base
class method from a subclass, which means we might fall prey to inattentive
maintainers.

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

Done.

   // If |create| is true, the service will be created using
   // BuildServiceInstanceFor() if it doesn't already exist.
   ProfileKeyedService* GetServiceForProfile(Profile* profile, bool create);
 
+  // Maps |profile| to |provider| with debug checks to prevent duplication.

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

s/|provider|/|service|/

+  void Associate(Profile* profile, ProfileKeyedService* service);
+
   // All subclasses of ProfileKeyedServiceFactory must return a
   // ProfileKeyedService instead of just a ProfileKeyedBase.
   virtual ProfileKeyedService* BuildServiceInstanceFor(
       Profile* profile) const = 0;
 
-  // Maps |profile| to |provider| with debug checks to prevent duplication.
-  virtual void Associate(Profile* profile, ProfileKeyedBase* base) OVERRIDE;
-
-  // Returns the previously associated |base| for |profile|, or NULL.
-  virtual bool GetAssociation(Profile* profile,
-                              ProfileKeyedBase** out) const OVERRIDE;
-
   // A helper object actually listens for notifications about Profile
   // destruction, calculates the order in which things are destroyed and then
   // does a two pass shutdown.
   //
   // First, ProfileShutdown() is called on every ServiceFactory and will
   // 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.

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

Every time I see "ProfileShutdown" and "ProfileDestroyed" I have a heart attack,
and have to look to make sure that the profiles aren't actually being shutdown
and destroyed, and then I remember what's going on here. I wonder if this whole
comment should be moved to the PDM's DestroyProfileServices method, which is
where this stuff is actually happening, again under the rule of Comment the
Details Where They Are Actually Happening -- and then here we can just say what
these methods actually do. This will also be helpful because they're called
outside of the notification system described here -- for example, when we set a
new factory in testing. 

One might also rename them according to what they do, and not according to the
context in which they're generally called, to also avoid confusion from easily
frightened people such as myself. If you think I should just be less easily
scared, that's fine too. :-)

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

Can I do that as a separate patch? Adding another method rename is a lot of
refactoring in an already busy patch.

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

Oh yes, definitely.

   virtual void ProfileShutdown(Profile* profile) OVERRIDE;
   virtual void ProfileDestroyed(Profile* profile) OVERRIDE;
 
+  virtual void SetEmptyTestingFactory(Profile* profile) OVERRIDE;
+  virtual void CreateServiceNow(Profile* profile) OVERRIDE;
+
  private:
   friend class ProfileDependencyManager;
   friend class ProfileDependencyManagerUnittests;
 
   // The mapping between a Profile and its service.
   std::map<Profile*, ProfileKeyedService*> mapping_;
+
+  // The mapping between a Profile and its overridden FactoryFunction.
+  std::map<Profile*, FactoryFunction> factories_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProfileKeyedServiceFactory);
 };
 
 #endif  // CHROME_BROWSER_PROFILES_PROFILE_KEYED_SERVICE_FACTORY_H_