[Password manager] Relplace the FormFieldData vector from autofill::FormData with named fields…

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..7639029f70b98f6142725e65b6c557456da7073b 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -67,22 +67,12 @@ typedef std::vector<FormElements*> FormElementsList;
 static bool FindFormInputElements(blink::WebFormElement* fe,
                                   const FormData& data,
                                   FormElements* result) {
-  const bool username_is_present = !data.fields[0].name.empty();
-
-  // Loop through the list of elements we need to find on the form in order to
-  // autofill it. If we don't find any one of them, abort processing this
-  // form; it can't be the right one.
-  // First field is the username, skip it if not present.
-  for (size_t j = (username_is_present ? 0 : 1); j < data.fields.size(); ++j) {
-    blink::WebVector<blink::WebNode> temp_elements;
-    fe->getNamedElements(data.fields[j].name, temp_elements);
-
-    // Match the first input element, if any.
-    // |getNamedElements| may return non-input elements where the names match,
-    // so the results are filtered for input elements.
-    // If more than one match is made, then we have ambiguity (due to misuse
-    // of "name" attribute) so is it considered not found.
-    bool found_input = false;
+  const bool username_is_present = !data.username.name.empty();
+  blink::WebVector<blink::WebNode> temp_elements;
+  bool found_input = false;
+  if (username_is_present) {
+    // Fill the username input field.
+    fe->getNamedElements(data.username.name, temp_elements);
     for (size_t i = 0; i < temp_elements.size(); ++i) {
       if (temp_elements[i].to<blink::WebElement>().hasHTMLTagName("input")) {
         // Check for a non-unique match.
@@ -95,29 +85,53 @@ static bool FindFormInputElements(blink::WebFormElement* fe,
         // text fields.

[Ilya Sherman, 2014/09/30 20:02:32]

It looks like you removed the code that addresses this comment.  Please restore
it, both here and below.

[Ilya Sherman, 2014/09/30 20:02:32]

Also, if there aren't any existing test cases that test this behavior, please
add them.

[Pritam Nikam, 2014/10/13 10:48:00]

Done.

[Pritam Nikam, 2014/10/13 10:48:00]

Test already exists.

         blink::WebInputElement input_element =
             temp_elements[i].to<blink::WebInputElement>();
-        if (input_element.isPasswordField() !=
-            (data.fields[j].form_control_type == "password"))
-          continue;
 
         // This element matched, add it to our temporary result. It's possible
         // there are multiple matches, but for purposes of identifying the form
         // one suffices and if some function needs to deal with multiple
         // matching elements it can get at them through the FormElement*.
         // Note: This assignment adds a reference to the InputElement.
-        result->input_elements[data.fields[j].name] = input_element;
+        result->input_elements[data.username.name] = input_element;
         found_input = true;
       }
     }
+  }
+
+  // Fill the password input field.
+  found_input = false;

[vabr (Chromium), 2014/09/30 14:19:49]

At this point you throw out the previous value of |found_input|. That's a
problem: consider a page with a duplicated input field. In the old code, that
would result in exiting the function and returning false. Here it will happily
continue and potentially return true.

[Pritam Nikam, 2014/10/13 10:48:00]

Done.

+  fe->getNamedElements(data.password.name, temp_elements);
+  for (size_t i = 0; i < temp_elements.size(); ++i) {

[vabr (Chromium), 2014/09/30 14:19:48]

Before you prepare the final version of this CL, please consider de-duplicating
the for-loop, e.g., by making a helper function  in the anonymous namespace for
that.

[Ilya Sherman, 2014/09/30 20:02:32]

I will emphasize this comment by phrasing it more strongly: You should
definitely avoid duplicating this code.  The loop is non-trivial, and the logic
is bound to be updated periodically in the future.  It's therefore important
that the logic only be written once, so that it's not possible for someone to
fix a bug by updating one copy of the logic, but fail to fix a sister bug by not
noticing the second copy.

[Pritam Nikam, 2014/10/13 10:48:00]

Done.

[Pritam Nikam, 2014/10/13 10:48:00]

Done.

+    if (temp_elements[i].to<blink::WebElement>().hasHTMLTagName("input")) {
+      // Check for a non-unique match.
+      if (found_input) {
+        found_input = false;
+        break;
+      }
 
-    // A required element was not found. This is not the right form.
-    // Make sure no input elements from a partially matched form in this
-    // iteration remain in the result set.
-    // Note: clear will remove a reference from each InputElement.
-    if (!found_input) {
-      result->input_elements.clear();
-      return false;
+      // Only fill saved passwords into password fields and usernames into
+      // text fields.
+      blink::WebInputElement input_element =
+          temp_elements[i].to<blink::WebInputElement>();
+
+      // This element matched, add it to our temporary result. It's possible
+      // there are multiple matches, but for purposes of identifying the form
+      // one suffices and if some function needs to deal with multiple
+      // matching elements it can get at them through the FormElement*.
+      // Note: This assignment adds a reference to the InputElement.
+      result->input_elements[data.password.name] = input_element;
+      found_input = true;
     }
   }
+
+  // A required element was not found. This is not the right form.
+  // Make sure no input elements from a partially matched form in this
+  // iteration remain in the result set.
+  // Note: clear will remove a reference from each InputElement.
+  if (!found_input) {
+    result->input_elements.clear();
+    return false;
+  }
+
   return true;
 }
 
@@ -222,7 +236,7 @@ void LogHTMLForm(SavePasswordProgressLogger* logger,
 }
 
 bool FillDataContainsUsername(const PasswordFormFillData& fill_data) {
-  return !fill_data.basic_data.fields[0].name.empty();
+  return !fill_data.basic_data.username.name.empty();
 }
 
 }  // namespace
@@ -815,17 +829,17 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     bool form_contains_username_field = FillDataContainsUsername(form_data);
     if (form_contains_username_field) {
       username_element =
-          form_elements->input_elements[form_data.basic_data.fields[0].name];
+          form_elements->input_elements[form_data.basic_data.username.name];
     }
 
     // No password field, bail out.
-    if (form_data.basic_data.fields[1].name.empty())
+    if (form_data.basic_data.password.name.empty())
       break;
 
     // Get pointer to password element. (We currently only support single
     // password forms).
     password_element =
-        form_elements->input_elements[form_data.basic_data.fields[1].name];
+        form_elements->input_elements[form_data.basic_data.password.name];
 
     // If wait_for_username is true, we don't want to initially fill the form
     // until the user types in a valid username.
@@ -874,8 +888,8 @@ void PasswordAutofillAgent::GetSuggestions(
     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);
+      StartsWith(fill_data.basic_data.username.value, input, false)) {
+    suggestions->push_back(fill_data.basic_data.username.value);
     realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
   }
 
@@ -963,7 +977,7 @@ void PasswordAutofillAgent::FillFormOnPasswordRecieved(
       IsElementAutocompletable(username_element) &&
       username_element.value().isEmpty()) {
     // TODO(tkent): Check maxlength and pattern.
-    username_element.setValue(fill_data.basic_data.fields[0].value, true);
+    username_element.setValue(fill_data.basic_data.username.value, true);
   }
 
   // Fill if we have an exact match for the username. Note that this sets
@@ -995,11 +1009,11 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
   base::string16 password;
 
   // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
+  if (DoUsernamesMatch(fill_data.basic_data.username.value,
                        current_username,
                        exact_username_match)) {
-    username = fill_data.basic_data.fields[0].value;
-    password = fill_data.basic_data.fields[1].value;
+    username = fill_data.basic_data.username.value;
+    password = fill_data.basic_data.password.value;
   } else {
     // Scan additional logins for a match.
     PasswordFormFillData::LoginCollection::const_iterator iter;