Autofill: Refactoring to support fetching password after a username is selected

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 df8056242e3eed16495550ae170eeafec6200860..236ed08c2884af9c7b870b95a42f556b3c5156c8 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -351,21 +351,28 @@ bool PasswordAutofillAgent::TextFieldHandlingKeyDown(
   return true;
 }
 
-bool PasswordAutofillAgent::DidAcceptAutofillSuggestion(
+bool PasswordAutofillAgent::AcceptAutofillSuggestion(
     const blink::WebNode& node,
-    const blink::WebString& username) {
-  blink::WebInputElement input;
-  PasswordInfo password;
-  if (!FindLoginInfo(node, &input, &password))
+    const blink::WebString& username,
+    const blink::WebString& password) {
+  blink::WebInputElement username_element;
+  PasswordInfo password_info;
+  if (!FindLoginInfo(node, &username_element, &password_info))
     return false;
 
-  // Set the incoming |username| in the text field and |FillUserNameAndPassword|
-  // will do the rest.
-  input.setValue(username, true);
-  return FillUserNameAndPassword(&input, &password.password_field,
-                                 password.fill_data,
-                                 true /* exact_username_match */,
-                                 true /* set_selection */);
+  if (IsElementAutocompletable(username_element) &&
+      IsElementAutocompletable(password_info.password_field)) {

[Ilya Sherman, 2014/03/28 21:33:34]

nit: I would invert this condition and include an early return stmt, so that the
body of the method can be less indented.

[Patrick Dubroy, 2014/04/01 16:08:48]

Done.

+    base::string16 current_username = username_element.value();
+    username_element.setValue(username, true);
+    SetElementAutofilled(&username_element, true);
+    username_element.setSelectionRange(current_username.length(),
+                                       username.length());
+
+    password_info.password_field.setValue(password, true);
+    SetElementAutofilled(&password_info.password_field, true);
+  }
+
+  return true;
 }

[Ilya Sherman, 2014/03/28 21:33:34]

Could you please add test coverage for this method?  Ideally the tests should
have been able to catch the lack of IsElementAutocompletable() calls or the
forgotten setSelectionRange() call.  (I apologize that there wasn't already test
coverage in place for this.)

[Patrick Dubroy, 2014/04/01 16:08:48]

Done. In doing so, I did noticed some weird behavior of the old code though:

- The username is always filled, even if the username or password is not
autocompletable or the login doesn't match.
- In those situations, the username is not marked as autofilled.

I'm assuming those behaviors weren't intentional, so I've fixed them in my code.
Do you know of a reason that I shouldn't fix them?

[Garrett Casto, 2014/04/01 17:32:21]

I think that this was an oversight, but note that if this happens you are in a
pretty strange situation to begin with. The user has selected their credentials
from the UI, so not filling them is going to be confusing as well.  I'm not sure
if it matters much what we plan on doing at this point as this shouldn't happen
on well behaving sites unless we have a bug.

 
 bool PasswordAutofillAgent::DidClearAutofillSelection(