Make Google Search autocomplete provider cursor aware.

chrome/browser/autocomplete/autocomplete_input.cc

Index: chrome/browser/autocomplete/autocomplete_input.cc
diff --git a/chrome/browser/autocomplete/autocomplete_input.cc b/chrome/browser/autocomplete/autocomplete_input.cc
index c964559b28899bea7cc0f501ebdd4dc55864ec86..c475712c15731f5981b7a66fbf462d0b29de9129 100644
--- a/chrome/browser/autocomplete/autocomplete_input.cc
+++ b/chrome/browser/autocomplete/autocomplete_input.cc
@@ -14,28 +14,51 @@
 #include "net/base/net_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
+namespace {
+
+void AdjustCursorPositionIfNecessary(size_t num_leading_chars_removed,
+                                     size_t* cursor_position) {
+  if (num_leading_chars_removed == 0 || *cursor_position == string16::npos)

[Peter Kasting, 2012/12/11 19:27:01]

Nit: I don't think the added fast-path condition here is important to
performance.  It seems simpler and clearer to me to just leave it out and let us
subtract zero below.

[Bart N., 2012/12/11 21:47:47]

Done.

+    return;
+  if (num_leading_chars_removed < *cursor_position)
+    *cursor_position -= num_leading_chars_removed;
+  else
+    *cursor_position = 0;
+}
+
+}  // namespace
+
 AutocompleteInput::AutocompleteInput()
-  : type_(INVALID),
-    prevent_inline_autocomplete_(false),
-    prefer_keyword_(false),
-    allow_exact_keyword_match_(true),
-    matches_requested_(ALL_MATCHES) {
+    : cursor_position_(string16::npos),
+      type_(INVALID),
+      prevent_inline_autocomplete_(false),
+      prefer_keyword_(false),
+      allow_exact_keyword_match_(true),
+      matches_requested_(ALL_MATCHES) {
 }
 
 AutocompleteInput::AutocompleteInput(const string16& text,
+                                     size_t cursor_position,
                                      const string16& desired_tld,
                                      bool prevent_inline_autocomplete,
                                      bool prefer_keyword,
                                      bool allow_exact_keyword_match,
                                      MatchesRequested matches_requested)
-    : desired_tld_(desired_tld),
+    : cursor_position_(cursor_position),
+      desired_tld_(desired_tld),
       prevent_inline_autocomplete_(prevent_inline_autocomplete),
       prefer_keyword_(prefer_keyword),
       allow_exact_keyword_match_(allow_exact_keyword_match),
       matches_requested_(matches_requested) {
+  // Initial cursor normalization not yet adjusted for possible text trimming.
+  if (cursor_position != string16::npos && cursor_position >= text.length()) {

[Peter Kasting, 2012/12/11 19:27:01]

When does this conditional actually fire?  I wonder if maybe whatever callers
need this should be adjusting the cursor position prior to entry and we should
just eliminate this here or convert to a DCHECK.

Also, no need for {}.

[Bart N., 2012/12/11 21:47:47]

I don't think this is possible, but I don't really want to go to each place
where AutucompleteInput is constructed and ensure this is true. Instead, I think
it's safer to simply normalize the cursor (which is done anyway later in this
method) to avoid obscure cases where for instance selection goes beyond the text
etc.

I also believe that DCHECKs are evil and should be avoided as much as possible
(details omitted). For instance, Search SREs don't allow code with DCHECKs in
prod :)
Done.

[Peter Kasting, 2012/12/11 22:15:50]

:(

That's never going to be an argument that wins my support.  Plus, you're adding
all the code that sets the cursor position in this very changelist.  You're
effectively saying you don't know what your additions do.  That doesn't make
much sense.
Different teams may have different intents for how code should be interpreted. 
In Chrome, DCHECKs mean "this is a function pre/post-condition" and are
effectively a form of documentation.  Because of this we want to encourage
DCHECKs wherever possible to guide readers in understanding the invariants in
play at any given time.

[Bart N., 2012/12/11 22:34:15]

You may remember your comment regarding selection and views implementation (on
this thread). Just by seeing the buggy behavior in views_views.cc (start/end are
reversed) and your remark regarding the code quality and lack of unit tests, I
really have no trust regarding the various view implementations (another example
is Linux view behavior). Rather than letting it fail in some cases, I much
prefer to ensure that 100% of the time that cursor is set properly, because it
may affect the results quality. There is nothing wrong with this, because not
only is this extra check cheap, but it's also conceptually in the right place,
because below we are going to normalize the cursor position anyway. The logic is
kept in one place and is easy to understand.

Saying that I don't know what my code does is a little bit unfair :)
While this is perhaps a good argument, I've seen enough cases where people put
in DCHECK non-trivial code, which resulted later in a very difficult to find
bugs. For this very reason, IMHO, I'd rather avoid risking this happening, but I
respect the Chrome's policy. Moreover, you may remember a bug I introduced by
not having DCHECKs enabled which is the default, at least in my git setup.

[Peter Kasting, 2012/12/12 00:24:11]

If we have bugs, we need to fix them.  Especially since omnibox_view_win.cc is
going to disappear and omnibox_view_views.cc is going to be the implementation
across Windows, Linux, and ChromeOS.

DCHECKing conditions that ought to hold here is a way to find these bugs.  The
Canary/Dev/Beta channel process is designed to ensure that we can check in
things like this if we are pretty sure they are correct, and nail any
outstanding issues we missed.
It's easy to understand what the code is doing.  What's misleading is what it
implies about the callers.  This is the same as a NULL-check on a variable that
can't be NULL.  If the situation shouldn't be able to happen, we need to make
that clear.

I know you will be unhappy at this, but I am simply not OK with checking this
code in as-is.  This code really should be

  DCHECK((cursor_position <= text.length()) ||
      (cursor_position == string16::npos));

We should also make UpdateText() DCHECK the same thing.

I just audited all the callsites for these.  Other than your new unit test, no
one should violate this condition.  Indeed, if anyone did, it would have to be
because GetSelectionBounds() returned a position outside the string.  If this
were to happen then all kinds of sites all over the omnibox code would behave
very badly (probably by crashing the browser).  Even were the start/end values
not properly directional (which I thought you said turned out not to be
happening after all in omnibox_view_views.cc?), that could not cause such a bug.
That is not a good thing.  All developer checkouts are supposed to enable
DCHECKs when you build in Debug mode.  If this isn't true for you, please file a
bug against maruel.

[Bart N., 2012/12/12 02:16:36]

Are Canary/Dev/Beta channel binaries built with DCHECK on?
I'll make the changes before I submit this CL and ping you back one more time in
case you want to take a look.
I build in Release mode as I found Debug mode extremely slow for the
development, at least on my Linux box.

+    cursor_position_ = text.length();
+  }
   // None of the providers care about leading white space so we always trim it.
   // Providers that care about trailing white space handle trimming themselves.
-  TrimWhitespace(text, TRIM_LEADING, &text_);
+  if ((TrimWhitespace(text, TRIM_LEADING, &text_) & TRIM_LEADING) != 0)
+    AdjustCursorPositionIfNecessary(text.length() - text_.length(),
+                                    &cursor_position_);
 
   GURL canonicalized_url;
   type_ = Parse(text_, desired_tld, &parts_, &scheme_, &canonicalized_url);
@@ -50,17 +73,21 @@ AutocompleteInput::AutocompleteInput(const string16& text,
        !canonicalized_url.host().empty()))
     canonicalized_url_ = canonicalized_url;
 
