Do not involve PasswordManagerDriver in filling HTTP-auth forms; also check realm

chrome/browser/ui/login/login_prompt.cc

Index: chrome/browser/ui/login/login_prompt.cc
diff --git a/chrome/browser/ui/login/login_prompt.cc b/chrome/browser/ui/login/login_prompt.cc
index 8051d83138dad0e8f008a3f5163e32310c236cce..6ee6149a5426eee6081416010753ca53800e7154 100644
--- a/chrome/browser/ui/login/login_prompt.cc
+++ b/chrome/browser/ui/login/login_prompt.cc
@@ -21,7 +21,6 @@
 #include "chrome/browser/ui/login/login_interstitial_delegate.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
-#include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
 #include "components/password_manager/core/browser/password_manager.h"
 #include "components/url_formatter/elide_url.h"
@@ -90,6 +89,13 @@ std::string GetSignonRealm(const GURL& url,
 // ----------------------------------------------------------------------------
 // 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),
@@ -100,8 +106,8 @@ LoginHandler::LoginHandler(net::AuthChallengeInfo* auth_info,
       password_manager_(NULL),
       login_model_(NULL) {
   // This constructor is called on the I/O thread, so we cannot load the nib
-  // here. BuildViewForPasswordManager() will be invoked on the UI thread
-  // later, so wait with loading the nib until then.
+  // here. BuildView() 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";
 
@@ -145,12 +151,10 @@ WebContents* LoginHandler::GetWebContentsForLogin() const {
   return WebContents::FromRenderFrameHost(rfh);
 }
 
-password_manager::ContentPasswordManagerDriver*
-LoginHandler::GetPasswordManagerDriverForLogin() {
-  content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
-      render_process_host_id_, render_frame_id_);
-  return password_manager::ContentPasswordManagerDriver::GetForRenderFrameHost(
-      rfh);
+password_manager::PasswordManager* LoginHandler::GetPasswordManagerForLogin() {
+  ChromePasswordManagerClient* client =
+      ChromePasswordManagerClient::FromWebContents(GetWebContentsForLogin());
+  return client ? client->GetPasswordManager() : nullptr;
 }
 
 void LoginHandler::SetAuth(const base::string16& username,
@@ -274,15 +278,20 @@ bool LoginHandler::WasAuthHandled() const {
 }
 
 LoginHandler::~LoginHandler() {
-  SetModel(NULL);
+  ResetModel();
 }
 
-void LoginHandler::SetModel(password_manager::LoginModel* model) {
+void LoginHandler::SetModel(LoginModelData model_data) {
   if (login_model_)
     login_model_->RemoveObserver(this);

[Peter Kasting, 2015/10/07 22:27:07]

Nit: I wonder if this should be a call to ResetModel() instead?

It doesn't matter much for this implementation; the question would be, let's say
someday we want resetting the model to include some more complicated logic or
series of actions.  In that world, would we need to add that code to both these
functions?  If yes, then this should probably just call that.

[vabr (Chromium), 2015/10/08 10:24:12]

I was considering this, and did not find compelling reasons to do so. But you
found one, and I agree, so will switch to ResetModel here.
Done.

-  login_model_ = model;
+  login_model_ = model_data.model;
+  login_model_->AddObserverAndDeliverCredentials(this, model_data.form);
+}
+
+void LoginHandler::ResetModel() {
   if (login_model_)
-    login_model_->AddObserver(this);
+    login_model_->RemoveObserver(this);
+  login_model_ = nullptr;
 }
 
 void LoginHandler::NotifyAuthNeeded() {
@@ -429,13 +438,11 @@ void LoginHandler::CloseContentsDeferred() {
     interstitial_page->Proceed();
 }
 
-// Helper to create a PasswordForm and stuff it into a vector as input
-// for PasswordManager::PasswordFormsParsed, the hook into PasswordManager.
-void MakeInputForPasswordManager(
-    const GURL& request_url,
-    net::AuthChallengeInfo* auth_info,
-    LoginHandler* handler,
-    std::vector<PasswordForm>* password_manager_input) {
+// 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) {

[davidben, 2015/10/07 23:13:54]

(Existing unrelated issue, but this and the two functions below probably want to
be in some anonymous namespace since they're not exported outside this header.)

[vabr (Chromium), 2015/10/08 10:24:12]

Done.

   PasswordForm dialog_form;
   if (base::LowerCaseEqualsASCII(auth_info->scheme, "basic")) {
     dialog_form.scheme = PasswordForm::SCHEME_BASIC;
@@ -459,9 +466,9 @@ void MakeInputForPasswordManager(
     dialog_form.origin = GURL(request_url.scheme() + "://" + host_and_port);
   }
   dialog_form.signon_realm = GetSignonRealm(dialog_form.origin, *auth_info);
-  password_manager_input->push_back(dialog_form);
   // Set the password form for the handler (by copy).
   handler->SetPasswordForm(dialog_form);

[davidben, 2015/10/07 23:13:54]

(Existing unrelated issue, but it's pretty weird to have this function bother
return a PasswordForm and also stash it somewhere. Could remove the LoginHandler
parameter so this purely creates a PasswordForm and then move the
SetPasswordForm call to the caller.)

[vabr (Chromium), 2015/10/08 10:24:12]

Done.

+  return dialog_form;
 }
 
 void ShowLoginPrompt(const GURL& request_url,
@@ -478,9 +485,6 @@ void ShowLoginPrompt(const GURL& request_url,
     return;
   }
 
-  password_manager::ContentPasswordManagerDriver* driver =
-      handler->GetPasswordManagerDriverForLogin();
-
   // 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;
@@ -504,12 +508,15 @@ void ShowLoginPrompt(const GURL& request_url,
           : l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_DESCRIPTION, authority,
                                        elided_realm);
 
-  if (!driver) {
+  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->BuildViewForPasswordManager(nullptr, explanation);
+      handler->BuildView(explanation, nullptr);
       return;
     }
 #endif
@@ -517,8 +524,6 @@ void ShowLoginPrompt(const GURL& request_url,
     return;
   }
 
-  password_manager::PasswordManager* password_manager =
-      driver->GetPasswordManager();
   if (password_manager && password_manager->client()->IsLoggingActive()) {
     password_manager::BrowserSavePasswordProgressLogger logger(
         password_manager->client());
@@ -526,14 +531,12 @@ void ShowLoginPrompt(const GURL& request_url,
         autofill::SavePasswordProgressLogger::STRING_SHOW_LOGIN_PROMPT_METHOD);
   }
 
-  // Tell the password manager to look for saved passwords.
-  std::vector<PasswordForm> v;
-  MakeInputForPasswordManager(request_url, auth_info, handler, &v);
-  driver->OnPasswordFormsParsedNoRenderCheck(v);
-  handler->SetPasswordManager(driver->GetPasswordManager());
+  handler->SetPasswordManager(password_manager);
 
-  handler->BuildViewForPasswordManager(driver->GetPasswordManager(),
-                                       explanation);
+  PasswordForm observed_form(
+      MakeInputForPasswordManager(request_url, auth_info, handler));
+  LoginHandler::LoginModelData model_data(password_manager, observed_form);
+  handler->BuildView(explanation, &model_data);

[davidben, 2015/10/07 23:13:54]

(Existing tangentially-related issue, but it's pretty weird to have LoginHandler
receive PasswordManager and PasswordForm in two ways, both in
SetPassword{Form,Manager} and as a parameter to BuildView.)

[vabr (Chromium), 2015/10/08 10:24:12]

Done.

 }
 
 // This callback is run on the UI thread and creates a constrained window with