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..f68ad1402afb83bf163177927694e20e26eb5711 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -218,6 +218,165 @@ void LogHTMLForm(SavePasswordProgressLogger* logger,
                       GURL(form.action().utf8()));
 }
 
+bool FillUserNameAndPassword(struct ParametersNeedUpdate* param,

[vabr (Chromium), 2014/09/25 10:25:49]

Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param
(should be the last argument). You can change usernames_usage_ through that one
directly.

[vabr (Chromium), 2014/09/25 10:25:49]

(Just for future: if you passed a struct argument like this, you can drop
"struct".)

[vabr (Chromium), 2014/09/25 10:25:49]

nit: Please also document that the return value indicates whether the username
and password were filled.

(Slightly unrelated note: apparently, the return value of
FillUserNameAndPassword was not used (but we start to use it in this CL). If you
are looking for clean-up CLs in the future, changing return value type of
functions to void if the returned value is always ignored is a good thing to
look for.)

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

[Deepak, 2014/09/25 12:02:26]

I have made OtherPossibleUsernamesUsage enum as public so that we can use that
in the helper functions, as earlier it is private and that can not be accessed
outside class.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+                             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)) {
+            param->usernames_usage = true;
+            username = iter->second[i];
+            password = iter->first.password;
+            break;
+          }
+        }
+        if (!username.empty() && !password.empty())
+          break;
+      }
+    }
+  }
+  if (password.empty())
+    return false;  // No match was found.
+
+  // Input matches the username, fill in required values.

[vabr (Chromium), 2014/09/25 10:25:49]

Why did you leave out the "TODO(kent)" present at this point in the original
code?

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+  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);
+  param->gate_keeper = true;

[vabr (Chromium), 2014/09/25 10:25:49]

There is no need to signal this through |gate_keeper| -- observe that
gate_keeper is true if and only if the whole method returns true.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+
+  password_element->setAutofilled(true);
+  return true;
+}
+
+void GetSuggestions(struct ParametersNeedUpdate* param,

[vabr (Chromium), 2014/09/25 10:25:49]

Please keep the order of the 3 functions as it was -- it made sense to group the
Fill* methods together. Also, keeping the same order makes it easier to diff
your change against the ToT when reviewing it.

[Deepak, 2014/09/25 12:02:26]

We are calling FillUserNameAndPassword() from FillFormOnPasswordRecieved(), so
FillUserNameAndPassword() should be before FillFormOnPasswordRecieved().

+                    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)) {
+        param->usernames_usage = true;
+        suggestions->push_back(iter->second[i]);
+        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
+      }
+    }
+  }
+}
+
+// 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.
+void FillFormOnPasswordRecieved(struct ParametersNeedUpdate* param,
+                                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(param,

[vabr (Chromium), 2014/09/25 10:25:49]

Forward the return value here, to signal out if the gatekeeper should register
the password element.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+                          &username_element,
+                          &password_element,
+                          fill_data,
+                          true /* exact_username_match */,
+                          false /* set_selection */);
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -303,11 +462,18 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
 
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
-  FillUserNameAndPassword(&username,
+  struct ParametersNeedUpdate param;
+  FillUserNameAndPassword(&param,
+                          &username,
                           &password,
                           fill_data,
                           true /* exact_username_match */,
                           false /* set_selection */);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+  if (param.gate_keeper)
+    gatekeeper_.RegisterElement(&password);
+
   return true;
 }
 
@@ -813,8 +979,15 @@ 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) {
+      struct ParametersNeedUpdate param;
+      FillFormOnPasswordRecieved(
+          &param, form_data, username_element, password_element);
+      if (param.usernames_usage)
+        usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+      if (param.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,11 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
 
   std::vector<base::string16> suggestions;
   std::vector<base::string16> realms;
+  struct ParametersNeedUpdate param;
   GetSuggestions(
-      fill_data, user_input.value(), &suggestions, &realms, show_all);
+      &param, fill_data, user_input.value(), &suggestions, &realms, show_all);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
   DCHECK_EQ(suggestions.size(), realms.size());
 
   FormData form;
@@ -920,126 +1060,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 +1083,18 @@ 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,
+  struct ParametersNeedUpdate param;

[tfarina, 2014/09/25 20:00:23]

no 'struct' here as well. This is a C[ism].

+  FillUserNameAndPassword(&param,
+                          &username,
                           &password,
                           fill_data,
                           false /* exact_username_match */,
                           true /* set_selection */);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+  if (param.gate_keeper)
+    gatekeeper_.RegisterElement(&password);
+
 #endif
 }