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

components/autofill/content/renderer/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/autofill_agent.h"
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "components/autofill/content/common/autofill_messages.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
 #include "components/autofill/content/renderer/page_click_tracker.h"
 #include "components/autofill/content/renderer/password_autofill_agent.h"
 #include "components/autofill/content/renderer/password_generation_agent.h"
 #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"
 #include "components/autofill/core/common/password_form.h"
 #include "components/autofill/core/common/web_element_descriptor.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/ssl_status.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_view.h"
@@ skipping 688 matching lines @@
                                        WebInputElement* node) {
   base::AutoReset<bool> auto_reset(&ignore_text_changes_, true);
   node->setEditingValue(value.substr(0, node->maxLength()));
 }
 
 void AutofillAgent::PreviewFieldWithValue(const base::string16& value,
                                           WebInputElement* node) {
   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() {
   // Record timestamp of when the forms are first seen. This is used to
   // measure the overhead of the Autofill feature.
   base::TimeTicks forms_seen_timestamp = base::TimeTicks::Now();
 
   WebLocalFrame* frame = render_frame()->GetWebFrame();
   std::vector<FormData> forms = form_cache_.ExtractNewForms();
 
@@ skipping 49 matching lines @@
 
 void AutofillAgent::LegacyAutofillAgent::OnDestruct() {
   // No-op. Don't delete |this|.
 }
 
 void AutofillAgent::LegacyAutofillAgent::FocusChangeComplete() {
   agent_->FocusChangeComplete();
 }
 
 }  // namespace autofill