[Password Autofill] Make better use of prefilled usernames.

components/autofill/content/renderer/password_autofill_agent.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/autofill/content/renderer/password_autofill_agent.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 146 matching lines @@
       curr_elements->form_element = fe;
       results->push_back(curr_elements.release());
     }
   }
 }
 
 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;
   element->setAutofilled(autofilled);
   // Notify any changeEvent listeners.
   element->dispatchFormControlChangeEvent();
 }
 
 bool DoUsernamesMatch(const base::string16& username1,
                       const base::string16& username2,
@@ skipping 299 matching lines @@
       usernames_usage_ = OTHER_POSSIBLE_USERNAMES_ABSENT;
   }
 
   FormElementsList forms;
   // We own the FormElements* in forms.
   FindFormElements(render_view()->GetWebView(), form_data.basic_data, &forms);
   FormElementsList::iterator iter;
   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 =
         form_elements->input_elements[form_data.basic_data.fields[0].name];
 
     // Get pointer to password element. (We currently only support single
     // password forms).
     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) !=
         login_to_password_info_.end())
       continue;
 
     PasswordInfo password_info;
     password_info.fill_data = form_data;
     password_info.password_field = password_element;
     login_to_password_info_[username_element] = password_info;
@@ skipping 74 matching lines @@
                                  bounding_box.width() * scale,
                                  bounding_box.height() * scale);
   Send(new AutofillHostMsg_ShowPasswordSuggestions(routing_id(),
                                                    field,
                                                    bounding_box_scaled,
                                                    suggestions,
                                                    realms));
   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,
     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;
@@ skipping 32 matching lines @@
         }
         if (!username.empty() && !password.empty())
           break;
       }
     }
   }
   if (password.empty())
     return false;  // No match was found.
 
   // Input matches the username, fill in required values.
   username_element->setValue(username);

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

Is this safe for a potentially non-modifiable element?  Doesn't seem like it
ought to be.

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

Depends on what you mean by safe. There is nothing in the API that prevents you
from changing the value of an WebElement that has a readonly set. Obviously we
don't want to change such a value, but since we have exact_username_match set,
we shouldn't. I just added a test to verify this.

I did notice this while I was making the change, and ended up not guarding it. I
wanted to make sure that this cl would stick and it seemed like a slight
additional risk. There are some corner cases that I wasn't sure about. For
instance, if the user is choosing from the autocomplete dropdown and the field
becomes readonly during that time, what do we want to have happen? If the field
is readonly and we can't fill the username and password, do we return false?
Looks like the only consuming code is DidAcceptAutofillSuggestion which assumes
this will succeed, so no? 

I can either change it in this CL, change in a different CL, or leave it. Sounds
like you don't want the latter. Do you have any preference on the other two?

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

I'm not convinced that it's safe to assume that the Blink API is a no-op if the
value is unchanged.  There could easily be side-effects that are observable in
JavaScript, which IMO is not ok.
In the Autofill code, we skip over any fields that happen to have become
readonly between when the page loaded and when the user attempted to autofill
the form.  Fortunately, this is a rare event, so it's not likely to cause much
surprise to real users.
I'd prefer to change it in this CL, since it's pretty directly related to the
purpose of the CL, i.e. better supporting prefilled usernames, which I'd expect
to often be packaged into readonly fields.

[Garrett Casto, 2013/11/05 01:56:39]

Fixed.

 
   if (set_selection) {
     username_element->setSelectionRange(current_username.length(),
                                         username.length());
   }
 
   SetElementAutofilled(username_element, true);

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

Likewise, is this safe for a potentially non-modifiable element?

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

Similar to above, though it looks like this currently sets the password
autofilled to true even if the value isn't autofilled. This seems really
strange, but given that they check if the element is editable literally the line
before, I wonder if there is a reason that they did it that way. Likely just an
oversight, but it gives me pause.

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

It looks like an oversight to me too.  And it definitely seems wrong to fire off
a JavaScript onChange event for a readonly field.

[Garrett Casto, 2013/11/05 01:56:39]

Fixed.

   if (IsElementEditable(*password_element))
     password_element->setValue(password);
   SetElementAutofilled(password_element, true);
   return true;
 }
 
 void PasswordAutofillAgent::PerformInlineAutocomplete(
     const WebKit::WebInputElement& username_input,
     const WebKit::WebInputElement& password_input,
     const PasswordFormFillData& fill_data) {
@@ skipping 45 matching lines @@
   LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(input);
   if (iter == login_to_password_info_.end())
     return false;
 
   *found_input = input;
   *found_password = iter->second;
   return true;
 }
 
 }  // namespace autofill