Teach PasswordAutofillAgent sometimes match prefixes of 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 893d84f30b0ab1569f50c664b8d93779c0df27b7..c4768a37513f02c6799ac27586b01cdfa07985d6 100644
--- a/components/autofill/content/renderer/password_autofill_agent.cc
+++ b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -268,13 +268,13 @@ bool IsElementEditable(const blink::WebInputElement& element) {
   return element.IsEnabled() && !element.IsReadOnly();
 }
 
-bool DoUsernamesMatch(const base::string16& username1,
-                      const base::string16& username2,
+bool DoUsernamesMatch(const base::string16& potential_suggestion,
+                      const base::string16& current_username,
                       bool exact_match) {
-  if (exact_match)
-    return username1 == username2;
-  return FieldIsSuggestionSubstringStartingOnTokenBoundary(username1, username2,
-                                                           true);
+  bool match = (potential_suggestion == current_username);

[vabr (Chromium), 2017/06/12 18:56:58]

optional nit: I am a bit concerned that we have two bool variables called
*match, but their logical type is different (one is a directive about how strict
matching to use, the other captures whether the two strings match).

A little more concise way to write this is by pulling the match out of the
branch, and it gets around the above issue by not naming |match|.

if (potential_suggestion == current_username)
  return true;
return !exact_match && IsPrefixEndingOnTokenBoundary(...)

[melandory, 2017/06/19 10:53:48]

Done.

+  return exact_match ? match
+                     : (match || IsPrefixEndingOnTokenBoundary(
+                                     current_username, potential_suggestion));
 }
 
 // Returns whether the given |element| is editable.
@@ -373,6 +373,51 @@ void UpdateFieldValueAndPropertiesMaskMap(
   }
 }
 
+void FindMatchesByUsername(const PasswordFormFillData& fill_data,

[vabr (Chromium), 2017/06/12 18:56:58]

nit: Please comment on what the function does and what is the meaning of the
arguments.

[melandory, 2017/06/19 10:53:48]

Done.

+                           const base::string16& current_username,
+                           bool exact_username_match,
+                           RendererSavePasswordProgressLogger* logger,
+                           base::string16* username,
+                           base::string16* password) {
+  // Look for any suitable matches to current field text.
+  if (DoUsernamesMatch(fill_data.username_field.value, current_username,
+                       exact_username_match)) {
+    *username = fill_data.username_field.value;
+    *password = fill_data.password_field.value;
+    if (logger)
+      logger->LogMessage(Logger::STRING_USERNAMES_MATCH);
+  } else {
+    // Scan additional logins for a match.
+    for (const auto& it : fill_data.additional_logins) {
+      if (DoUsernamesMatch(it.first, current_username, exact_username_match)) {
+        *username = it.first;
+        *password = it.second.password;
+        break;
+      }
+    }
+    if (logger) {
+      logger->LogBoolean(Logger::STRING_MATCH_IN_ADDITIONAL,
+                         !(username->empty() && password->empty()));
+    }
+
+    // Check possible usernames.
+    if (username->empty() && password->empty()) {
+      for (const auto& it : fill_data.other_possible_usernames) {
+        for (size_t i = 0; i < it.second.size(); ++i) {
+          if (DoUsernamesMatch(it.second[i], current_username,
+                               exact_username_match)) {
+            *username = it.second[i];
+            *password = it.first.password;
+            break;
+          }
+        }
+        if (!username->empty() && !password->empty())

[vabr (Chromium), 2017/06/12 18:56:58]

I know that this is just the old code moved around, but I think it is slightly
wrong here. I would just drop the !username->empty() part -- the username could
be empty.

The pre-condition of the cycle implies that the password is empty, and whenever
a username is assigned inside it, password is also. So even restricted to
non-empty usernames, just checking !password->empty() should be equivalent.

[melandory, 2017/06/19 10:53:48]

Done.

+          break;
+      }
+    }
+  }
+}
+
 // This function attempts to fill |username_element| and |password_element|
 // with values from |fill_data|. The |password_element| will only have the
 // suggestedValue set, and will be registered for copying that to the real
@@ -404,43 +449,9 @@ bool FillUserNameAndPassword(
   base::string16 username;
   base::string16 password;
 
-  // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.username_field.value, current_username,
-                       exact_username_match)) {
-    username = fill_data.username_field.value;
-    password = fill_data.password_field.value;
-    if (logger)
-      logger->LogMessage(Logger::STRING_USERNAMES_MATCH);
-  } else {
-    // Scan additional logins for a match.
-    for (const auto& it : fill_data.additional_logins) {
-      if (DoUsernamesMatch(it.first, current_username, exact_username_match)) {
-        username = it.first;
-        password = it.second.password;
-        break;
-      }
-    }
-    if (logger) {
-      logger->LogBoolean(Logger::STRING_MATCH_IN_ADDITIONAL,
-                         !(username.empty() && password.empty()));
-    }
+  FindMatchesByUsername(fill_data, current_username, exact_username_match,
+                        logger, &username, &password);
 
-    // Check possible usernames.
-    if (username.empty() && password.empty()) {
-      for (const auto& it : fill_data.other_possible_usernames) {
-        for (size_t i = 0; i < it.second.size(); ++i) {
-          if (DoUsernamesMatch(
-                  it.second[i], current_username, exact_username_match)) {
-            username = it.second[i];
-            password = it.first.password;
-            break;
-          }
-        }
-        if (!username.empty() && !password.empty())
-          break;
-      }
-    }
-  }
   if (password.empty())
     return false;
 
@@ -462,9 +473,10 @@ bool FillUserNameAndPassword(
       form_util::PreviewSuggestion(username, current_username,
                                    username_element);
     }
-  } else if (current_username != username) {
+  } else if (exact_username_match && current_username != username) {

[vabr (Chromium), 2017/06/12 18:56:58]

There is not code path to satisfy this condition: exact_username_match means
that either username was set equal to current_username above, or the password
stayed empty, in which case this function exited on line 456. Please drop the
branch completely.

[melandory, 2017/06/19 10:53:48]

Done.

     // If the username can't be filled and it doesn't match a saved password
-    // as is, don't autofill a password.
+    // as is, autofill the password only in case the username element id read
+    // only.
     return false;
   }
 
@@ -550,12 +562,15 @@ bool FillFormOnPasswordReceived(
         blink::WebString::FromUTF16(fill_data.username_field.value));
   }
 
-  // Fill if we have an exact match for the username. Note that this sets
-  // username to autofilled.
+  bool exact_username_match =
+      username_element.IsNull() || IsElementEditable(username_element);
+  // User the exact match for the editable username fields and allow prefix

[vabr (Chromium), 2017/06/12 18:56:58]

nit: User -> Use

[melandory, 2017/06/19 10:53:48]

Done.

+  // match for read-only username fields.
+  // Note that this sets username to autofilled.

[vabr (Chromium), 2017/06/12 18:56:58]

nit: The latter sentence no longer makes sense ("this" referred to "Fill...",
which was deleted). Also, I suspect that it is not true for read-only fields. My
suggestion is to drop this sentence.

[melandory, 2017/06/19 10:53:48]

Done.

   return FillUserNameAndPassword(
-      &username_element, &password_element, fill_data,
-      true /* exact_username_match */, false /* set_selection */,
-      field_value_and_properties_map, registration_callback, logger);
+      &username_element, &password_element, fill_data, exact_username_match,
+      false /* set_selection */, field_value_and_properties_map,
+      registration_callback, logger);
 }
 
 // Annotate |forms| with form and field signatures as HTML attributes.