[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 44 matching lines @@
 // necessary form elements. To avoid having to look these up again when we want
 // to fill the form, the FindFormElements function stores the pointers
 // in a FormElements* result, referenced to ensure they are safe to use.
 struct FormElements {
   blink::WebFormElement form_element;
   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();
+enum FieldType { PASSWORD_FIELD, TEXT_FIELD };
 
-  // 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);
+// 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;
 
-    // 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;
-        }
+  // 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;
+      }
 
-        // 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;
+      // 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 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;
+      // 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|.
 void FindFormElements(blink::WebView* view,
                       const FormData& data,
                       FormElementsList* results) {
   DCHECK(view);
   DCHECK(results);
   blink::WebFrame* main_frame = view->mainFrame();
   if (!main_frame)
@@ skipping 83 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_field.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_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.
     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_field.value, input, false)) {
+    suggestions->push_back(fill_data.basic_data.username_field.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_field.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_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;
     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