[Autofill/Autocomplete Feature] Substring matching instead of prefix matching.

components/password_manager/core/browser/password_autofill_manager.cc

Index: components/password_manager/core/browser/password_autofill_manager.cc
diff --git a/components/password_manager/core/browser/password_autofill_manager.cc b/components/password_manager/core/browser/password_autofill_manager.cc
index 15d5f7e3c8da005d4853f4dc3a8e78098bcaaebb..6f4214e10b3272944f99dcdfe4edd9f1bf6db9e4 100644
--- a/components/password_manager/core/browser/password_autofill_manager.cc
+++ b/components/password_manager/core/browser/password_autofill_manager.cc
@@ -15,6 +15,7 @@
 #include "components/autofill/core/browser/suggestion.h"
 #include "components/autofill/core/common/autofill_constants.h"
 #include "components/autofill/core/common/autofill_data_validation.h"
+#include "components/autofill/core/common/autofill_util.h"
 #include "components/password_manager/core/browser/affiliation_utils.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/strings/grit/components_strings.h"
@@ -57,13 +58,24 @@ base::string16 GetHumanReadableRealm(const std::string& signon_realm) {
   return base::UTF8ToUTF16(signon_realm);
 }
 
+// Returns |true| if |kEnableSuggestionsWithSubStringMatch| command line switch
+// is on and |field_contents| matches the sub-string within |field_suggestion|;

[vabr (Chromium), 2015/03/23 14:17:33]

What does "the sub-string within" mean?
This comment makes it not clear, what the matching criterion is. Is any
substring match OK? and if you introduce "tokens", then please do not forget to
explain what are token separators.

[Pritam Nikam, 2015/03/24 11:39:37]

Done.

Rephrased to "Returns |true| if |kEnableSuggestionsWithSubstringMatch| command
line switch is on and |field_contents| matches substring token within
|field_suggestion| delimited by characters ' ', '.', ',', '-', '_' and '@';
otherwise |false|.".

+// otherwise |false|.
+bool DoesSuggestionsSubStringMatchContents(
+    const base::string16& field_suggestion,
+    const base::string16& field_contents) {
+  return autofill::IsFeatureSubStringMatchEnabled() &&
+         autofill::HasTokoneStartsWith(field_suggestion, field_contents);

[vabr (Chromium), 2015/03/23 14:17:33]

typo: Tokone

[Pritam Nikam, 2015/03/24 11:39:37]

Done.

+}
+
 // This function attempts to fill |suggestions| and |realms| form |fill_data|
 // based on |current_username|. Unless |show_all| is true, it only picks
-// suggestions where the username has |current_username| as a prefix.
+// suggestions where the username has |current_username| as a sub-string.
 void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
                     const base::string16& current_username,
                     std::vector<autofill::Suggestion>* suggestions,
                     bool show_all) {
+  std::vector<autofill::Suggestion> substring_matched_suggestions;

[vabr (Chromium), 2015/03/23 14:17:33]

Why do you separate the substring matched suggestions from the rest, when you
end up appending them anyway? Is it because of the order?

[Pritam Nikam, 2015/03/24 11:39:37]

Yes. Just to keep prefix matched suggestions up in the order followed by
substring matched suggestions. 

[vabr (Chromium), 2015/03/24 13:32:28]

First of all -- this is a significant UI decision. Reading
http://crbug.com/77194, I only see you mentioning in #16, but nobody else
comments on whether this is the desired order. Please clarify that on the bug.

If a consensus is reached that prefix matches should precede other token
matches, then please document that with a code comment here to explain the
reason for having |substring_matched_suggestions| separately. Also please do
mention a link to the consensus comment (http://crbug.com/77194#cXY for a
suitable XY).

[Pritam Nikam, 2015/03/25 14:25:26]

Sure! I'll wait for UX approval from ainslie@ regarding ordering for
suggestions. As you mentioned I'll document it in code providing the link to the
consensus comment.

   if (show_all ||
       StartsWith(fill_data.username_field.value, current_username, false)) {
     autofill::Suggestion suggestion(
@@ -71,6 +83,13 @@ void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
     suggestion.label = GetHumanReadableRealm(fill_data.preferred_realm);
     suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
     suggestions->push_back(suggestion);
+  } else if (DoesSuggestionsSubStringMatchContents(
+                 fill_data.username_field.value, current_username)) {
+    autofill::Suggestion suggestion(
+        ReplaceEmptyUsername(fill_data.username_field.value));
+    suggestion.label = GetHumanReadableRealm(fill_data.preferred_realm);
+    suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
+    substring_matched_suggestions.push_back(suggestion);
   }
 
   for (const auto& login : fill_data.additional_logins) {
@@ -79,6 +98,12 @@ void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
       suggestion.label = GetHumanReadableRealm(login.second.realm);
       suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
       suggestions->push_back(suggestion);
+    } else if (DoesSuggestionsSubStringMatchContents(login.first,
+                                                     current_username)) {
+      autofill::Suggestion suggestion(ReplaceEmptyUsername(login.first));
+      suggestion.label = GetHumanReadableRealm(login.second.realm);
+      suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
+      substring_matched_suggestions.push_back(suggestion);
     }
   }
 
@@ -91,9 +116,21 @@ void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
         suggestion.label = GetHumanReadableRealm(usernames.first.realm);
         suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
         suggestions->push_back(suggestion);
+      } else if (DoesSuggestionsSubStringMatchContents(usernames.second[i],
+                                                       current_username)) {
+        autofill::Suggestion suggestion(
+            ReplaceEmptyUsername(usernames.second[i]));
+        suggestion.label = GetHumanReadableRealm(usernames.first.realm);
+        suggestion.frontend_id = autofill::POPUP_ITEM_ID_PASSWORD_ENTRY;
+        substring_matched_suggestions.push_back(suggestion);
       }
     }
   }
+
+  // Now append usernames having sub-string matching.
+  for (const auto& suggestion : substring_matched_suggestions) {
+    suggestions->push_back(suggestion);

[vabr (Chromium), 2015/03/23 14:17:33]

optional nit: What about using std::insert instead?

[Pritam Nikam, 2015/03/24 11:39:37]

With std::insert we have to pass position as well, moreover here we need to
simply append to the end of the list. Currently, I'm planning to keep it as is.

[vabr (Chromium), 2015/03/24 13:32:28]

Having looked more deeply into this:
There are plenty [1] of places in the codebase where insert is used for
appending at the end. Technically, that's not std::insert, but the insert method
of the vector, but that's what I meant.

Calling Z.insert(...end(), X, Y) is therefore well understood as appending X...Y
to Z. When you write this as a for-loop, one has to check if you did it because
you do some further processing (like dereferencing) on the elements.

So I drop the "optional" from my comment, please rewrite this with
suggestions->insert().

[1]
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=i...

[Pritam Nikam, 2015/03/25 14:25:26]

Done.
Got rid of |substring_matched_suggestions| and instead populated all suggestions
marking their match type - prefix match or substring match and std::sorted them
to get desire order.

+  }
 }
 
 }  // namespace