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 fd52df402f4af45b7093dd304339c83e79794f98..b605ebb74aff7b12e9246c1aeffc76c8b46810b6 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"
@@ -301,8 +303,7 @@ bool PasswordAutofillAgent::TextFieldDidEndEditing(
   FillUserNameAndPassword(&username,
                           &password,
                           fill_data,
-                          true /* exact_username_match */,
-                          false /* set_selection */);
+                          EXACT_USERNAME_MATCH);
   return true;
 }
 
@@ -917,34 +918,45 @@ void PasswordAutofillAgent::FillFormOnPasswordRecieved(
   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) &&
-      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 */);
+                          EXACT_USERNAME_MATCH | FILL_PREFERRED_USERNAME);
 }
 
 bool PasswordAutofillAgent::FillUserNameAndPassword(
     blink::WebInputElement* username_element,
     blink::WebInputElement* password_element,
     const PasswordFormFillData& fill_data,
-    bool exact_username_match,
-    bool set_selection) {
+    const int options) {

[Garrett Casto, 2014/09/08 21:27:20]

So I think that the behavior if the username if already filled out is slightly
strange. If the username is one that we have data for, the user will see the
username with autofill highlighting but not password. If they click on the
username field the dropdown should appear and if they select that they should
get a password. If we don't have data for the particular username it's also less
likely that the user will understand what is going on, as it looks like the
username field was just autofilled, but at least the password will fill if they
try to change the username.

I would probably just avoid these cases for now. If the user sees a field that
is highlighted with Autofill but doesn't have any data, I'm not sure what they
are going to think, but if they see a field with data that is highlighted they
will probably just think that it was autofilled. If we decide to go down this
route and have a better indicator we should show it in all cases, but for now I
would just fill normally if the username if prefilled. I also know that this
comes up on accounts.google.com where the username field is not only prefilled
but hidden, so if we don't do this the password manager will just break.

This also makes the logic much simpler, just add an additional if statement in
FillFormOnPasswordRecieved()

if (fill_on_account_select) {
  username_element.setAutofilled(true);
  return;
} else {
  username_element.setValue(...);
}

[jww, 2014/09/09 01:40:34]

I was admittedly a bit confused by your comment, but I think I got the gist of
it and have addressed the issues, but you'll definitely want to double check my
latest CL :-)
On 2014/09/08 21:27:20, Garrett Casto wrote:

+  bool exact_username_match = (options & EXACT_USERNAME_MATCH) != 0;
+  bool set_selection = (options & SET_SELECTION) != 0;
+  bool fill_preferred_username = (options & FILL_PREFERRED_USERNAME) != 0;
+  bool fill_on_account_select = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableFillOnAccountSelect);

[Garrett Casto, 2014/09/08 21:27:20]

You should pull this out into a function so that we can easily add a finch
experiment for this. Also you are currently ignoring the disable flag that you
added.

[jww, 2014/09/09 01:40:34]

Done.

   base::string16 current_username = username_element->value();
   // username and password will contain the match found if any.
   base::string16 username;
   base::string16 password;
 
+  // Try to set the username to the preferred name, but only if the field
+  // can be set and isn't prefilled. If FILL_PREFERRED_USERNAME
+  // is set, set |current_username| to the preferred value, but don't set the
+  // element's value. This is done so that even if values aren't actually
+  // filled, if there is a valid value to fill with, the elements will be
+  // properly marked as autofilled later.
+  if (fill_preferred_username &&
+      IsElementAutocompletable(*username_element) &&
+      username_element->value().isEmpty()) {
+    current_username = fill_data.basic_data.fields[0].value;
+    if (!fill_on_account_select) {
+      // TODO(tkent): Check maxlength and pattern.
+      username_element->setValue(current_username, true);
+    }
+  }
+
   // Look for any suitable matches to current field text.
   if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
                        current_username,
@@ -998,12 +1010,15 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
 
   // 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 (!fill_on_account_select) {
+      username_element->setValue(username, 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
@@ -1014,7 +1029,9 @@ bool PasswordAutofillAgent::FillUserNameAndPassword(
   // 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);
+  if (!fill_on_account_select) {
+    password_element->setSuggestedValue(password);
+  }
   gatekeeper_.RegisterElement(password_element);
 
   password_element->setAutofilled(true);
@@ -1047,8 +1064,7 @@ void PasswordAutofillAgent::PerformInlineAutocomplete(
   FillUserNameAndPassword(&username,
                           &password,
                           fill_data,
-                          false /* exact_username_match */,
-                          true /* set_selection */);
+                          SET_SELECTION);
 #endif
 }