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 480bfc617eb95ca620779cf045b0eeb3c9a37899..4760c17075af01a7a6a8406e451112d91076cf7b 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -225,6 +225,188 @@ bool FillDataContainsUsername(const PasswordFormFillData& fill_data) {
   return !fill_data.basic_data.fields[0].name.empty();
 }
 
+// This helper function will fill the |suggestions| and |realms| form the

[Garrett Casto, 2014/09/30 00:16:41]

"fill the |suggestions| and |realms| form the fill_data", I think you mean "will
fill |suggestions| and |realms| from |fill_data|".

[Deepak, 2014/09/30 04:07:21]

Done.

[vabr (Chromium), 2014/09/30 08:19:41]

The form->from typo seems to be still present. :)

+// fill_data based on the |input| and  return true when suggestion

[vabr (Chromium), 2014/09/29 15:19:09]

typo: suggestion -> suggestions

[Garrett Casto, 2014/09/30 00:16:40]

Just "|input|" not "the |input|". "Input" is also not very descriptive. Maybe
rename to something like "current_username"? Also break up the sentence at that
point and start a new sentence for "Return true ...".

[Deepak, 2014/09/30 04:07:21]

Done.

[Deepak, 2014/09/30 04:07:21]

Done.

[vabr (Chromium), 2014/09/30 08:19:41]

I believe Garrett meant renaming the argument |input| to |current_username|,
rather than specifying that only in the comment.

Also, "break up a sentence" is not the same as "break a line" -- please follow
the comment of Garrett, and use as much space on comment lines, as is permitted
by the 80 character length limit without breaking words in the middle. (Tip:
write your comments on a single line, ignoring the length limit, and then run
git cl format. It will break the lines in the comments for you.)

+// get filled from usernamescollection else return false.

[vabr (Chromium), 2014/09/29 15:19:09]

nit: Not clear what is meant by the username collection here. Maybe just write
"from |fill_data|" instead?

[Garrett Casto, 2014/09/30 00:16:40]

From |fill_data| isn't specific, all suggestions are coming from fill_data. The
function only returns true when data filled is from
fill_data.other_possible_usernames. You should specify this instead of filled
from PasswordFormFillData::UsernamesCollection, as the type that contains
other_possible_usernames isn't particularly relevant.

[Deepak, 2014/09/30 04:07:21]

I have made this like:
return true when suggestions get filled from other_possible_usernames
of |fill_data| else return false.

This will address both.

[Deepak, 2014/09/30 04:07:21]

Done.

[vabr (Chromium), 2014/09/30 08:19:41]

2 nits:
(1) Replace other_possible_usernames with |fill_data.other_possible_usernames|,
to be precise.
(2) "Return" -> "Returns"  -- to be consistent with the previous sentence which
speaks about the function in 3rd person.

+bool GetSuggestions(const PasswordFormFillData& fill_data,
+                    const base::string16& input,
+                    std::vector<base::string16>* suggestions,
+                    std::vector<base::string16>* realms,
+                    bool show_all) {
+  bool usernames_shown = false;
+  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_shown = true;
+        suggestions->push_back(iter->second[i]);
+        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
+      }
+    }
+  }
+  return usernames_shown;
+}
+
+// This function attempts to set the |username_element]
+// and |password_element| values with the |fill_data|

[Garrett Casto, 2014/09/30 00:16:41]

Line wrapping too early. You should wrap closer to 80 chars (approximately when
the next word will put you over 80 chars).

[Deepak, 2014/09/30 04:07:22]

Acknowledged.

+// returns true when username get selected from possible usernames

[vabr (Chromium), 2014/09/29 15:19:10]

typo: "username get" -> "a username gets"

[Garrett Casto, 2014/09/30 00:16:40]

I think  there is supposed to be a period and "returns" should be capitalized.

[Deepak, 2014/09/30 04:07:21]

Done.

[Deepak, 2014/09/30 04:07:22]

Done.

+// else returns false.it will also register the password element when

[vabr (Chromium), 2014/09/29 15:19:10]

nit: Specify that it will register the password element by passing it to the
|gatekeeper_callback|. Or rename |gatekeeper_callback| to
|registration_callback|.

[vabr (Chromium), 2014/09/29 15:19:10]

nit: "false.it" -> "false. It"

[Deepak, 2014/09/30 04:07:21]

Done.

