Fill on account select in the password manager

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 f2882c9ff03c8f317012bc560fe544ab5dbe9f69..ac534c0f7d6dc04fb9f98e75bf2bf6793b8b2eae 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -5,6 +5,7 @@
 #include "components/autofill/content/renderer/password_autofill_agent.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ -13,6 +14,7 @@
 #include "components/autofill/content/renderer/form_autofill_util.h"
 #include "components/autofill/content/renderer/password_form_conversion_utils.h"
 #include "components/autofill/content/renderer/renderer_save_password_progress_logger.h"
+#include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/form_field_data.h"
 #include "components/autofill/core/common/password_autofill_util.h"
 #include "components/autofill/core/common/password_form.h"
@@ -39,6 +41,12 @@
 namespace autofill {
 namespace {
 
+enum FillUserNameAndPasswordOptions {
+  EXACT_USERNAME_MATCH = 1 << 0,
+  SET_SELECTION = 1 << 1,
+  FILL_PREFERRED_USERNAME = 1 << 2
+};
+
 // The size above which we stop triggering autocomplete.
 static const size_t kMaximumTextSizeForAutocomplete = 1000;
 
@@ -62,6 +70,23 @@ struct FormElements {
 
 typedef std::vector<FormElements*> FormElementsList;
 
+bool ShouldFillOnAccountSelect() {
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableFillOnAccountSelect)) {
+    return true;
+  }
+
+  // This is made explicit in anticiption of experimental groups being added.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableFillOnAccountSelect)) {
+    return false;
+  }
+
+  // TODO(jww): Add experimental groups check here.
+
+  return false;
+}
+
 // 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,
