[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..ed8bf0c21efbff3a7091ff447b65ef66b4b114ee 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -62,63 +62,61 @@ struct FormElements {
 
 typedef std::vector<FormElements*> FormElementsList;
 
+// Helper funtion to fill the input field.

[vabr (Chromium), 2014/10/13 12:30:52]

The comment is not useful -- it only says what the function name says.
It should describe:
* what the arguments are
* what the return value means
* what non-trivial function it carries out (just filling an input field sounds
as easy as a single assignment, so explain the hard job done here)

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

+static bool FillInputField(blink::WebFormElement* fe,

[vabr (Chromium), 2014/10/13 12:30:52]

Drop the "static", this is already in the anonymous namespace.

Also in the signature of FindFormInputElements.

[vabr (Chromium), 2014/10/13 12:30:52]

nit: fe -> form_element
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin...

Also in the signature of FindFormInputElements.

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

+                           const base::string16& field_name,
+                           FormElements* result,
+                           bool is_password_field) {

[vabr (Chromium), 2014/10/13 12:30:53]

optional nit: To make the callsite easier to read, consider using an enum like {
PASSWORD_FIELD, TEXT_FIELD } instead of the bool.

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

+  bool found_input = false;
+  blink::WebVector<blink::WebNode> temp_elements;
+
+  // Fill the username input field.
+  fe->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 (found_input) {
+        found_input = false;

[vabr (Chromium), 2014/10/13 12:30:53]

While you are changing this code anyway:
the old way of found_input being a Boolean is confusing here, since it's value
says "no input found", whereas more than one was found.
I suggest replacing it with a counter, and check at the end that it's exactly 1.

[Pritam Nikam, 2014/10/16 12:55:13]

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() != is_password_field)
+        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.

[vabr (Chromium), 2014/10/13 12:30:53]

While at it, please replace InputElement with HTMLInputElement.
Also below.

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

+      result->input_elements[field_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 found_input;
+}
+
 // 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;
-        }
+  bool found_input = FillInputField(fe, data.password.name, result, true);

[vabr (Chromium), 2014/10/13 12:30:52]

The code will be easier to read if you rename:
found_input -> password_found
and then exit early below as described there.

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

+  if (found_input && !data.username.name.empty())
+    found_input = FillInputField(fe, data.username.name, result, false);

[vabr (Chromium), 2014/10/13 12:30:53]

You can just exit early by

return found_input && (data.username.name.empty() || FillInputField(fe,
data.username.name, result, false));

[Pritam Nikam, 2014/10/16 12:55:13]

Done.

 
-        // 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;
-      }
-    }
-
-    // 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;
+  return found_input;
 }
 
 // Helper to locate form elements identified by |data|.
@@ -222,7 +220,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 +813,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 +872,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 +961,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 +993,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;