Follow-up clean-up in LoginHandler

chrome/browser/ui/login/login_prompt.cc

 // 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.
 
 #include "chrome/browser/ui/login/login_prompt.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 37 matching lines @@
 using content::BrowserThread;
 using content::NavigationController;
 using content::RenderViewHost;
 using content::RenderViewHostDelegate;
 using content::ResourceDispatcherHost;
 using content::ResourceRequestInfo;
 using content::WebContents;
 
 class LoginHandlerImpl;
 
+namespace {
+
 // Helper to remove the ref from an net::URLRequest to the LoginHandler.
 // Should only be called from the IO thread, since it accesses an
 // net::URLRequest.
 void ResetLoginHandlerForRequest(net::URLRequest* request) {
   ResourceDispatcherHost::Get()->ClearLoginDelegateForRequest(request);
 }
 
-// Get the signon_realm under which this auth info should be stored.
-//
-// The format of the signon_realm for proxy auth is:
-//     proxy-host/auth-realm
-// The format of the signon_realm for server auth is:
-//     url-scheme://url-host[:url-port]/auth-realm
-//
-// Be careful when changing this function, since you could make existing
-// saved logins un-retrievable.
-std::string GetSignonRealm(const GURL& url,

[vabr (Chromium), 2015/10/08 10:23:34]

This whole method just got moved without changes to the end of the file, the
Public API section.

-                           const net::AuthChallengeInfo& auth_info) {
-  std::string signon_realm;
-  if (auth_info.is_proxy) {
-    signon_realm = auth_info.challenger.ToString();
-    signon_realm.append("/");
+// Helper to create a PasswordForm for PasswordManager to start looking for
+// saved credentials.
+PasswordForm MakeInputForPasswordManager(const GURL& request_url,

[vabr (Chromium), 2015/10/08 10:23:34]

This and the following methods have just been moved. The only exception is a
change noted below, otherwise the code was unchanged.

+                                         net::AuthChallengeInfo* auth_info) {
+  PasswordForm dialog_form;
+  if (base::LowerCaseEqualsASCII(auth_info->scheme, "basic")) {
+    dialog_form.scheme = PasswordForm::SCHEME_BASIC;
+  } else if (base::LowerCaseEqualsASCII(auth_info->scheme, "digest")) {
+    dialog_form.scheme = PasswordForm::SCHEME_DIGEST;
   } else {
-    // Take scheme, host, and port from the url.
-    signon_realm = url.GetOrigin().spec();
-    // This ends with a "/".
+    dialog_form.scheme = PasswordForm::SCHEME_OTHER;
   }
-  signon_realm.append(auth_info.realm);
-  return signon_realm;
+  std::string host_and_port(auth_info->challenger.ToString());
+  if (auth_info->is_proxy) {
+    std::string origin = host_and_port;
+    // We don't expect this to already start with http:// or https://.
+    DCHECK(origin.find("http://") != 0 && origin.find("https://") != 0);
+    origin = std::string("http://") + origin;
+    dialog_form.origin = GURL(origin);
+  } else if (!auth_info->challenger.Equals(
+                 net::HostPortPair::FromURL(request_url))) {
+    dialog_form.origin = GURL();
+    NOTREACHED();  // crbug.com/32718
+  } else {
+    dialog_form.origin = GURL(request_url.scheme() + "://" + host_and_port);
+  }
+  dialog_form.signon_realm = GetSignonRealm(dialog_form.origin, *auth_info);
+  return dialog_form;

[vabr (Chromium), 2015/10/08 10:23:34]

The handler->SetPasswordForm(dialog_form); call before the return has been
pulled out to the callsite. The |handler| argument was dropped.

 }
 
+void ShowLoginPrompt(const GURL& request_url,
+                     net::AuthChallengeInfo* auth_info,
+                     LoginHandler* handler) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  WebContents* parent_contents = handler->GetWebContentsForLogin();
+  if (!parent_contents)
+    return;
+  prerender::PrerenderContents* prerender_contents =
+      prerender::PrerenderContents::FromWebContents(parent_contents);
+  if (prerender_contents) {
+    prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
+    return;
+  }
+
+  // The realm is controlled by the remote server, so there is no reason
+  // to believe it is of a reasonable length.
+  base::string16 elided_realm;
+  gfx::ElideString(base::UTF8ToUTF16(auth_info->realm), 120, &elided_realm);
+
+  std::string languages;
+  content::WebContents* web_contents = handler->GetWebContentsForLogin();
+  if (web_contents) {
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents->GetBrowserContext());
+    if (profile)
+      languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
+  }
+
+  base::string16 authority =
+      url_formatter::FormatUrlForSecurityDisplay(request_url, languages);
+  base::string16 explanation =
+      elided_realm.empty()
+          ? l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_DESCRIPTION_NO_REALM,
+                                       authority)
+          : l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_DESCRIPTION, authority,
+                                       elided_realm);
+
+  password_manager::PasswordManager* password_manager =
+      handler->GetPasswordManagerForLogin();
+
+  if (!password_manager) {
+#if defined(ENABLE_EXTENSIONS)
+    // A WebContents in a <webview> (a GuestView type) does not have a password
+    // manager, but still needs to be able to show login prompts.
+    if (guest_view::GuestViewBase::FromWebContents(parent_contents)) {
+      handler->BuildViewWithoutPasswordManager(explanation);
+      return;
+    }
+#endif
+    handler->CancelAuth();
+    return;
+  }
+
+  if (password_manager && password_manager->client()->IsLoggingActive()) {
+    password_manager::BrowserSavePasswordProgressLogger logger(
+        password_manager->client());
+    logger.LogMessage(
+        autofill::SavePasswordProgressLogger::STRING_SHOW_LOGIN_PROMPT_METHOD);
+  }
+
+  PasswordForm observed_form(
+      MakeInputForPasswordManager(request_url, auth_info));
+  handler->BuildViewWithPasswordManager(explanation, password_manager,
+                                        observed_form);
+}
+
+}  // namespace
+
 // ----------------------------------------------------------------------------
 // LoginHandler
 
 LoginHandler::LoginModelData::LoginModelData(
     password_manager::LoginModel* login_model,
     const autofill::PasswordForm& observed_form)
     : model(login_model), form(observed_form) {
   DCHECK(model);
 }
 
 LoginHandler::LoginHandler(net::AuthChallengeInfo* auth_info,
                            net::URLRequest* request)
     : handled_auth_(false),
       auth_info_(auth_info),
       request_(request),
       http_network_session_(
           request_->context()->http_transaction_factory()->GetSession()),
       password_manager_(NULL),
       login_model_(NULL) {
   // This constructor is called on the I/O thread, so we cannot load the nib
-  // here. BuildView() will be invoked on the UI thread later, so wait with
+  // here. BuildViewImpl() will be invoked on the UI thread later, so wait with
   // loading the nib until then.
   DCHECK(request_) << "LoginHandler constructed with NULL request";
   DCHECK(auth_info_.get()) << "LoginHandler constructed with NULL auth info";
 
   AddRef();  // matched by LoginHandler::ReleaseSoon().
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&LoginHandler::AddObservers, this));
 
   if (!ResourceRequestInfo::ForRequest(request_)->GetAssociatedRenderFrame(
           &render_process_host_id_,  &render_frame_id_)) {
     NOTREACHED();
   }
 }
 
 void LoginHandler::OnRequestCancelled() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) <<
       "Why is OnRequestCancelled called from the UI thread?";
 
   // Reference is no longer valid.
   request_ = NULL;
 
   // Give up on auth if the request was cancelled.
   CancelAuth();
 }
 