@@ -281,14 +306,16 @@ bool GetSuggestionsStats(const PasswordFormFillData& fill_data,
 // with values from |fill_data|. The |password_element| will only have the
 // |suggestedValue| set, and will be registered for copying that to the real
 // value through |registration_callback|. The function returns true when
-// selected username comes from |fill_data.other_possible_usernames|.
+// selected username comes from |fill_data.other_possible_usernames|. |options|
+// should be a bitwise mask of FillUserNameAndPasswordOptions values.
 bool FillUserNameAndPassword(
     blink::WebInputElement* username_element,
     blink::WebInputElement* password_element,
     const PasswordFormFillData& fill_data,
-    bool exact_username_match,
-    bool set_selection,
+    const int options,
     base::Callback<void(blink::WebInputElement*)> registration_callback) {
+  bool exact_username_match = (options & EXACT_USERNAME_MATCH) != 0;
+  bool set_selection = (options & SET_SELECTION) != 0;
   bool other_possible_username_selected = false;
   // Don't fill username if password can't be set.
   if (!IsElementAutocompletable(*password_element))
@@ -383,7 +410,7 @@ bool FillUserNameAndPassword(
 // |suggestedValue| set, and |suggestedValue| will be registered for copying to
 // the real value through |registration_callback|. Returns true when the
 // username gets selected from |other_possible_usernames|, else returns false.
-bool FillFormOnPasswordRecieved(
+bool FillFormOnPasswordReceived(
     const PasswordFormFillData& fill_data,
     blink::WebInputElement username_element,
     blink::WebInputElement password_element,
@@ -402,23 +429,36 @@ bool FillFormOnPasswordRecieved(
   if (!IsElementAutocompletable(password_element))
     return false;
 
-  // 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);
+  // Try to set the username to the preferred name, but only if:
+  //   (a) The username element is autocompletable, and
+  //   (b) The fill-on-account-select flag is not set, and
+  //   (b) The username element isn't prefilled

[Garrett Casto, 2014/11/04 23:35:30]

Is that last line supposed to be (c)?

[jww, 2014/11/06 21:28:56]

Yup :-)

+  //
+  // If (a) is true, but (b) is false, then just mark the username element as
+  // autofilled and return so the fill step is skipped.
+  //
+  // If (a) is false, but (b) is true, then the username element should not be
+  // autofilled, but the user should still be able to select to fill the
+  // password element, so the password element must be marked as autofilled and
+  // the fill step should also be skipped.
+  if (IsElementAutocompletable(username_element)) {

[Garrett Casto, 2014/11/04 23:35:30]

I think that you dropped the form_contains_username_field check. It's possible
that username_element is invalid here.

[jww, 2014/11/06 21:28:56]

Done.

[Garrett Casto, 2014/11/23 07:49:48]

Can you add a test so that this would have been caught? 

[jww, 2014/11/25 02:46:25]

Yup, unit tests are my next step.

+    if (ShouldFillOnAccountSelect()) {
+      username_element.setAutofilled(true);
+      return false;
+    } else if (username_element.value().isEmpty()) {
+      // TODO(tkent): Check maxlength and pattern.
+      username_element.setValue(fill_data.basic_data.fields[0].value, true);
+    }
+  } else if (ShouldFillOnAccountSelect()) {
+    password_element.setAutofilled(true);
+    return false;
   }
 
   // Fill if we have an exact match for the username. Note that this sets
   // username to autofilled.
-  return FillUserNameAndPassword(&username_element,
-                                 &password_element,
-                                 fill_data,
-                                 true /* exact_username_match */,
-                                 false /* set_selection */,
-                                 registration_callback);
+  return FillUserNameAndPassword(
+      &username_element, &password_element, fill_data,
+      EXACT_USERNAME_MATCH | FILL_PREFERRED_USERNAME, registration_callback);
 }
 
 // Takes a |map| with pointers as keys and linked_ptr as values, and returns
@@ -520,11 +560,7 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
   if (FillUserNameAndPassword(
-          &username,
-          &password,
-          fill_data,
-          true /* exact_username_match */,
-          false /* set_selection */,
+          &username, &password, fill_data, EXACT_USERNAME_MATCH,
           base::Bind(&PasswordValueGatekeeper::RegisterElement,
                      base::Unretained(&gatekeeper_)))) {
     usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
@@ -684,11 +720,28 @@ bool PasswordAutofillAgent::DidClearAutofillSelection(
   return true;
 }
 
+PasswordAutofillAgent::LoginToPasswordInfoMap::iterator
+PasswordAutofillAgent::FindPasswordInfoForElement(
+    const blink::WebInputElement& element) {
+  const blink::WebInputElement* username_element;

[Garrett Casto, 2014/11/04 23:35:30]

Why does this need to be a pointer?

[jww, 2014/11/06 21:28:56]

I was trying to have only return statement, and since the value username_element
is dependent on whether element is a password field, I made it assignable.
However, I've rewritten the method to get rid of the pointer, so let me know
which version you prefer (or if there's another option I'm not thinking of).

+  if (!element.isPasswordField()) {
+    username_element = &element;
+  } else {
+    PasswordToLoginMap::const_iterator password_iter =
+        password_to_username_.find(element);

[Garrett Casto, 2014/11/04 23:35:30]

Where is this defined and populated?

[jww, 2014/11/06 21:28:57]

It's defined in password_autofill_agent.h and it is populated in
OnFillPasswordForm in password_autofill_agent.cc (at the same time and place
that login_to_password_info_ is populated). In case it's not clear, I did not
add this field.

+    if (password_iter == password_to_username_.end())
+      return login_to_password_info_.end();
+    username_element = &password_iter->second;
+  }
+
+  return login_to_password_info_.find(*username_element);
+}
+
 bool PasswordAutofillAgent::ShowSuggestions(
     const blink::WebInputElement& element,
     bool show_all) {
   LoginToPasswordInfoMap::const_iterator iter =
-      login_to_password_info_.find(element);
+      FindPasswordInfoForElement(element);
   if (iter == login_to_password_info_.end())
     return false;
 
@@ -700,7 +753,13 @@ bool PasswordAutofillAgent::ShowSuggestions(
       !IsElementAutocompletable(iter->second.password_field))
     return true;
 
-  return ShowSuggestionPopup(iter->second.fill_data, element, show_all);
+  // Chrome should never show more than one account for a password element since
+  // this implies that the username element cannot be modified. Thus even if
+  // |show_all| is true, check if the element in question is a password element
+  // for the call to ShowSuggestionPopup.
+  return ShowSuggestionPopup(iter->second.fill_data, iter->first,
+                             show_all && !element.isPasswordField(),
+                             element.isPasswordField());

[Garrett Casto, 2014/11/04 23:35:30]

Is there any reason why you need the fourth parameter here instead of just
passing |element| as the second param and change behavior in
ShowSuggestionPopup() depending on if element.isPasswordField() is true or not?

[jww, 2014/11/06 21:28:57]

Yes, because we ultimately need the username as the element, not the password
field. Of course, we could pass in the password element and then check and find
the username element, but given that this is a special case, it seems to make
the most sense to abstract out the search for the username element to the one
place it's used. Note the additional search step on line 723 in
FindPasswordInfoForElement; it seems weird to call that again inside
ShowSuggestionPopup (since it's already needed inside ShowSuggestions()).

[Garrett Casto, 2014/11/23 07:49:48]

I'm not sure that I understand why we need the username in ShowSuggestionPopup
if we are going to show the popup on the password field. As is, it looks like it
just immediately converts the username element to the password element. Assuming
|user_input| was always the element that you wanted the popup to be shown on,
couldn't you just remove the first new if statement in ShowSuggestionPopup?

[jww, 2014/11/25 02:46:25]

I believe it's because the browser process can only look up mappings by
username. Thus, we need to pass to the browser the username that we want the
credential for.

 }
 
 bool PasswordAutofillAgent::OriginCanAccessPasswordManager(
@@ -1049,10 +1108,8 @@ void PasswordAutofillAgent::OnFillPasswordForm(
     // 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,
+        FillFormOnPasswordReceived(
+            form_data, username_element, password_element,
             base::Bind(&PasswordValueGatekeeper::RegisterElement,
                        base::Unretained(&gatekeeper_)))) {
       usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
@@ -1095,7 +1152,8 @@ PasswordAutofillAgent::PasswordInfo::PasswordInfo()
 bool PasswordAutofillAgent::ShowSuggestionPopup(
     const PasswordFormFillData& fill_data,
     const blink::WebInputElement& user_input,
-    bool show_all) {
+    bool show_all,
+    bool show_on_password_field) {
   blink::WebFrame* frame = user_input.document().frame();
   if (!frame)
     return false;
@@ -1110,6 +1168,12 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
       user_input, &form, &field, REQUIRE_NONE);
 
   blink::WebInputElement selected_element = user_input;
+  if (show_on_password_field) {
+    LoginToPasswordInfoMap::const_iterator iter =
+        login_to_password_info_.find(user_input);
+    DCHECK(iter != login_to_password_info_.end());
+    selected_element = iter->second.password_field;
+  }
   gfx::Rect bounding_box(selected_element.boundsInViewportSpace());
 
   float scale = web_view_->pageScaleFactor();
@@ -1117,8 +1181,15 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
                                  bounding_box.y() * scale,
                                  bounding_box.width() * scale,
                                  bounding_box.height() * scale);
-  Send(new AutofillHostMsg_ShowPasswordSuggestions(
-      routing_id(), field, user_input.value(), show_all, bounding_box_scaled));
+  if (show_on_password_field) {
+    Send(new AutofillHostMsg_ShowPasswordSuggestionsForPasswordField(
+        routing_id(), field, user_input.value(), show_all,
+        bounding_box_scaled));
+  } else {
+    Send(new AutofillHostMsg_ShowPasswordSuggestions(
+        routing_id(), field, user_input.value(), show_all,
+        bounding_box_scaled));
+  }
 
   bool suggestions_present = false;
   if (GetSuggestionsStats(fill_data, user_input.value(), show_all,
@@ -1152,11 +1223,7 @@ void PasswordAutofillAgent::PerformInlineAutocomplete(
   // Fill the user and password field with the most relevant match. Android
   // only fills in the fields after the user clicks on the suggestion popup.
   if (FillUserNameAndPassword(
-          &username,
-          &password,
-          fill_data,
-          false /* exact_username_match */,
-          true /* set_selection */,
+          &username, &password, fill_data, SET_SELECTION,
           base::Bind(&PasswordValueGatekeeper::RegisterElement,
                      base::Unretained(&gatekeeper_)))) {
     usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
@@ -1197,7 +1264,7 @@ bool PasswordAutofillAgent::FindLoginInfo(const blink::WebNode& node,
     return false;
 
   blink::WebInputElement input = element.to<blink::WebInputElement>();
-  LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(input);
+  LoginToPasswordInfoMap::iterator iter = FindPasswordInfoForElement(input);
   if (iter == login_to_password_info_.end())
     return false;