+// its value set.
+bool FillUserNameAndPassword(
+    blink::WebInputElement* username_element,
+    blink::WebInputElement* password_element,
+    const PasswordFormFillData& fill_data,
+    bool exact_username_match,
+    bool set_selection,
+    base::Callback<void(blink::WebInputElement*)> gatekeeper_callback) {
+  bool username_selected = false;

[Garrett Casto, 2014/09/30 00:16:41]

|username_selected| is too vague. It's not just if any username is selected,
just an other_possible_username.

[Deepak, 2014/09/30 04:07:21]

I have made this "possible_username_selected" to make it distinct that other
possible username is selected.

[vabr (Chromium), 2014/09/30 08:19:41]

I would actually suggest |other_possible_username_selected|, to make it clear it
relates to the |other_possible_usernames| field. It's rather long, but should
not cause formatting issues.

[vabr (Chromium), 2014/09/30 13:54:53]

You seem to have missed this comment as well.

+  // Don't fill username if password can't be set.
+  if (!IsElementAutocompletable(*password_element))
+    return false;
+
+  base::string16 current_username;
+  if (!username_element->isNull()) {
+    current_username = username_element->value();
+  }
+
+  // username and password will contain the match found if any.
+  base::string16 username;
+  base::string16 password;
+
+  // 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)) {
+            username_selected = true;
+            username = iter->second[i];
+            password = iter->first.password;
+            break;
+          }
+        }
+        if (!username.empty() && !password.empty())
+          break;
+      }
+    }
+  }
+  if (password.empty())
+    return username_selected;  // 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 (!username_element->isNull() &&
+      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 username_selected;
+  }
+
+  // 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_callback.Run(password_element);
+
+  password_element->setAutofilled(true);
+  return username_selected;
+}
+
+// Attempts to fill |username_element| and |password_element| with the

[Garrett Casto, 2014/09/30 00:16:40]

As above, be consistent with pronouns. It's more common to refer to parameters
without "the", just "|fill_data|". 

+// |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.gatekeeper_ptr will register the password element.

[vabr (Chromium), 2014/09/29 15:19:09]

nit: gatekeeper_ptr sounds like a typo. Maybe say here the same what you'll say
about the callback for the function above.

[vabr (Chromium), 2014/09/29 15:19:10]

nit: space after a full-stop

[Deepak, 2014/09/30 04:07:21]

Done.

[Deepak, 2014/09/30 04:07:21]

Done.

+// returns true when username is selected else return false.

[Garrett Casto, 2014/09/30 00:16:41]

Username is selected is misleading, it's just other_possible_username. Also
capitalize "Returns" and add a comma after "selected".

[Deepak, 2014/09/30 04:07:21]

Done.

+bool FillFormOnPasswordRecieved(
+    const PasswordFormFillData& fill_data,
+    blink::WebInputElement username_element,
+    blink::WebInputElement password_element,
+    base::Callback<void(blink::WebInputElement*)> gatekeeper_callback) {
+  // Do not fill if the password field is in an iframe.
+  DCHECK(password_element.document().frame());
+  if (password_element.document().frame()->parent())
+    return false;
+
+  bool form_contains_username_field = FillDataContainsUsername(fill_data);
+  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
+      form_contains_username_field && !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 (form_contains_username_field &&
+      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 */,
+                                 gatekeeper_callback);
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -310,11 +492,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 */);
+  if (FillUserNameAndPassword(
+          &username,
+          &password,
+          fill_data,
+          true /* exact_username_match */,
+          false /* set_selection */,
+          base::Bind(&PasswordValueGatekeeper::RegisterElement,
+                     base::Unretained(&gatekeeper_))))
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;

[vabr (Chromium), 2014/09/29 15:19:10]

nit: Please add curly braces around the statement, because the if condition
spans more lines. Also at the modified if-blocks below.

[Deepak, 2014/09/30 04:07:21]

Done.

   return true;
 }
 
@@ -829,8 +1015,14 @@ 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 &&
+        FillFormOnPasswordRecieved(
+            form_data,
+            username_element,
+            password_element,
+            base::Bind(&PasswordValueGatekeeper::RegisterElement,
+                       base::Unretained(&gatekeeper_))))
+      usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
 
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
@@ -867,42 +1059,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,
@@ -917,8 +1073,10 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
 
   std::vector<base::string16> suggestions;
   std::vector<base::string16> realms;
-  GetSuggestions(
-      fill_data, user_input.value(), &suggestions, &realms, show_all);
+  if (GetSuggestions(
+          fill_data, user_input.value(), &suggestions, &realms, show_all))
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
+
   DCHECK_EQ(suggestions.size(), realms.size());
 
   FormData form;
@@ -939,133 +1097,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;
-
-  bool form_contains_username_field = FillDataContainsUsername(fill_data);
-  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      form_contains_username_field && !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 (form_contains_username_field &&
-      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) {
-  // Don't fill username if password can't be set.
-  if (!IsElementAutocompletable(*password_element))
-    return false;
-
-  base::string16 current_username;
-  if (!username_element->isNull()) {
-    current_username = username_element->value();
-  }
-
-  // username and password will contain the match found if any.
-  base::string16 username;
-  base::string16 password;
-
-  // 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 (!username_element->isNull() &&
-      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,
@@ -1089,11 +1120,16 @@ 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 */);
+  if (FillUserNameAndPassword(
+          &username,
+          &password,
+          fill_data,
+          false /* exact_username_match */,
+          true /* set_selection */,
+          base::Bind(&PasswordValueGatekeeper::RegisterElement,
+                     base::Unretained(&gatekeeper_))))
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+
 #endif
 }