[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..f82151cce0707e99050afd591f9694748e43d8f2 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -62,63 +62,68 @@ struct FormElements {
 
 typedef std::vector<FormElements*> FormElementsList;
 
-// Helper to search the given form element for the specified input elements
-// in |data|, and add results to |result|.
-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;
-    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.
-        if (found_input) {
-          found_input = false;
-          break;
-        }
+enum FieldType { PASSWORD_FIELD, TEXT_FIELD };
+
+// Utility function to search the unique entry of the |form_element| for the

[vabr (Chromium), 2014/10/16 16:34:24]

nit: search -> find
Currently it sounds like the entry is being searched, i.e., the code is trying
to find something in the entry instead of finding the entry.

[Pritam Nikam, 2014/11/04 13:22:21]

Done.

+// specified input |field_name|. On successful search, adds it to |result|
+// and returns |true|. Otherwise clears the references from each
+// |HTMLInputElement| from |result| and returns |false|.
+bool FillInputField(blink::WebFormElement* form_element,
+                    const base::string16& field_name,
+                    FormElements* result,
+                    FieldType field_type) {
+  size_t field_match_counter = 0;
+  blink::WebVector<blink::WebNode> temp_elements;
 
-        // Only fill saved passwords into password fields and usernames into
-        // text fields.
-        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;
-        found_input = true;
+  // Fill the username input field.
+  form_element->getNamedElements(field_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.
+      if (field_match_counter > 0) {
+        ++field_match_counter;

[vabr (Chromium), 2014/10/16 16:34:24]

Currently the behaviour wrt. duplicates depends on their order. Suppose the form
has two elements with the same name, equalt to |field_name|, and that one of
those elements is a password field, and the other is a plain text field.

Now, if the plain text field comes first, |field_match_counter| is not increased
for it (due to the "continue" on line 93), and thus the second field (the
password field) is happily accepted.

But when the password field comes first, and then the text field is seen, the
|field_match_counter| reaches 2 and leads to returning "false".

Please decide what is the appropriate result for the situation independent of
the order of the two entries, and adjust the code.

[Pritam Nikam, 2014/11/04 13:22:22]

Done.


In my opinion, even if we would resolve the ordering problem that you
highlighted here, because of @line no. 101, field that appears in last shall
overwrite the entry |result->input_elements| map. Best way to bail out in such
cases is to skip filling and return immediately.

Please correct me, if missing something here?

+        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>();
+      if (input_element.isPasswordField() != (field_type == PASSWORD_FIELD))
+        continue;
+
+      // This |input_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 |input_elements| it can get at them through the
+      // |FormElement*|. Note: This assignment adds a reference to the
+      // |HTMLInputElement|.
+      result->input_elements[field_name] = input_element;
+      ++field_match_counter;
     }
   }
-  return true;
+
+  // A required |input_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
+  // |HTMLInputElement|.
+  if (field_match_counter != 1)
+    result->input_elements.clear();

[vabr (Chromium), 2014/10/16 16:34:24]

optional nit: If you rephrase the last 4 lines to:

if (field_match_counter != 1) {
  result->input_elements.clear();
  return false;
}

return true;

then it will be clearer that if the counter is not 1, we are bailing out
"early". Currently it does not do much difference (hence this is optional), but
if somebody will add some more processing in the case of success, the code above
will make it easier to recognise there is no need to handle the counter != 1
case any more.

[Pritam Nikam, 2014/11/04 13:22:22]

Done.

+
+  return (field_match_counter == 1);
+}
+
+// Helper to search the given |form_element| for the specified input elements in
+// |data|, and add results to |result|.
+bool FindFormInputElements(blink::WebFormElement* form_element,
+                           const FormData& data,
+                           FormElements* result) {
+  bool password_found = FillInputField(
+      form_element, data.password_field.name, result, PASSWORD_FIELD);
+  return password_found &&
+         (data.username_field.name.empty() ||
+          FillInputField(
+              form_element, data.username_field.name, result, TEXT_FIELD));
 }
 
 // Helper to locate form elements identified by |data|.
@@ -222,7 +227,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_field.name.empty();
 }
 
 }  // namespace
@@ -815,17 +820,18 @@ 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_field.name];
     }
 
     // No password field, bail out.
-    if (form_data.basic_data.fields[1].name.empty())
+    if (form_data.basic_data.password_field.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_field.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 +880,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_field.value, input, false)) {
+    suggestions->push_back(fill_data.basic_data.username_field.value);
     realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
   }
 
@@ -963,7 +969,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_field.value, true);
   }
 
   // Fill if we have an exact match for the username. Note that this sets
@@ -995,11 +1001,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_field.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_field.value;
+    password = fill_data.basic_data.password_field.value;
   } else {
     // Scan additional logins for a match.
     PasswordFormFillData::LoginCollection::const_iterator iter;