[Password Autofill] Make better use of prefilled usernames.

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 ce2f1031844cf0f582c32be523841057e09e4ebd..3a7cd14b5cce4b565843b7495ac477d35366d5b7 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -164,39 +164,6 @@ bool IsElementEditable(const WebKit::WebInputElement& element) {
   return element.isEnabled() && !element.isReadOnly();
 }
 
-void FillForm(FormElements* fe, const FormData& data) {
-  if (!fe->form_element.autoComplete())
-    return;
-
-  std::map<base::string16, base::string16> data_map;
-  for (size_t i = 0; i < data.fields.size(); i++)
-    data_map[data.fields[i].name] = data.fields[i].value;
-
-  for (FormInputElementMap::iterator it = fe->input_elements.begin();
-       it != fe->input_elements.end(); ++it) {
-    WebKit::WebInputElement element = it->second;
-    // Don't fill a form that has pre-filled values distinct from the ones we
-    // want to fill with.
-    if (!element.value().isEmpty() && element.value() != data_map[it->first])
-      return;
-
-    // Don't fill forms with uneditable fields or fields with autocomplete
-    // disabled.
-    if (!IsElementEditable(element) || !element.autoComplete())
-      return;
-  }
-
-  for (FormInputElementMap::iterator it = fe->input_elements.begin();
-       it != fe->input_elements.end(); ++it) {
-    WebKit::WebInputElement element = it->second;
-
-    // TODO(tkent): Check maxlength and pattern.
-    element.setValue(data_map[it->first]);
-    element.setAutofilled(true);
-    element.dispatchFormControlChangeEvent();
-  }
-}
-
 void SetElementAutofilled(WebKit::WebInputElement* element, bool autofilled) {
   if (element->isAutofilled() == autofilled)
     return;
@@ -516,11 +483,6 @@ void PasswordAutofillAgent::OnFillPasswordForm(
   for (iter = forms.begin(); iter != forms.end(); ++iter) {
     scoped_ptr<FormElements> form_elements(*iter);
 
-    // 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)
-      FillForm(form_elements.get(), form_data.basic_data);
-
     // Attach autocomplete listener to enable selecting alternate logins.
     // First, get pointers to username element.
     WebKit::WebInputElement username_element =
@@ -531,6 +493,12 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     WebKit::WebInputElement 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, form_elements->form_element,
+                                 username_element, password_element);

[Ilya Sherman, 2013/11/04 23:10:48]

nit: Please use curly braces to enclose the if-stmt body, since it spans
multiple lines.

[Garrett Casto, 2013/11/05 00:40:35]

Do you know where this is spelled out? I was actually looking to try and figure
out the rule for this, and I couldn't find it in either the Chrome or Google
style guides. Fixed regardless, I just can never remember if the rule is to use
braces if the actual clause is more than one line, or if the body is more than
one line.

[Ilya Sherman, 2013/11/05 01:25:11]

The style guide is pretty flexible: "In general, curly braces are not required
for single-line statements, but they are allowed if you like them; conditional
or loop statements with complex conditions or statements may be more readable
with curly braces. Some projects require that an if must always always have an
accompanying brace."  In practice, people generally tend to ask for curly braces
when the body spans multiple lines.  Some reviewers also ask for curly braces
when the condition spans multiple lines.  Autofill code is pretty consistent
about using curly braces around multi-line bodies; less so with multi-line
conditions.

+
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
     if (login_to_password_info_.find(username_element) !=
@@ -625,6 +593,31 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
   return !suggestions.empty();
 }
 
+void PasswordAutofillAgent::FillFormOnPasswordRecieved(
+    const PasswordFormFillData& fill_data,
+    const WebKit::WebFormElement& form_element,
+    WebKit::WebInputElement username_element,
+    WebKit::WebInputElement password_element) {
+  if (!form_element.autoComplete())
+    return;
+
+  // If we can't modify the password, don't try to set the username
+  if (!IsElementEditable(password_element) || !password_element.autoComplete())
+    return;
+
+  // Try and set the username to the preferred name, but only if the field
+  // can be set and isn't prefilled.
+  if (IsElementEditable(username_element) &&
+      username_element.autoComplete() &&
+      username_element.value().isEmpty()) {
+    username_element.setValue(fill_data.basic_data.fields[0].value);

[Ilya Sherman, 2013/11/04 23:10:48]

What happened to setAutofilled(), dispatchFormControlChangeEvent(), and tkent's
TODO?  (This applies to both the username, which is being set here, and the
password, which is being set below.  Might be worthwhile to decompose a helper
function to autofill an element.)

[Garrett Casto, 2013/11/05 00:40:35]

The TODO got lost, I'll add it back somewhere. The setAutofilled and
dispatchFormControlChangeEvent still happens in FillUserNameAndPassword(). Do
you want me to add a comment to this affect?

[Ilya Sherman, 2013/11/05 01:25:11]

Ah, sorry, I didn't read the SetElementAutofilled calls closely enough. 
Nevermind :)

+  }
+
+  // Fill if we have an exact match for the username.
+  FillUserNameAndPassword(&username_element, &password_element, fill_data,
+                          true, false);

[Ilya Sherman, 2013/11/04 23:10:48]

nit: The semantics of "true" and "false" are really hard to make out here.  If
you're feeling particularly motivated, you could convert the API to accept
enumerated values for these parameters rather than booleans.  But if not, please
at least document them inline as "true /* exact_usernames_match */" or by using
a local named variable like "bool exact_usernames_match = true;".

[Garrett Casto, 2013/11/05 00:40:35]

Done with comments for now.

[Ilya Sherman, 2013/11/05 01:25:11]

Thanks :)

+}
+
 bool PasswordAutofillAgent::FillUserNameAndPassword(
     WebKit::WebInputElement* username_element,
     WebKit::WebInputElement* password_element,