+void LoginHandler::BuildViewWithPasswordManager(
+    const base::string16& explanation,
+    password_manager::PasswordManager* password_manager,
+    const autofill::PasswordForm& observed_form) {
+  SetPasswordManager(password_manager);
+  SetPasswordForm(observed_form);

[davidben, 2015/10/08 15:33:51]

These two methods no longer need to be public and can just be replaced by
setting password_form_ and password_manager_ I think.

[vabr (Chromium), 2015/10/08 15:56:40]

Good call!
Done.

+  LoginHandler::LoginModelData model_data(password_manager, observed_form);
+  BuildViewImpl(explanation, &model_data);
+}
+
+void LoginHandler::BuildViewWithoutPasswordManager(
+    const base::string16& explanation) {
+  BuildViewImpl(explanation, nullptr);
+}
+
 void LoginHandler::SetPasswordForm(const autofill::PasswordForm& form) {
   password_form_ = form;
 }
 
 void LoginHandler::SetPasswordManager(
     password_manager::PasswordManager* password_manager) {
   password_manager_ = password_manager;
 }
 
 WebContents* LoginHandler::GetWebContentsForLogin() const {
@@ skipping 128 matching lines @@
   base::AutoLock lock(handled_auth_lock_);
   bool was_handled = handled_auth_;
   return was_handled;
 }
 
 LoginHandler::~LoginHandler() {
   ResetModel();
 }
 
 void LoginHandler::SetModel(LoginModelData model_data) {
-  if (login_model_)
-    login_model_->RemoveObserver(this);
+  ResetModel();
   login_model_ = model_data.model;
   login_model_->AddObserverAndDeliverCredentials(this, model_data.form);
 }
 
 void LoginHandler::ResetModel() {
   if (login_model_)
     login_model_->RemoveObserver(this);
   login_model_ = nullptr;
 }
 
