[Password Manager] Modified to support saving passwords on forms without username 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 86ff3779dc76fb185f1e36c899ca5c95f9d6cf88..acce21899084a0519abec99cd51469662e8ef4f7 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -71,6 +71,11 @@ static bool FindFormInputElements(blink::WebFormElement* fe,
   // 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);
 
@@ -218,6 +223,12 @@ void LogHTMLForm(SavePasswordProgressLogger* logger,
                       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
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -802,13 +813,22 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     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
@@ -818,8 +838,9 @@ void PasswordAutofillAgent::OnFillPasswordForm(
 
     // 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;
@@ -830,8 +851,10 @@ void PasswordAutofillAgent::OnFillPasswordForm(
 
     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));
   }
@@ -929,8 +952,9 @@ void PasswordAutofillAgent::FillFormOnPasswordRecieved(
   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
@@ -939,7 +963,7 @@ void PasswordAutofillAgent::FillFormOnPasswordRecieved(
 
   // 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);
@@ -960,74 +984,82 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
     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;
+      // 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 (!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.
+    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);
+    // 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());
+      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;
     }
-  } 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
@@ -1074,7 +1106,10 @@ void PasswordAutofillAgent::PerformInlineAutocomplete(
 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 {