-  RemoveForcedQueryStringIfNecessary(type_, &text_);
+  size_t chars_removed = RemoveForcedQueryStringIfNecessary(type_, &text_);
+  AdjustCursorPositionIfNecessary(chars_removed, &cursor_position_);
 }
 
 AutocompleteInput::~AutocompleteInput() {
 }
 
 // static
-void AutocompleteInput::RemoveForcedQueryStringIfNecessary(Type type,
-                                                           string16* text) {
-  if (type == FORCED_QUERY && !text->empty() && (*text)[0] == L'?')
-    text->erase(0, 1);
+size_t AutocompleteInput::RemoveForcedQueryStringIfNecessary(Type type,
+                                                             string16* text) {
+  if (type != FORCED_QUERY || text->empty() || (*text)[0] != L'?')
+    return 0;
+  // Drop the leading '?'.
+  text->erase(0, 1);
+  return 1;
 }
 
 // static
@@ -456,13 +483,16 @@ int AutocompleteInput::NumNonHostComponents(const url_parse::Parsed& parts) {
 }
 
 void AutocompleteInput::UpdateText(const string16& text,
+                                   size_t cursor_position,
                                    const url_parse::Parsed& parts) {
   text_ = text;
+  cursor_position_ = cursor_position;
   parts_ = parts;
 }
 
 bool AutocompleteInput::Equals(const AutocompleteInput& other) const {
   return (text_ == other.text_) &&
+         (cursor_position_ == other.cursor_position_) &&
          (type_ == other.type_) &&
          (desired_tld_ == other.desired_tld_) &&
          (scheme_ == other.scheme_) &&
@@ -473,6 +503,7 @@ bool AutocompleteInput::Equals(const AutocompleteInput& other) const {
 
 void AutocompleteInput::Clear() {
   text_.clear();
+  cursor_position_ = string16::npos;
   type_ = INVALID;
   parts_ = url_parse::Parsed();
   scheme_.clear();