signin: pull basic SigninManager functionality into new SigninManagerBase class.

chrome/browser/signin/signin_manager_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_SIGNIN_SIGNIN_MANAGER_FACTORY_H_
 #define CHROME_BROWSER_SIGNIN_SIGNIN_MANAGER_FACTORY_H_
 
 #include "base/memory/singleton.h"
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
 
 class SigninManager;
+class SigninManagerBase;
 class PrefRegistrySimple;
 class PrefRegistrySyncable;
 class Profile;
 
 // Singleton that owns all SigninManagers and associates them with
 // Profiles. Listens for the Profile's destruction notification and cleans up
 // the associated SigninManager.
 class SigninManagerFactory : public ProfileKeyedServiceFactory {
  public:
   // Returns the instance of SigninManager associated with this profile
   // (creating one if none exists). Returns NULL if this profile cannot have a
   // SigninManager (for example, if |profile| is incognito).
-  static SigninManager* GetForProfile(Profile* profile);
+  static SigninManagerBase* GetBaseForProfile(Profile* profile);

[Roger Tawa OOO till Jul 10th, 2013/03/27 17:36:33]

Instead of two methods GetBaseForProfile() and GetForProfile(), why not
something like:

#ifdef chromeos
  static SigninManagerBase* GetForProfile();
#else
  static SigninManager* GetForProfile();
#endif

Same for XXXIfExists().  I guess you're thinking that on desktop some callers
only need the "basic" functionality and could call GetBaseForProfile().  Not
sure if the extra complexity is worth it.

[tim (not reviewing), 2013/03/29 18:03:16]

Right I noticed the vast majority of callsites just want Base functionality /
dependencies, so it seemed worth it to me. But I guess it'd be easier to do it
your way... I"ll change it.

 
   // Returns the instance of SigninManager associated with this profile. Returns
   // null if no SigninManager instance currently exists (will not create a new
   // instance).
+  static SigninManagerBase* GetBaseForProfileIfExists(Profile* profile);
+
+  // On non-ChromeOS platforms, the SigninManager the factory creates will be
+  // an instance of the extended SigninManager class.
+  //
+  // TODO(tim): Make the above statement true in BuildServiceInstanceFor, and
+  // rename these (to e.g GetExtendedForProfile) and ifdef out on chrome os.

[Roger Tawa OOO till Jul 10th, 2013/03/27 17:36:33]

Why change BuildServiceInstanceFor() in a second step?

[tim (not reviewing), 2013/03/29 18:03:16]

I wanted to split it up to avoid a massive CL mixed with real changes and
renames. But with the ifdef suggestion I implemented I'll need to audit all the
callsites. It also looks like I'll have to refactor ProfileSyncServiceHarness,
since it's using StartSignin on all platforms.

I'll let you know when I have a working patch again.

+  // Most callers should use SigninManagerBase.
+  static SigninManager* GetForProfile(Profile* profile);
   static SigninManager* GetForProfileIfExists(Profile* profile);
 
   // Returns an instance of the SigninManagerFactory singleton.
   static SigninManagerFactory* GetInstance();
 
   // Implementation of ProfileKeyedServiceFactory (public so tests can call it).
   virtual void RegisterUserPrefs(PrefRegistrySyncable* registry) OVERRIDE;
 
   // Registers the browser-global prefs used by SigninManager.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
  private:
   friend struct DefaultSingletonTraits<SigninManagerFactory>;
 
   SigninManagerFactory();
   virtual ~SigninManagerFactory();
 
   // ProfileKeyedServiceFactory:
   virtual ProfileKeyedService* BuildServiceInstanceFor(
       Profile* profile) const OVERRIDE;
 };
 
 #endif  // CHROME_BROWSER_SIGNIN_SIGNIN_MANAGER_FACTORY_H_