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

components/autofill/content/renderer/password_autofill_agent.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/content/renderer/password_autofill_agent.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 49 matching lines @@
   FormInputElementMap input_elements;
 };
 
 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;
+  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.
         if (found_input) {
           found_input = false;
           break;
         }
 
         // Only fill saved passwords into password fields and usernames into
         // 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;
       }
     }
+  }
 
-    // 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;
+  // 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;
+      }
+
+      // 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;
 }
 
 // Helper to locate form elements identified by |data|.
 void FindFormElements(blink::WebView* view,
                       const FormData& data,
                       FormElementsList* results) {
   DCHECK(view);
   DCHECK(results);
   blink::WebFrame* main_frame = view->mainFrame();
@@ skipping 84 matching lines @@
 // Log a message including the name, method and action of |form|.
 void LogHTMLForm(SavePasswordProgressLogger* logger,
                  SavePasswordProgressLogger::StringID message_id,
                  const blink::WebFormElement& form) {
   logger->LogHTMLForm(message_id,
                       form.name().utf8(),
                       GURL(form.action().utf8()));
 }
 
 bool FillDataContainsUsername(const PasswordFormFillData& fill_data) {
-  return !fill_data.basic_data.fields[0].name.empty();
+  return !fill_data.basic_data.username.name.empty();
 }
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, public:
 
 PasswordAutofillAgent::PasswordAutofillAgent(content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       usernames_usage_(NOTHING_TO_AUTOFILL),
@@ skipping 572 matching lines @@
   for (iter = forms.begin(); iter != forms.end(); ++iter) {
     scoped_ptr<FormElements> form_elements(*iter);
 
     // Attach autocomplete listener to enable selecting alternate logins.
     blink::WebInputElement username_element, password_element;
 
     // Check whether the password form has a username input field.
     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.
     if (!form_data.wait_for_username)
       FillFormOnPasswordRecieved(form_data, username_element, password_element);
 
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
     if (login_to_password_info_.find(username_element) !=
         login_to_password_info_.end())
@@ skipping 28 matching lines @@
     : 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);
+      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));
   }
 
   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));
@@ skipping 67 matching lines @@
   // 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 (form_contains_username_field &&
       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
   // username to autofilled.
   FillUserNameAndPassword(&username_element,
                           &password_element,
                           fill_data,
                           true /* exact_username_match */,
                           false /* set_selection */);
 }
@@ skipping 11 matching lines @@
   base::string16 current_username;
   if (!username_element->isNull()) {
     current_username = username_element->value();
   }
 
   // username and password will contain the match found if any.
   base::string16 username;
   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;
     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;
@@ skipping 149 matching lines @@
   scoped_ptr<PasswordForm> password_form(CreatePasswordForm(form));
   if (!password_form || (restriction == RESTRICTION_NON_EMPTY_PASSWORD &&
                          password_form->password_value.empty() &&
                          password_form->new_password_value.empty())) {
     return;
   }
   provisionally_saved_forms_[frame].reset(password_form.release());
 }
 
 }  // namespace autofill