Refactor PasswordAutofillAgent: methods to functions.

components/autofill/content/renderer/password_autofill_agent.cc

Index: components/autofill/content/renderer/password_autofill_agent.cc
diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
index a4720aac19665b6e35defecf4fae38e7aa9e7432..27d502486e2c9e77f45e8f7d284d99223fc94db3 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -218,6 +218,171 @@ void LogHTMLForm(SavePasswordProgressLogger* logger,
                       GURL(form.action().utf8()));
 }
 
+void GetSuggestions(

[vabr (Chromium), 2014/09/26 10:06:48]

nit: Please provide a short comment describing what this function does, the
meaning of arguments, and the (new) return value.

[vabr (Chromium), 2014/09/26 10:06:48]

I suggest removing the |usernames_usage| and signalling the needed change
through the return value instead (see my comment in the header file).

+    const PasswordFormFillData& fill_data,
+    const base::string16& input,
+    std::vector<base::string16>* suggestions,
+    std::vector<base::string16>* realms,
+    bool show_all,
+    PasswordAutofillAgent::OtherPossibleUsernamesUsage* usernames_usage) {
+  if (show_all ||
+      StartsWith(fill_data.basic_data.fields[0].value, input, false)) {
+    suggestions->push_back(fill_data.basic_data.fields[0].value);
+    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
+  }
+
+  for (PasswordFormFillData::LoginCollection::const_iterator iter =
+           fill_data.additional_logins.begin();
+       iter != fill_data.additional_logins.end();
+       ++iter) {
+    if (show_all || StartsWith(iter->first, input, false)) {
+      suggestions->push_back(iter->first);
+      realms->push_back(base::UTF8ToUTF16(iter->second.realm));
+    }
+  }
+
+  for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
+           fill_data.other_possible_usernames.begin();
+       iter != fill_data.other_possible_usernames.end();
+       ++iter) {
+    for (size_t i = 0; i < iter->second.size(); ++i) {
+      if (show_all || StartsWith(iter->second[i], input, false)) {
+        *usernames_usage = PasswordAutofillAgent::OtherPossibleUsernamesUsage::
+            OTHER_POSSIBLE_USERNAME_SELECTED;

[vabr (Chromium), 2014/09/26 10:06:48]

Please keep the correct value. SHOWN, not SELECTED!

+        suggestions->push_back(iter->second[i]);
+        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
+      }
+    }
+  }
+}
+
+bool FillUserNameAndPassword(

[vabr (Chromium), 2014/09/26 10:06:48]

I suggest the following changes to the signature of this function, and of
FillFormOnPasswordRecieved as well:

(1) Remove |usernames_usage|, use the return value to signal the need for
updating that (similarly to what I suggest for GetSuggestions).

(2) Because the return value can no longer be used for signalling the gatekeeper
registration, and also to avoid the need to call the gatekeeper on each
callsite, pass a callback argument |password_registration| of type
base::Callback<  void(blink::WebInputElement*)>. Mention in the documentation
commend for both functions that |password_registration| should be the gatekeeper
registration call for the autofilled password field, if any.

[vabr (Chromium), 2014/09/26 10:06:48]

nit: Please also here provide a short comment describing what this function
does, the meaning of arguments, and the return value.

+    blink::WebInputElement* username_element,
+    blink::WebInputElement* password_element,
+    const PasswordFormFillData& fill_data,
+    bool exact_username_match,
+    bool set_selection,
+    PasswordAutofillAgent::OtherPossibleUsernamesUsage* usernames_usage) {
+  base::string16 current_username = username_element->value();
+  // username and password will contain the match found if any.
+  base::string16 username;
+  base::string16 password;
+
+  // Don't fill username if password can't be set.
+  if (!IsElementAutocompletable(*password_element))
+    return false;
+
+  // Look for any suitable matches to current field text.
+  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
+                       current_username,
+                       exact_username_match)) {
+    username = fill_data.basic_data.fields[0].value;
+    password = fill_data.basic_data.fields[1].value;
+  } else {
+    // Scan additional logins for a match.
+    PasswordFormFillData::LoginCollection::const_iterator iter;
+    for (iter = fill_data.additional_logins.begin();
+         iter != fill_data.additional_logins.end();
+         ++iter) {
+      if (DoUsernamesMatch(
+              iter->first, current_username, exact_username_match)) {
+        username = iter->first;
+        password = iter->second.password;
+        break;
+      }
+    }
+
+    // Check possible usernames.
+    if (username.empty() && password.empty()) {
+      for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
+               fill_data.other_possible_usernames.begin();
+           iter != fill_data.other_possible_usernames.end();
+           ++iter) {
+        for (size_t i = 0; i < iter->second.size(); ++i) {
+          if (DoUsernamesMatch(
+                  iter->second[i], current_username, exact_username_match)) {
+            *usernames_usage = PasswordAutofillAgent::
+                OtherPossibleUsernamesUsage::OTHER_POSSIBLE_USERNAME_SELECTED;
+            username = iter->second[i];
+            password = iter->first.password;
+            break;
+          }
+        }
+        if (!username.empty() && !password.empty())
+          break;
+      }
+    }
+  }
+  if (password.empty())
+    return false;  // No match was found.
+
+  // TODO(tkent): Check maxlength and pattern for both username and password
+  // fields.
+
+  // Input matches the username, fill in required values.
+  if (IsElementAutocompletable(*username_element)) {
+    username_element->setValue(username, true);
+    username_element->setAutofilled(true);
+
+    if (set_selection) {
+      username_element->setSelectionRange(current_username.length(),
+                                          username.length());
+    }
+  } else if (current_username != username) {
+    // If the username can't be filled and it doesn't match a saved password
+    // as is, don't autofill a password.
+    return false;
+  }
+
+  // Wait to fill in the password until a user gesture occurs. This is to make
+  // sure that we do not fill in the DOM with a password until we believe the
+  // user is intentionally interacting with the page.
+  password_element->setSuggestedValue(password);
+  password_element->setAutofilled(true);
+  return true;
+}
+
+// Attempts to fill |username_element| and |password_element| with the
+// |fill_data|.  Will use the data corresponding to the preferred username,
+// unless the |username_element| already has a value set.  In that case,
+// attempts to fill the password matching the already filled username, if
+// such a password exists.

