[Password Manager] Modified to support saving passwords on forms without username 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 53 matching lines @@
 
 // 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) {
   // 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.
   for (size_t j = 0; j < data.fields.size(); j++) {
+    // Only consider non-empty input fields for autofilling

[vabr (Chromium), 2014/09/24 09:47:43]

I'm confused by this comment -- http://crbug.com/406343 is about saving
passwords on forms without a username, not about empty input fields.
Also, the comment says "non-empty input field", whereas the code checks for the
field's name (not field itself) being empty.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+    // (http://crbug.com/406343).
+    if (data.fields[j].name.empty())
+      continue;
+
     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) {
@@ skipping 127 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()));
 }
 
+// Returns |true| if |fill_data| holds an empty username field; otherwise
+// |false|.
+static bool FormWithEmptyUsernameField(const PasswordFormFillData& fill_data) {

[vabr (Chromium), 2014/09/24 09:47:43]

Again -- the comment and the function name say "empty field", whereas the code
tests the emptiness of the field's name.

My suggestion is: rename the function to FillDataContainUsername (and negate the
result to accommodate the change in semantics). You can leave out the explaining
comment entirely, because all will be clear from the code.

[vabr (Chromium), 2014/09/24 09:47:44]

No need to include "static" -- the function is invisible outside of the .cc file
because of being in the anonymous namespace already.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+  return fill_data.basic_data.fields[0].name.empty();
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, public:
 
 PasswordAutofillAgent::PasswordAutofillAgent(content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       usernames_usage_(NOTHING_TO_AUTOFILL),
       web_view_(render_view->GetWebView()),
       logging_state_active_(false),
@@ skipping 564 matching lines @@
   }
 
   FormElementsList forms;
   // We own the FormElements* in forms.
   FindFormElements(render_view()->GetWebView(), form_data.basic_data, &forms);
   FormElementsList::iterator iter;
   for (iter = forms.begin(); iter != forms.end(); ++iter) {
     scoped_ptr<FormElements> form_elements(*iter);
 
     // Attach autocomplete listener to enable selecting alternate logins.
-    // First, get pointers to username element.
-    blink::WebInputElement username_element =
-        form_elements->input_elements[form_data.basic_data.fields[0].name];
+    blink::WebInputElement username_element, password_element;
+
+    // Check whether password form has username input field

[vabr (Chromium), 2014/09/24 09:47:43]

grammar nit: add articles -- the password form, a username input field

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+    // (http://crbug.com/406343).

[vabr (Chromium), 2014/09/24 09:47:43]

nit: I'm not sure it's necessary to include the reference to the bug. It's
already clear that the code tests the existence of the username field, no more
context (possibly found on the bug) is necessary to understand the code.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+    bool is_password_only_form = FormWithEmptyUsernameField(form_data);

[vabr (Chromium), 2014/09/24 09:47:43]

(Note: if you change the function to FillDataContainUsername, don't forget to
rename the bool appropriately, and stop negating it below.)

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+    if (!is_password_only_form)

[vabr (Chromium), 2014/09/24 09:47:43]

nit: Add braces, because the body of the if-clause spans more lines.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+      username_element =
+          form_elements->input_elements[form_data.basic_data.fields[0].name];
+
+    // No password field, bail out.
+    if (form_data.basic_data.fields[1].name.empty())
+      break;
 
     // Get pointer to password element. (We currently only support single
     // password forms).
-    blink::WebInputElement password_element =
+    password_element =
         form_elements->input_elements[form_data.basic_data.fields[1].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())
+    if (!is_password_only_form &&

[vabr (Chromium), 2014/09/24 09:47:43]

Why do you exclude the case of no-username passwords? You apparently add an
entry for them in login_to_password_info_.

If any two empty WebInputElements are equal, then the find() below should work.
If not, then there is no point in adding forms without username element to the
login_to_password_info_ map. Please find out what is the situation, and amend
accordingly.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+        login_to_password_info_.find(username_element) !=
+            login_to_password_info_.end())
       continue;
 
     PasswordInfo password_info;
     password_info.fill_data = form_data;
     password_info.password_field = password_element;
     login_to_password_info_[username_element] = password_info;
     password_to_username_[password_element] = username_element;
 
     FormData form;
     FormFieldData field;
-    FindFormAndFieldForFormControlElement(
-        username_element, &form, &field, REQUIRE_NONE);
+    if (!is_password_only_form)
+      FindFormAndFieldForFormControlElement(

[vabr (Chromium), 2014/09/24 09:47:43]

nit: missing braces

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+          username_element, &form, &field, REQUIRE_NONE);
+
     Send(new AutofillHostMsg_AddPasswordFormMapping(
         routing_id(), field, form_data));
   }
 }
 
 void PasswordAutofillAgent::OnSetLoggingState(bool active) {
   logging_state_active_ = active;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ skipping 77 matching lines @@
 
 void PasswordAutofillAgent::FillFormOnPasswordRecieved(
     const PasswordFormFillData& fill_data,
     blink::WebInputElement username_element,
     blink::WebInputElement password_element) {
   // Do not fill if the password field is in an iframe.
   DCHECK(password_element.document().frame());
   if (password_element.document().frame()->parent())
     return;
 
+  bool is_password_only_form = FormWithEmptyUsernameField(fill_data);
   if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      !username_element.form().autoComplete())
+      (!is_password_only_form && !username_element.form().autoComplete()))

[vabr (Chromium), 2014/09/24 09:47:43]

nit: no need for the additional parentheses -- && is associative

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

     return;
 
   // 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 (IsElementAutocompletable(username_element) &&
