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

components/autofill/content/renderer/autofill_agent.cc

Index: components/autofill/content/renderer/autofill_agent.cc
diff --git a/components/autofill/content/renderer/autofill_agent.cc b/components/autofill/content/renderer/autofill_agent.cc
index b6686c3cb5eb678c7b96f121ea07cdef8f133984..cffccfb6d840c4214e3b1753a327f7df1878b81b 100644
--- a/components/autofill/content/renderer/autofill_agent.cc
+++ b/components/autofill/content/renderer/autofill_agent.cc
@@ -22,6 +22,7 @@
 #include "components/autofill/core/common/autofill_constants.h"
 #include "components/autofill/core/common/autofill_data_validation.h"
 #include "components/autofill/core/common/autofill_switches.h"
+#include "components/autofill/core/common/autofill_util.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/autofill/core/common/form_data_predictions.h"
 #include "components/autofill/core/common/form_field_data.h"
@@ -730,8 +731,14 @@ void AutofillAgent::PreviewFieldWithValue(const base::string16& value,
   was_query_node_autofilled_ = element_.isAutofilled();
   node->setSuggestedValue(value.substr(0, node->maxLength()));
   node->setAutofilled(true);
-  node->setSelectionRange(node->value().length(),
-                          node->suggestedValue().length());
+
+  if (IsFeatureSubstringMatchEnabled()) {
+    size_t start = autofill::GetTextSelectionStart(value, node->value());

[please use gerrit instead, 2015/06/11 19:10:25]

You've calculated the |start| value previously, so it seems a bad design to
re-calculate it again. I would change this particular function to the following.

void AutofillAgent::PreviewFieldWithValue(size_t start,
                                          const base::string16& value,
                                          WebInputElement* node) {
  was_query_node_autofilled_ = element_.isAutofilled();
  node->setSuggestedValue(value.substr(0, node->maxLength()));
  node->setAutofilled(true);
  node->setSelectionRange(start, node->suggestedValue().length());
}

The next comment suggests that you change WebFormControlElement to have
setSuggestedValue() setter. After you make that change, this function will look
like this:

void AutofillAgent::PreviewFieldWithValue(size_t start,
                                          const base::string16& value,
                                          WebInputElement* node) {
  was_query_node_autofilled_ = element_.isAutofilled();
  node->setSuggestedValueStart(
      start > node->maxLength() ? node->maxLength() : start);
  node->setSuggestedValue(value.substr(0, node->maxLength()));
  node->setAutofilled(true);
  node->setSelectionRange(nodde->suggestedValueStart(),
                          node->suggestedValue().length());
}

[Pritam Nikam, 2015/06/12 11:09:11]

We do not recalculate |start| here, we do it only once in renderer.

Instead, on browser-process side we just verify whether suggestions (token) do
have substring matching the user inputs
(autofill::ContainsTokenThatStartsWith()) & order suggestions prefixes followed
by substrings. And on renderer-process side while preview we do this calculation
to get |start| index (autofill::GetTextSelectionStart()) just before the text
range selection. Please correct me if I'm missing something here.

Are you suggesting to get rid of ContainsTokenThatStartsWith() and instead
calculate |start| using GetTextSelectionStart() on browser-process side while
populating suggestions?

IMO, in that case we do need to cache start indices along the suggestions and
pass them along over IPC on previews (filed/form etc...) to renderer-process to
use them to set selection range.

[please use gerrit instead, 2015/06/22 00:32:59]

Ah, I was slightly confused. I thought that you calculate |start| in the browser
and also in the renderer. After looking at the code some more, I see that you're
actually doing tokenization in the browser and call GetTextSelectionStart() in
the renderer. I think this approach is not the best, because there's a slight
chance that GetTextSelectionStart() will return base::string16::npos even though
ContainsTokenThatStartsWith() returned false. For example,. I see that
ContainsTokenThatStartsWith() takes a |case_sensitive| parameter, but
|GetTextSelectionStart()| does not. This is one discrepancy that raises alarms.

Therefore, I think that you should rework autofill_util.cc so that  it has only
GetTextSelectionStart(). The caller to GetTextSelectionStart() should check for
base::string16::npos. If the return value is not base::string16::npos, then it
should be stored in node->setSuggestedValueStart(int).

Yes, this will involve changing some IPCs.

I would also prefer that you change
components/autofill/core/browser/suggestion.cc to contain a "size_t
match_start;" instead of MatchMode. MatchMode enum is not necessary, because
PREFIX_MATCH is essentially the same as a SUBSBSTRING_MATCH where |match_start|
is always 0. This change will make more of your code generic.

[Pritam Nikam, 2015/06/22 18:31:39]

Cases where ContainsTokenThatStartsWith() returned false,
GetTextSelectionStart() will not get call. Moreover, in GetTextSelectionStart()
we have a cushion of NOTREACHED() just to address this case.
Once we have shortlisted suggestions by considering |case_sensitive|, for
preview we can safely ignore it.
If its ok, I'll do these in follow-up CLs:
1. Blink side modifications - adding setSuggestedValueStart
2. Chromium side - Changes in suggestion structure and IPCs.

+    node->setSelectionRange(start, node->suggestedValue().length());
+  } else {
+    node->setSelectionRange(node->value().length(),
+                            node->suggestedValue().length());
+  }
 }
 
 void AutofillAgent::ProcessForms() {