[vabr (Chromium), 2014/09/26 10:06:48]

nit: Please describe the meaning of the (new) return value.

+bool FillFormOnPasswordRecieved(
+    const PasswordFormFillData& fill_data,
+    blink::WebInputElement username_element,
+    blink::WebInputElement password_element,
+    PasswordAutofillAgent::OtherPossibleUsernamesUsage* usernames_usage) {
+  // Do not fill if the password field is in an iframe.
+  DCHECK(password_element.document().frame());
+  if (password_element.document().frame()->parent())
+    return false;
+
+  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
+      !username_element.form().autoComplete())
+    return false;
+
+  // If we can't modify the password, don't try to set the username
+  if (!IsElementAutocompletable(password_element))
+    return false;
+
+  // Try to set the username to the preferred name, but only if the field
+  // can be set and isn't prefilled.
+  if (IsElementAutocompletable(username_element) &&
+      username_element.value().isEmpty()) {
+    // TODO(tkent): Check maxlength and pattern.
+    username_element.setValue(fill_data.basic_data.fields[0].value, true);
+  }
+
+  // Fill if we have an exact match for the username. Note that this sets
+  // username to autofilled.
+  return FillUserNameAndPassword(&username_element,
+                                 &password_element,
+                                 fill_data,
+                                 true /* exact_username_match */,
+                                 false /* set_selection */,
+                                 usernames_usage);
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -303,11 +468,15 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
 
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
-  FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
-                          true /* exact_username_match */,
-                          false /* set_selection */);
+  bool gate_keeper = FillUserNameAndPassword(&username,

[vabr (Chromium), 2014/09/26 10:06:48]

(The following is made obsolete by my other comments, but just for learning
purposes, this would be my comment if the code stayed this way:

nit: Calling this bool |gate_keeper| in the presence of the gatekeeper object
named |gatekeeper_| is rather confusing. I suggest naming the bool after the
meaning of the return value, e.g., |password_filled|.)

+                                             &password,
+                                             fill_data,
+                                             true /* exact_username_match */,
+                                             false /* set_selection */,
+                                             &usernames_usage_);
+  if (gate_keeper)
+    gatekeeper_.RegisterElement(&password);
+
   return true;
 }
 