@@ skipping 134 matching lines @@
   if (!requesting_contents)
     return;
   // If a (blank) login interstitial was displayed, proceed so that the
   // navigation is committed.
   content::InterstitialPage* interstitial_page =
       requesting_contents->GetInterstitialPage();
   if (interstitial_page)
     interstitial_page->Proceed();
 }
 
-// Helper to create a PasswordForm for PasswordManager to start looking for
-// saved credentials.
-PasswordForm MakeInputForPasswordManager(const GURL& request_url,
-                                         net::AuthChallengeInfo* auth_info,
-                                         LoginHandler* handler) {
-  PasswordForm dialog_form;
-  if (base::LowerCaseEqualsASCII(auth_info->scheme, "basic")) {
-    dialog_form.scheme = PasswordForm::SCHEME_BASIC;
-  } else if (base::LowerCaseEqualsASCII(auth_info->scheme, "digest")) {
-    dialog_form.scheme = PasswordForm::SCHEME_DIGEST;
-  } else {
-    dialog_form.scheme = PasswordForm::SCHEME_OTHER;
-  }
-  std::string host_and_port(auth_info->challenger.ToString());
-  if (auth_info->is_proxy) {
-    std::string origin = host_and_port;
-    // We don't expect this to already start with http:// or https://.
-    DCHECK(origin.find("http://") != 0 && origin.find("https://") != 0);
-    origin = std::string("http://") + origin;
-    dialog_form.origin = GURL(origin);
-  } else if (!auth_info->challenger.Equals(
-      net::HostPortPair::FromURL(request_url))) {
-    dialog_form.origin = GURL();
-    NOTREACHED();  // crbug.com/32718
-  } else {
-    dialog_form.origin = GURL(request_url.scheme() + "://" + host_and_port);
-  }
-  dialog_form.signon_realm = GetSignonRealm(dialog_form.origin, *auth_info);
-  // Set the password form for the handler (by copy).
-  handler->SetPasswordForm(dialog_form);
-  return dialog_form;
-}
-
-void ShowLoginPrompt(const GURL& request_url,
-                     net::AuthChallengeInfo* auth_info,
-                     LoginHandler* handler) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  WebContents* parent_contents = handler->GetWebContentsForLogin();
-  if (!parent_contents)
-    return;
-  prerender::PrerenderContents* prerender_contents =
-      prerender::PrerenderContents::FromWebContents(parent_contents);
-  if (prerender_contents) {
-    prerender_contents->Destroy(prerender::FINAL_STATUS_AUTH_NEEDED);
-    return;
-  }
-
-  // The realm is controlled by the remote server, so there is no reason
-  // to believe it is of a reasonable length.
-  base::string16 elided_realm;
-  gfx::ElideString(base::UTF8ToUTF16(auth_info->realm), 120, &elided_realm);
-
-  std::string languages;
-  content::WebContents* web_contents = handler->GetWebContentsForLogin();
-  if (web_contents) {
-    Profile* profile =
-        Profile::FromBrowserContext(web_contents->GetBrowserContext());
-    if (profile)
-      languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
-  }
-
-  base::string16 authority =
-      url_formatter::FormatUrlForSecurityDisplay(request_url, languages);
-  base::string16 explanation =
-      elided_realm.empty()
-          ? l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_DESCRIPTION_NO_REALM,
-                                       authority)
-          : l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_DESCRIPTION, authority,
-                                       elided_realm);
-
-  password_manager::PasswordManager* password_manager =
-      handler->GetPasswordManagerForLogin();
-
-  if (!password_manager) {
-#if defined(ENABLE_EXTENSIONS)
-    // A WebContents in a <webview> (a GuestView type) does not have a password
-    // manager, but still needs to be able to show login prompts.
-    if (guest_view::GuestViewBase::FromWebContents(parent_contents)) {
-      handler->BuildView(explanation, nullptr);
-      return;
-    }
-#endif
-    handler->CancelAuth();
-    return;
-  }
-
-  if (password_manager && password_manager->client()->IsLoggingActive()) {
-    password_manager::BrowserSavePasswordProgressLogger logger(
-        password_manager->client());
-    logger.LogMessage(
-        autofill::SavePasswordProgressLogger::STRING_SHOW_LOGIN_PROMPT_METHOD);
-  }
-
-  handler->SetPasswordManager(password_manager);
-
-  PasswordForm observed_form(
-      MakeInputForPasswordManager(request_url, auth_info, handler));
-  LoginHandler::LoginModelData model_data(password_manager, observed_form);
-  handler->BuildView(explanation, &model_data);
-}
-
 // This callback is run on the UI thread and creates a constrained window with
 // a LoginView to prompt the user. If the prompt is triggered because of
 // a cross origin navigation in the main frame, a blank interstitial is first
 // created which in turn creates the LoginView. Otherwise, a LoginView is
 // directly in this callback. In both cases, the response will be sent to
 // LoginHandler, which then routes it to the net::URLRequest on the I/O thread.
 void LoginDialogCallback(const GURL& request_url,
                          net::AuthChallengeInfo* auth_info,
                          LoginHandler* handler,
                          bool is_main_frame) {
@@ skipping 51 matching lines @@
                                 net::URLRequest* request) {
   bool is_main_frame = (request->load_flags() & net::LOAD_MAIN_FRAME) != 0;
   LoginHandler* handler = LoginHandler::Create(auth_info, request);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&LoginDialogCallback, request->url(),
                  make_scoped_refptr(auth_info), make_scoped_refptr(handler),
                  is_main_frame));
   return handler;
 }
+
+// Get the signon_realm under which this auth info should be stored.
+//
+// The format of the signon_realm for proxy auth is:
+//     proxy-host/auth-realm
+// The format of the signon_realm for server auth is:
+//     url-scheme://url-host[:url-port]/auth-realm
+//
+// Be careful when changing this function, since you could make existing
+// saved logins un-retrievable.
+std::string GetSignonRealm(const GURL& url,
+                           const net::AuthChallengeInfo& auth_info) {
+  std::string signon_realm;
+  if (auth_info.is_proxy) {
+    signon_realm = auth_info.challenger.ToString();
+    signon_realm.append("/");
+  } else {
+    // Take scheme, host, and port from the url.
+    signon_realm = url.GetOrigin().spec();
+    // This ends with a "/".
+  }
+  signon_realm.append(auth_info.realm);
+  return signon_realm;
+}