+  if (!is_password_only_form && IsElementAutocompletable(username_element) &&
       username_element.value().isEmpty()) {
     // TODO(tkent): Check maxlength and pattern.
     username_element.setValue(fill_data.basic_data.fields[0].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 */);
 }
 
 bool PasswordAutofillAgent::FillUserNameAndPassword(
     blink::WebInputElement* username_element,
     blink::WebInputElement* password_element,
     const PasswordFormFillData& fill_data,
     bool exact_username_match,
     bool set_selection) {
-  base::string16 current_username = username_element->value();
   // username and password will contain the match found if any.
   base::string16 username;
   base::string16 password;
+  bool is_password_only_form = FormWithEmptyUsernameField(fill_data);
 
   // Don't fill username if password can't be set.
   if (!IsElementAutocompletable(*password_element))
     return false;
 
-  // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,

[vabr (Chromium), 2014/09/24 09:47:44]

Suggestion: if you modify this check to also consider an empty
|current_username| and an empty |fill_data.basic_data.fields[0].value| as a
match, then you can leave most of the code below (and the line above) as it is.

[Pritam Nikam, 2014/09/24 12:57:55]

I think, I didn't get this correctly.

-                       current_username,
-                       exact_username_match)) {
-    username = fill_data.basic_data.fields[0].value;
+  // Password form can have only password-input field (http://crbug.com/406343).
+  if (is_password_only_form) {
     password = fill_data.basic_data.fields[1].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;
-        break;
+    base::string16 current_username = username_element->value();
+
+    // Look for any suitable matches to current field text.
+    if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
+                         current_username,
+                         exact_username_match)) {
+      username = fill_data.basic_data.fields[0].value;
+      password = fill_data.basic_data.fields[1].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;
+          break;
+        }
+      }
+
+      // Check possible usernames.
+      if (username.empty() && password.empty()) {
+        for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
+                 fill_data.other_possible_usernames.begin();
+             iter != fill_data.other_possible_usernames.end();
+             ++iter) {
+          for (size_t i = 0; i < iter->second.size(); ++i) {
+            if (DoUsernamesMatch(
+                    iter->second[i], current_username, exact_username_match)) {
+              usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+              username = iter->second[i];
+              password = iter->first.password;
+              break;
+            }
+          }
+          if (!username.empty() && !password.empty())
+            break;
+        }
       }
     }
 
-    // Check possible usernames.
-    if (username.empty() && password.empty()) {
-      for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
-               fill_data.other_possible_usernames.begin();
-           iter != fill_data.other_possible_usernames.end();
-           ++iter) {
-        for (size_t i = 0; i < iter->second.size(); ++i) {
-          if (DoUsernamesMatch(
-                  iter->second[i], current_username, exact_username_match)) {
-            usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
-            username = iter->second[i];
-            password = iter->first.password;
-            break;
-          }
-        }
-        if (!username.empty() && !password.empty())
-          break;
+    if (password.empty())
+      return false;  // No match was found.
+
+    // TODO(tkent): Check maxlength and pattern for both username and password
+    // fields.
+
+    // Input matches the username, fill in required values.
+    if (IsElementAutocompletable(*username_element)) {
+      username_element->setValue(username, true);
+      username_element->setAutofilled(true);
+
+      if (set_selection) {
+        username_element->setSelectionRange(current_username.length(),
+                                            username.length());
       }
+    } else if (current_username != username) {
+      // If the username can't be filled and it doesn't match a saved password
+      // as is, don't autofill a password.
+      return false;
     }
   }
-  if (password.empty())
-    return false;  // No match was found.
-
-  // TODO(tkent): Check maxlength and pattern for both username and password
-  // fields.
-
-  // Input matches the username, fill in required values.
-  if (IsElementAutocompletable(*username_element)) {
-    username_element->setValue(username, true);
-    username_element->setAutofilled(true);
-
-    if (set_selection) {
-      username_element->setSelectionRange(current_username.length(),
-                                          username.length());
-    }
-  } else if (current_username != username) {
-    // If the username can't be filled and it doesn't match a saved password
-    // as is, don't autofill a password.
-    return false;
-  }
 
   // Wait to fill in the password until a user gesture occurs. This is to make
   // sure that we do not fill in the DOM with a password until we believe the
   // user is intentionally interacting with the page.
   password_element->setSuggestedValue(password);
   gatekeeper_.RegisterElement(password_element);
 
   password_element->setAutofilled(true);
   return true;
 }
@@ skipping 25 matching lines @@
                           &password,
                           fill_data,
                           false /* exact_username_match */,
                           true /* set_selection */);
 #endif
 }
 
 void PasswordAutofillAgent::FrameClosing(const blink::WebFrame* frame) {
   for (LoginToPasswordInfoMap::iterator iter = login_to_password_info_.begin();
        iter != login_to_password_info_.end();) {
-    if (iter->first.document().frame() == frame) {
+    // Password form may not have username input fields

[vabr (Chromium), 2014/09/24 09:47:43]

Suggested rephrasing to simplify the comment:

// There may not be a username field, so get the frame from the password field.

[Pritam Nikam, 2014/09/24 12:57:55]

Done.

+    // (http://crbug.com/406343), and hence consider password input field
+    // instead as a reference.
+    if (iter->second.password_field.document().frame() == frame) {
       password_to_username_.erase(iter->second.password_field);
       login_to_password_info_.erase(iter++);
     } else {
       ++iter;
     }
   }
   for (FrameToPasswordFormMap::iterator iter =
            provisionally_saved_forms_.begin();
        iter != provisionally_saved_forms_.end();) {
     if (iter->first == frame)
@@ skipping 46 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