@@ -813,8 +982,12 @@ void PasswordAutofillAgent::OnFillPasswordForm(
 
     // If wait_for_username is true, we don't want to initially fill the form
     // until the user types in a valid username.
-    if (!form_data.wait_for_username)
-      FillFormOnPasswordRecieved(form_data, username_element, password_element);
+    if (!form_data.wait_for_username) {
+      bool gate_keeper = FillFormOnPasswordRecieved(
+          form_data, username_element, password_element, &usernames_usage_);
+      if (gate_keeper)
+        gatekeeper_.RegisterElement(&password_element);
+    }
 
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
@@ -848,42 +1021,6 @@ PasswordAutofillAgent::PasswordInfo::PasswordInfo()
     : backspace_pressed_last(false), password_was_edited_last(false) {
 }
 
-void PasswordAutofillAgent::GetSuggestions(
-    const PasswordFormFillData& fill_data,
-    const base::string16& input,
-    std::vector<base::string16>* suggestions,
-    std::vector<base::string16>* realms,
-    bool show_all) {
-  if (show_all ||
-      StartsWith(fill_data.basic_data.fields[0].value, input, false)) {
-    suggestions->push_back(fill_data.basic_data.fields[0].value);
-    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
-  }
-
-  for (PasswordFormFillData::LoginCollection::const_iterator iter =
-           fill_data.additional_logins.begin();
-       iter != fill_data.additional_logins.end();
-       ++iter) {
-    if (show_all || StartsWith(iter->first, input, false)) {
-      suggestions->push_back(iter->first);
-      realms->push_back(base::UTF8ToUTF16(iter->second.realm));
-    }
-  }
-
-  for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
-           fill_data.other_possible_usernames.begin();
-       iter != fill_data.other_possible_usernames.end();
-       ++iter) {
-    for (size_t i = 0; i < iter->second.size(); ++i) {
-      if (show_all || StartsWith(iter->second[i], input, false)) {
-        usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
-        suggestions->push_back(iter->second[i]);
-        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
-      }
-    }
-  }
-}
-
 bool PasswordAutofillAgent::ShowSuggestionPopup(
     const PasswordFormFillData& fill_data,
     const blink::WebInputElement& user_input,
@@ -898,8 +1035,12 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
 
   std::vector<base::string16> suggestions;
   std::vector<base::string16> realms;
-  GetSuggestions(
-      fill_data, user_input.value(), &suggestions, &realms, show_all);
+  GetSuggestions(fill_data,
+                 user_input.value(),
+                 &suggestions,
+                 &realms,
+                 show_all,
+                 &usernames_usage_);
   DCHECK_EQ(suggestions.size(), realms.size());
 
   FormData form;
@@ -920,126 +1061,6 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
   return !suggestions.empty();
 }
 
