Move the SigninProcess APIs from SigninManager to ChromeSigninClient.

These APIs are //chrome-specific, and rely on no other state from
SigninManager. As these APIs are per-Profile, ChromeSigninClient is an
appropriate place for them to live.

BUG=334205
TBR=thakis

Review URL: https://codereview.chromium.org/216703002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260163 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_content_browser_client.cc

@@ -199,6 +199,8 @@
 #endif
 
 #if !defined(OS_CHROMEOS)
+#include "chrome/browser/signin/chrome_signin_client.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #endif
@@ -1180,8 +1182,9 @@ bool ChromeContentBrowserClient::IsSuitableHost(
   }
 
 #if !defined(OS_CHROMEOS)
-  SigninManager* signin_manager = SigninManagerFactory::GetForProfile(profile);
-  if (signin_manager && signin_manager->IsSigninProcess(process_host->GetID()))
+  ChromeSigninClient* signin_client =
+      ChromeSigninClientFactory::GetForProfile(profile);
+  if (signin_client && signin_client->IsSigninProcess(process_host->GetID()))
     return SigninManager::IsWebBasedSigninFlowURL(site_url);
 #endif
 
@@ -1286,10 +1289,10 @@ void ChromeContentBrowserClient::SiteInstanceGotProcess(
   // for signin URLs. The signin process will be cleared from SigninManager
   // when the renderer is destroyed.
   if (SigninManager::IsWebBasedSigninFlowURL(site_instance->GetSiteURL())) {
-    SigninManager* signin_manager =
-        SigninManagerFactory::GetForProfile(profile);
-    if (signin_manager)
-      signin_manager->SetSigninProcess(site_instance->GetProcess()->GetID());
+    ChromeSigninClient* signin_client =
+        ChromeSigninClientFactory::GetForProfile(profile);
+    if (signin_client)
+      signin_client->SetSigninProcess(site_instance->GetProcess()->GetID());
     BrowserThread::PostTask(
         BrowserThread::IO,
         FROM_HERE,
@@ -1541,9 +1544,9 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
         command_line->AppendSwitch(switches::kInstantProcess);
 
 #if !defined(OS_CHROMEOS)
-      SigninManager* signin_manager =
-          SigninManagerFactory::GetForProfile(profile);
-      if (signin_manager && signin_manager->IsSigninProcess(process->GetID()))
+      ChromeSigninClient* signin_client =
+          ChromeSigninClientFactory::GetForProfile(profile);
+      if (signin_client && signin_client->IsSigninProcess(process->GetID()))
         command_line->AppendSwitch(switches::kSigninProcess);
 #endif
     }

chrome/browser/extensions/browser_permissions_policy_delegate.cc

@@ -11,8 +11,8 @@
 #include "extensions/common/manifest_constants.h"
 
 #if !defined(OS_CHROMEOS)
-#include "chrome/browser/signin/signin_manager.h"
-#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/signin/chrome_signin_client.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
 #endif
 
 namespace extensions {
@@ -48,9 +48,9 @@ bool BrowserPermissionsPolicyDelegate::CanExecuteScriptOnPage(
       g_browser_process->profile_manager()->GetLoadedProfiles();
   for (std::vector<Profile*>::iterator profile = profiles.begin();
        profile != profiles.end(); ++profile) {
-    SigninManager* signin_manager =
-        SigninManagerFactory::GetForProfile(*profile);
-    if (signin_manager && signin_manager->IsSigninProcess(process_id)) {
+    ChromeSigninClient* signin_client =
+        ChromeSigninClientFactory::GetForProfile(*profile);
+    if (signin_client && signin_client->IsSigninProcess(process_id)) {
       if (error)
         *error = errors::kCannotScriptSigninPage;
       return false;

chrome/browser/extensions/browser_permissions_policy_delegate_unittest.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/signin/chrome_signin_client.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "chrome/browser/signin/signin_manager.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -62,9 +64,10 @@ TEST_F(BrowserPermissionsPolicyDelegateTest, CanExecuteScriptOnPage) {
 
   content::MockRenderProcessHost signin_process(profile_);
   content::MockRenderProcessHost normal_process(profile_);
-  SigninManager* signin_manager = SigninManagerFactory::GetForProfile(profile_);
-  ASSERT_TRUE(signin_manager);
-  signin_manager->SetSigninProcess(signin_process.GetID());
+  ChromeSigninClient* signin_client =
+      ChromeSigninClientFactory::GetForProfile(profile_);
+  ASSERT_TRUE(signin_client);
+  signin_client->SetSigninProcess(signin_process.GetID());
 
   scoped_refptr<const Extension> extension(CreateTestExtension("a"));
   std::string error;

chrome/browser/signin/chrome_signin_client.cc

@@ -8,6 +8,8 @@
 #include "chrome/browser/signin/local_auth.h"
 #include "chrome/browser/webdata/web_data_service_factory.h"
 #include "chrome/common/profile_management_switches.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/common/child_process_host.h"
 #include "url/gurl.h"
 
 #if defined(ENABLE_MANAGED_USERS)
@@ -18,15 +20,25 @@
 #include "chrome/browser/chromeos/login/user_manager.h"
 #endif
 
+using content::ChildProcessHost;
+using content::RenderProcessHost;
+
 namespace {
 
 const char kGoogleAccountsUrl[] = "https://accounts.google.com";
 
 }  // namespace
 
-ChromeSigninClient::ChromeSigninClient(Profile* profile) : profile_(profile) {}
+ChromeSigninClient::ChromeSigninClient(Profile* profile)
+    : profile_(profile), signin_host_id_(ChildProcessHost::kInvalidUniqueID) {}
 
-ChromeSigninClient::~ChromeSigninClient() {}
+ChromeSigninClient::~ChromeSigninClient() {
+  std::set<RenderProcessHost*>::iterator i;
+  for (i = signin_hosts_observed_.begin(); i != signin_hosts_observed_.end();
+       ++i) {
+    (*i)->RemoveObserver(this);
+  }
+}
 
 // static
 bool ChromeSigninClient::ProfileAllowsSigninCookies(Profile* profile) {
@@ -43,6 +55,40 @@ bool ChromeSigninClient::SettingsAllowSigninCookies(
                                                  GURL(kGoogleAccountsUrl));
 }
 
+void ChromeSigninClient::SetSigninProcess(int process_id) {
+  if (process_id == signin_host_id_)
+    return;
+  DLOG_IF(WARNING, signin_host_id_ != ChildProcessHost::kInvalidUniqueID)
+      << "Replacing in-use signin process.";
+  signin_host_id_ = process_id;
+  RenderProcessHost* host = RenderProcessHost::FromID(process_id);
+  DCHECK(host);
+  host->AddObserver(this);
+  signin_hosts_observed_.insert(host);
+}
+
+void ChromeSigninClient::ClearSigninProcess() {
+  signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
+}
+
+bool ChromeSigninClient::IsSigninProcess(int process_id) const {
+  return process_id == signin_host_id_;
+}
+
+bool ChromeSigninClient::HasSigninProcess() const {
+  return signin_host_id_ != ChildProcessHost::kInvalidUniqueID;
+}
+
+void ChromeSigninClient::RenderProcessHostDestroyed(RenderProcessHost* host) {
+  // It's possible we're listening to a "stale" renderer because it was replaced
+  // with a new process by process-per-site. In either case, stop observing it,
+  // but only reset signin_host_id_ tracking if this was from the current signin
+  // process.
+  signin_hosts_observed_.erase(host);
+  if (signin_host_id_ == host->GetID())
+    signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
+}
+
 PrefService* ChromeSigninClient::GetPrefs() { return profile_->GetPrefs(); }
 
 scoped_refptr<TokenWebData> ChromeSigninClient::GetDatabase() {

chrome/browser/signin/chrome_signin_client.h

@@ -9,11 +9,14 @@
 #include "base/compiler_specific.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/signin/core/browser/signin_client.h"
+#include "content/public/browser/render_process_host_observer.h"
 
 class CookieSettings;
 class Profile;
 
-class ChromeSigninClient : public SigninClient, public KeyedService {
+class ChromeSigninClient : public SigninClient,
+                           public KeyedService,
+                           public content::RenderProcessHostObserver {
  public:
   explicit ChromeSigninClient(Profile* profile);
   virtual ~ChromeSigninClient();
@@ -22,6 +25,23 @@ class ChromeSigninClient : public SigninClient, public KeyedService {
   static bool ProfileAllowsSigninCookies(Profile* profile);
   static bool SettingsAllowSigninCookies(CookieSettings* cookie_settings);
 
+  // Tracks the privileged signin process identified by |host_id| so that we
+  // can later ask (via IsSigninProcess) if it is safe to sign the user in from
+  // the current context (see OneClickSigninHelper).  All of this tracking
+  // state is reset once the renderer process terminates.
+  //
+  // N.B. This is the id returned by RenderProcessHost::GetID().
+  // TODO(guohui): Eliminate these APIs once the web-based signin flow is
+  // replaced by a native flow. crbug.com/347247
+  void SetSigninProcess(int host_id);
+  void ClearSigninProcess();
+  bool IsSigninProcess(int host_id) const;
+  bool HasSigninProcess() const;
+
+  // content::RenderProcessHostObserver implementation.
+  virtual void RenderProcessHostDestroyed(content::RenderProcessHost* host)
+      OVERRIDE;
+
   // SigninClient implementation.
   virtual PrefService* GetPrefs() OVERRIDE;
   virtual scoped_refptr<TokenWebData> GetDatabase() OVERRIDE;
@@ -33,6 +53,13 @@ class ChromeSigninClient : public SigninClient, public KeyedService {
  private:
   Profile* profile_;
 
+  // See SetSigninProcess. Tracks the currently active signin process
+  // by ID, if there is one.
+  int signin_host_id_;
+
+  // The RenderProcessHosts being observed.
+  std::set<content::RenderProcessHost*> signin_hosts_observed_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeSigninClient);
 };
 

chrome/browser/signin/signin_browsertest.cc

@@ -6,8 +6,8 @@
 #define CHROME_BROWSER_SIGNIN_SIGNIN_BROWSERTEST_H_
 
 #include "base/command_line.h"
-#include "chrome/browser/signin/signin_manager.h"
-#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/signin/chrome_signin_client.h"
+#include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "chrome/browser/signin/signin_promo.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/singleton_tabs.h"
@@ -107,8 +107,8 @@ const bool kOneClickSigninEnabled = false;
 #define MAYBE_ProcessIsolation ProcessIsolation
 #endif
 IN_PROC_BROWSER_TEST_F(SigninBrowserTest, MAYBE_ProcessIsolation) {
-  SigninManager* signin = SigninManagerFactory::GetForProfile(
-      browser()->profile());
+  ChromeSigninClient* signin =
+      ChromeSigninClientFactory::GetForProfile(browser()->profile());
   EXPECT_FALSE(signin->HasSigninProcess());
 
   ui_test_utils::NavigateToURL(browser(), signin::GetPromoURL(
@@ -149,8 +149,8 @@ IN_PROC_BROWSER_TEST_F(SigninBrowserTest, MAYBE_ProcessIsolation) {
 }
 
 IN_PROC_BROWSER_TEST_F(SigninBrowserTest, NotTrustedAfterRedirect) {
-  SigninManager* signin = SigninManagerFactory::GetForProfile(
-      browser()->profile());
+  ChromeSigninClient* signin =
+      ChromeSigninClientFactory::GetForProfile(browser()->profile());
   EXPECT_FALSE(signin->HasSigninProcess());
 
   GURL url = signin::GetPromoURL(signin::SOURCE_NTP_LINK, true);
@@ -206,8 +206,8 @@ IN_PROC_BROWSER_TEST_F(SigninBrowserTest, SigninSkipForNowAndGoBack) {
   GURL start_url = signin::GetPromoURL(signin::SOURCE_START_PAGE, true);
   GURL skip_url = signin::GetLandingURL("ntp", 1);
 
-  SigninManager* signin = SigninManagerFactory::GetForProfile(
-      browser()->profile());
+  ChromeSigninClient* signin =
+      ChromeSigninClientFactory::GetForProfile(browser()->profile());
   EXPECT_FALSE(signin->HasSigninProcess());
 
   ui_test_utils::NavigateToURL(browser(), start_url);

chrome/browser/signin/signin_manager.cc

@@ -23,18 +23,13 @@
 #include "components/signin/core/browser/signin_internals_util.h"
 #include "components/signin/core/browser/signin_manager_cookie_helper.h"
 #include "content/public/browser/notification_service.h"
-#include "content/public/browser/render_process_host.h"
-#include "content/public/common/child_process_host.h"
 #include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/base/escape.h"
 #include "third_party/icu/source/i18n/unicode/regex.h"
 
 using namespace signin_internals_util;
 
-using content::ChildProcessHost;
-using content::RenderProcessHost;
-
 namespace {
 
 const char kChromiumSyncService[] = "service=chromiumsync";
@@ -72,34 +67,8 @@ SigninManager::SigninManager(SigninClient* client)
       prohibit_signout_(false),
       type_(SIGNIN_TYPE_NONE),
       weak_pointer_factory_(this),
-      signin_host_id_(ChildProcessHost::kInvalidUniqueID),
       client_(client) {}
 
-void SigninManager::SetSigninProcess(int process_id) {
-  if (process_id == signin_host_id_)
-    return;
-  DLOG_IF(WARNING,
-          signin_host_id_ != ChildProcessHost::kInvalidUniqueID)
-      << "Replacing in-use signin process.";
-  signin_host_id_ = process_id;
-  RenderProcessHost* host = RenderProcessHost::FromID(process_id);
-  DCHECK(host);
-  host->AddObserver(this);
-  signin_hosts_observed_.insert(host);
-}
-
-void SigninManager::ClearSigninProcess() {
-  signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
-}
-
-bool SigninManager::IsSigninProcess(int process_id) const {
-  return process_id == signin_host_id_;
-}
-
-bool SigninManager::HasSigninProcess() const {
-  return signin_host_id_ != ChildProcessHost::kInvalidUniqueID;
-}
-
 void SigninManager::AddMergeSessionObserver(
     MergeSessionHelper::Observer* observer) {
   if (merge_session_helper_)
@@ -113,12 +82,6 @@ void SigninManager::RemoveMergeSessionObserver(
 }
 
 SigninManager::~SigninManager() {
-  std::set<RenderProcessHost*>::iterator i;
-  for (i = signin_hosts_observed_.begin();
-       i != signin_hosts_observed_.end();
-       ++i) {
-    (*i)->RemoveObserver(this);
-  }
 }
 
 void SigninManager::InitTokenService() {
@@ -445,16 +408,6 @@ void SigninManager::OnSignedIn(const std::string& username) {
   DisableOneClickSignIn(profile_);  // Don't ever offer again.
 }
 
-void SigninManager::RenderProcessHostDestroyed(RenderProcessHost* host) {
-  // It's possible we're listening to a "stale" renderer because it was replaced
-  // with a new process by process-per-site. In either case, stop observing it,
-  // but only reset signin_host_id_ tracking if this was from the current signin
-  // process.
-  signin_hosts_observed_.erase(host);
-  if (signin_host_id_ == host->GetID())
-    signin_host_id_ = ChildProcessHost::kInvalidUniqueID;
-}
-
 void SigninManager::ProhibitSignout(bool prohibit_signout) {
   prohibit_signout_ = prohibit_signout;
 }