[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 c5986925655c495ce14568681a46695e53a3f80d..4cfba1e1410a62f38c278819c45ca233218b1f30 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -62,68 +62,72 @@ 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 find the unique entry of the |form_element| for the
+// specified input |field_name|. On successful find, adds it to |result|
+// and returns |true|. Otherwise clears the references from each

[vabr (Chromium), 2014/11/04 21:28:33]

This function can return true with an empty |result| -- that's when the sought
input field exists, but has wrong type (password for username or plain text for
a password). Please unify what the comment says and what the function does.
Given the usage below, the function should not return true if the result is
empty, AFAIU.

[Pritam Nikam, 2014/11/05 05:58:13]

Done.

[vabr (Chromium), 2014/11/05 08:23:40]

I don't see this comment addressed. The function can still return true with an
empty result.

[Pritam Nikam, 2014/11/05 10:53:00]

Done.

Sorry! I've missed to incorporate changes.

+// |HTMLInputElement| from |result| and returns |false|.
+bool FillInputField(blink::WebFormElement* form_element,

[Ilya Sherman, 2014/11/04 23:05:30]

What does "Fill" refer to in this function name?

[Pritam Nikam, 2014/11/05 05:58:13]

Done.

Rephrased to AddInputElementToResultWithMatchingFieldName().

+                    const base::string16& field_name,
+                    FormElements* result,
+                    FieldType field_type) {
+  blink::WebVector<blink::WebNode> temp_elements;
+  size_t field_match_counter = 0;
+  // Fill the username input field.

[Ilya Sherman, 2014/11/04 23:05:30]

What does this comment mean?

[Pritam Nikam, 2014/11/05 05:58:14]

Done.

Rephrased it to express the below excerpt.

+  form_element->getNamedElements(field_name, temp_elements);
+  for (size_t i = 0; i < temp_elements.size(); ++i) {

[Ilya Sherman, 2014/11/04 23:05:31]

nit: Please use a C++11 range-based for loop.

[Pritam Nikam, 2014/11/05 05:58:13]

I think we cannot use C++11 range-based for loop here.
for loop here, make use of custom implementation of vector i.e.
blink::WebVector<blink::WebNode> temp_elements; And unlike std::vector<>, it
does not support begin() or end() APIs.

Please correct me if I'm missing anything here.

[Ilya Sherman, 2014/11/07 20:19:25]

Yes, you're right -- my bad.

+    if (temp_elements[i].to<blink::WebElement>().hasHTMLTagName("input")) {
+      // Check for a non-unique match.
+      if (++field_match_counter > 1)

[Ilya Sherman, 2014/11/04 23:05:30]

Please use the boolean style that was used before.  You currently have a bug: if
the field_match_counter is 0, the behavior should be the same as if it is 2.

[Pritam Nikam, 2014/11/05 05:58:14]

Done.

+        break;
 
-        // 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;
-      }
-    }
+      // 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;
 
-    // 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;
+      // 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;
     }
   }
+
+  // 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

[Ilya Sherman, 2014/11/04 23:05:30]

Why did you change the case of "Clear"?  The function called below is named
"clear()".

[Pritam Nikam, 2014/11/05 05:58:13]

Done.

+  // |HTMLInputElement|.
+  if (field_match_counter > 1) {
+    result->input_elements.clear();
+    return false;
+  }
+
   return true;
 }
 
+// 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 PasswordFormFillData& data,
+                           FormElements* result) {
+  bool password_found = FillInputField(form_element, data.password_field.name,
+                                       result, PASSWORD_FIELD);

[Ilya Sherman, 2014/11/04 23:05:31]

nit: I think it would be clearer to inline this code rather than to define a
named variable for it.

[Pritam Nikam, 2014/11/05 05:58:14]

Done.

+
+  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|.
 void FindFormElements(blink::WebView* view,
-                      const FormData& data,
+                      const PasswordFormFillData& data,
                       FormElementsList* results) {
   DCHECK(view);
   DCHECK(results);
@@ -237,7 +241,7 @@ void LogHTMLForm(SavePasswordProgressLogger* logger,
 }
 
 bool FillDataContainsUsername(const PasswordFormFillData& fill_data) {
-  return !fill_data.basic_data.fields[0].name.empty();
+  return !fill_data.username_field.name.empty();
 }
 
 // This function attempts to fill |suggestions| and |realms| form |fill_data|
@@ -250,9 +254,8 @@ bool GetSuggestions(const PasswordFormFillData& fill_data,
                     bool show_all) {
   bool other_possible_username_shown = false;
   if (show_all ||
-      StartsWith(
-          fill_data.basic_data.fields[0].value, current_username, false)) {
-    suggestions->push_back(fill_data.basic_data.fields[0].value);
+      StartsWith(fill_data.username_field.value, current_username, false)) {
+    suggestions->push_back(fill_data.username_field.value);
     realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
   }
 
@@ -308,11 +311,10 @@ bool FillUserNameAndPassword(
   base::string16 password;
 
   // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
-                       current_username,
+  if (DoUsernamesMatch(fill_data.username_field.value, current_username,

[Ilya Sherman, 2014/11/04 23:05:30]

nit: Please run "git cl format" over your changes.  I believe this is not the
format that it recommends.

[Pritam Nikam, 2014/11/05 05:58:14]

Done.

Formatting here is after "git cl format" only. I've confirmed it again.
Shall I manually adjust it instead?

[Ilya Sherman, 2014/11/07 20:19:25]

Thanks for checking that, and sorry for the false alarm.

                        exact_username_match)) {
-    username = fill_data.basic_data.fields[0].value;
-    password = fill_data.basic_data.fields[1].value;
+    username = fill_data.username_field.value;
+    password = fill_data.password_field.value;
   } else {
     // Scan additional logins for a match.
     PasswordFormFillData::LoginCollection::const_iterator iter;
@@ -412,7 +414,7 @@ bool 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.username_field.value, true);
   }
 
   // Fill if we have an exact match for the username. Note that this sets
@@ -1026,7 +1028,7 @@ void PasswordAutofillAgent::OnFillPasswordForm(
 
   FormElementsList forms;
   // We own the FormElements* in forms.
-  FindFormElements(render_view()->GetWebView(), form_data.basic_data, &forms);
+  FindFormElements(render_view()->GetWebView(), form_data, &forms);
   FormElementsList::iterator iter;
   for (iter = forms.begin(); iter != forms.end(); ++iter) {
     scoped_ptr<FormElements> form_elements(*iter);
@@ -1038,17 +1040,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.username_field.name];
     }
 
     // No password field, bail out.
-    if (form_data.basic_data.fields[1].name.empty())
+    if (form_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.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.