-void PasswordAutofillAgent::FillFormOnPasswordRecieved(
-    const PasswordFormFillData& fill_data,
-    blink::WebInputElement username_element,
-    blink::WebInputElement password_element) {
-  // Do not fill if the password field is in an iframe.
-  DCHECK(password_element.document().frame());
-  if (password_element.document().frame()->parent())
-    return;
-
-  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      !username_element.form().autoComplete())
-    return;
-
-  // If we can't modify the password, don't try to set the username
-  if (!IsElementAutocompletable(password_element))
-    return;
-
-  // Try to set the username to the preferred name, but only if the field
-  // can be set and isn't prefilled.
-  if (IsElementAutocompletable(username_element) &&
-      username_element.value().isEmpty()) {
-    // TODO(tkent): Check maxlength and pattern.
-    username_element.setValue(fill_data.basic_data.fields[0].value, true);
-  }
-
-  // Fill if we have an exact match for the username. Note that this sets
-  // username to autofilled.
-  FillUserNameAndPassword(&username_element,
-                          &password_element,
-                          fill_data,
-                          true /* exact_username_match */,
-                          false /* set_selection */);
-}
-
-bool PasswordAutofillAgent::FillUserNameAndPassword(
-    blink::WebInputElement* username_element,
-    blink::WebInputElement* password_element,
-    const PasswordFormFillData& fill_data,
-    bool exact_username_match,
-    bool set_selection) {
-  base::string16 current_username = username_element->value();
-  // username and password will contain the match found if any.
-  base::string16 username;
-  base::string16 password;
-
-  // Don't fill username if password can't be set.
-  if (!IsElementAutocompletable(*password_element))
-    return false;
-
-  // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
-                       current_username,
-                       exact_username_match)) {
-    username = fill_data.basic_data.fields[0].value;
-    password = fill_data.basic_data.fields[1].value;
-  } else {
-    // Scan additional logins for a match.
-    PasswordFormFillData::LoginCollection::const_iterator iter;
-    for (iter = fill_data.additional_logins.begin();
-         iter != fill_data.additional_logins.end();
-         ++iter) {
-      if (DoUsernamesMatch(
-              iter->first, current_username, exact_username_match)) {
-        username = iter->first;
-        password = iter->second.password;
-        break;
-      }
-    }
-
-    // Check possible usernames.
-    if (username.empty() && password.empty()) {
-      for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
-               fill_data.other_possible_usernames.begin();
-           iter != fill_data.other_possible_usernames.end();
-           ++iter) {
-        for (size_t i = 0; i < iter->second.size(); ++i) {
-          if (DoUsernamesMatch(
-                  iter->second[i], current_username, exact_username_match)) {
-            usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
-            username = iter->second[i];
-            password = iter->first.password;
-            break;
-          }
-        }
-        if (!username.empty() && !password.empty())
-          break;
-      }
-    }
-  }
-  if (password.empty())
-    return false;  // No match was found.
-
-  // TODO(tkent): Check maxlength and pattern for both username and password
-  // fields.
-
-  // Input matches the username, fill in required values.
-  if (IsElementAutocompletable(*username_element)) {
-    username_element->setValue(username, true);
-    username_element->setAutofilled(true);
-
-    if (set_selection) {
-      username_element->setSelectionRange(current_username.length(),
-                                          username.length());
-    }
-  } else if (current_username != username) {
-    // If the username can't be filled and it doesn't match a saved password
-    // as is, don't autofill a password.
-    return false;
-  }
-
-  // Wait to fill in the password until a user gesture occurs. This is to make
-  // sure that we do not fill in the DOM with a password until we believe the
-  // user is intentionally interacting with the page.
-  password_element->setSuggestedValue(password);
-  gatekeeper_.RegisterElement(password_element);
-
-  password_element->setAutofilled(true);
-  return true;
-}
-
 void PasswordAutofillAgent::PerformInlineAutocomplete(
     const blink::WebInputElement& username_input,
     const blink::WebInputElement& password_input,
@@ -1063,11 +1084,14 @@ void PasswordAutofillAgent::PerformInlineAutocomplete(
 #if !defined(OS_ANDROID)
   // Fill the user and password field with the most relevant match. Android
   // only fills in the fields after the user clicks on the suggestion popup.
-  FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
-                          false /* exact_username_match */,
-                          true /* set_selection */);
+  bool gate_keeper = FillUserNameAndPassword(&username,
+                                             &password,
+                                             fill_data,
+                                             false /* exact_username_match */,
+                                             true /* set_selection */,
+                                             &usernames_usage_);
+  if (gate_keeper)
+    gatekeeper_.RegisterElement(&password);
 #endif
 }