Autofill: Refactoring to support fetching password after a username is selected

components/autofill/core/browser/password_autofill_manager.cc

 // Copyright 2013 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 "base/logging.h"
 #include "components/autofill/core/browser/autofill_driver.h"
 #include "components/autofill/core/browser/password_autofill_manager.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 
 namespace autofill {
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillManager, public:
 
 PasswordAutofillManager::PasswordAutofillManager(
-    AutofillDriver* autofill_driver) : autofill_driver_(autofill_driver) {
+    PasswordManagerClient* password_manager_client,
+    AutofillDriver* autofill_driver)
+    : password_manager_client_(password_manager_client),
+      autofill_driver_(autofill_driver) {
+  DCHECK(password_manager_client);
   DCHECK(autofill_driver);
 }
 
 PasswordAutofillManager::~PasswordAutofillManager() {
 }
 
-bool PasswordAutofillManager::DidAcceptAutofillSuggestion(
+void PasswordAutofillManager::DidAcceptAutofillSuggestion(

[Garrett Casto, 2014/03/06 00:42:05]

You should change the function name if this is meant to be declarative.
"AcceptAutofillSuggestion" seems fine.

     const FormFieldData& field,
     const base::string16& username) {
-  PasswordFormFillData password;
-  if (!FindLoginInfo(field, &password))
-    return false;
-
-  if (WillFillUserNameAndPassword(username, password)) {
-    autofill_driver_->RendererShouldAcceptPasswordAutofillSuggestion(username);
-    return true;
+  PasswordFormFillData fill_data;
+  base::string16 password;
+  if (FindLoginInfo(field, &fill_data) &&
+      GetPasswordForUsername(username, fill_data, &password)) {
+    autofill_driver_->RendererShouldAcceptPasswordAutofillSuggestion(
+        username, password);
+    return;
   }
-
-  return false;
+  NOTREACHED();
 }
 
 void PasswordAutofillManager::AddPasswordFormMapping(
     const FormFieldData& username_element,
     const PasswordFormFillData& password) {
   login_to_password_info_[username_element] = password;
 }
 
 void PasswordAutofillManager::Reset() {
   login_to_password_info_.clear();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillManager, private:
 
-bool PasswordAutofillManager::WillFillUserNameAndPassword(
+bool PasswordAutofillManager::GetPasswordForUsername(
     const base::string16& current_username,
-    const PasswordFormFillData& fill_data) {
+    const PasswordFormFillData& fill_data,
+    base::string16* password) {
+  // TODO(dubroy): When password access requires some kind of authentication

[Garrett Casto, 2014/03/06 00:42:05]

This still has the same dependence problems that I was thinking about before.
You need an object from the PasswordManager component in order to construct
something in the Autofill component. The alternative that I was thinking of was
to have this call go through AutofillManagerDelegate, so
TabAutofillManagerDelegate would get the PasswordManager from
ChromePasswordManagerClient and load the password from there. The indirection is
a little higher, but it keep the two components more separated.

[Patrick Dubroy, 2014/03/10 14:29:57]

Yeah, that might be slightly better, though it still means a direct dependency
from the autofill component to the password manager component.

Alternatively, it looks like it should be possible to just move all the
password-related IPCs into the password manager component. What do you think
about doing that?

[Ilya Sherman, 2014/03/10 21:44:07]

Yes, if we can do that, definitely let's! :)

+  // (e.g. Keychain access on Mac OS), use |password_manager_client_| here to
+  // fetch the actual password. See crbug.com/178358 for more context.
+
   // Look for any suitable matches to current field text.
-  if (fill_data.basic_data.fields[0].value == current_username)
+  if (fill_data.basic_data.fields[0].value == current_username) {
+    *password = fill_data.basic_data.fields[1].value;
     return true;
+  }
 
   // Scan additional logins for a match.
    for (PasswordFormFillData::LoginCollection::const_iterator iter =
            fill_data.additional_logins.begin();
        iter != fill_data.additional_logins.end(); ++iter) {
-    if (iter->first == current_username)
-      return true;
+     if (iter->first == current_username) {
+       *password = iter->second.password;
+       return true;
+     }
   }
 
   for (PasswordFormFillData::UsernamesCollection::const_iterator usernames_iter
            = fill_data.other_possible_usernames.begin();
        usernames_iter != fill_data.other_possible_usernames.end();
        ++usernames_iter) {
     for (size_t i = 0; i < usernames_iter->second.size(); ++i) {
-      if (usernames_iter->second[i] == current_username)
+      if (usernames_iter->second[i] == current_username) {
+        *password = usernames_iter->first.password;
         return true;
+      }
     }
   }
 
   return false;
 }
 
 bool PasswordAutofillManager::FindLoginInfo(
     const FormFieldData& field,
     PasswordFormFillData* found_password) {
   LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(field);
   if (iter == login_to_password_info_.end())
     return false;
 
   *found_password = iter->second;
   return true;
 }
 
 }  // namespace autofill