Fix failing DCHECK in AutoCompleteInput()

components/omnibox/browser/omnibox_edit_model.cc

 // Copyright 2012 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/omnibox/browser/omnibox_edit_model.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 394 matching lines @@
                                   false, true);
   client_->OnRevert();
 }
 
 void OmniboxEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete,
     bool entering_keyword_mode) {
   size_t cursor_position;
   const base::string16 input_text = MaybePrependKeyword(user_text_);
-  if (inline_autocomplete_text_.empty()) {
+  if (user_text_ == view_->GetText()) {

[Peter Kasting, 2016/10/19 05:32:06]

I spent some time tonight thinking about this and debugging various cases.  I
think this is basically correct, but I think we can go further to clean up the
code + comments in this function, since they're already somewhat misleading, and
I think this change will make them moreso.

First, the existing comments talk about cases where there's a non-empty inline
autocompletion.  The example mentioned -- pressing ctrl with an autocompletion
showing -- can no longer happen (we don't trigger autocompletion for this
anymore), so we should remove that example.  (There is still a case where this
can happen (hitting ctrl-k with an inline autocompletion visible) -- I filed
http://crbug.com/657243 regarding our behavior in that case.)  I think we should
motivate this better anyway.

Second, the comment about how |entering_keyword_mode| is handled doesn't really
make sense anymore.  It was updated in
https://codereview.chromium.org/1943683002/diff/160001/components/omnibox/bro...
, but rather mechanically, and after a lot of peering at this, I don't think it
accurately describes the world.

One reason is that the code there isn't even correct anymore.  In some cases,
when entering keyword mode, adjusting is actually the right thing to do -- for
example, when hitting ctrl-k after typing some text.  It turns out the cases
where the code is correct are the ones where the user_text_ and the visible text
match -- so after your change above, we can also just eliminate the "if
(!entering_keyword_mode)" below and that parameter to the function entirely, and
correspondingly simplify the comment.

Third, there's a weird edge case where if you type something, arrow down to an
entry whose temporary text exactly matches your typing, and then hit ctrl-K, we
probably want to set the cursor position to "end of the input" rather than
"current cursor position", to match how we handle other cases of typing and
hitting ctrl-K.  (I doubt this would ever have a visible effect but whatever.)

Here's my suggestion for how to write the beginning of this function so as to
address all the above:

void OmniboxEditModel::StartAutocomplete(bool has_selected_text,
                                         bool prevent_inline_autocomplete) {
  const base::string16 input_text = MaybePrependKeyword(user_text_);

  // Compute the cursor position.  There are three cases:
  //   1. The user is in the midst of typing; there is no inline autocompletion.
  //   2. The user is in the midst of typing; there is inline autocompletion.
  //   3. The user is not in the midst of typing, but is triggering this some
  //      other way, e.g. hitting ctrl-K while the view is showing the permanent
  //      or temporary text.
  size_t cursor_position;
  if (!has_temporary_text_ && (user_text_ == view_->GetText())) {
    // Case 1 above.  In this case there's a meaningful current cursor position,
    // so we read it from the view.  (Note that if there is a selected range,
    // the "cursor position" is considered to be the selection's end.)
    size_t start;
    view_->GetSelectionBounds(&start, &cursor_position);

    // For keyword searches, the text that AutocompleteInput expects is of the
    // form "<keyword> <query>", where our query is |user_text_|.  So we need to
    // adjust the cursor position forward by the length of any keyword added by
    // MaybePrependKeyword() above.
    cursor_position += input_text.length() - user_text_.length();
  } else {
    // Case 2 or 3 above.  In case 2, the existing inline autocompletion will be
    // ignored for this next autocomplete run.  The current cursor position is
    // always effectively "the end of the input text".  (If the user changes
    // this cursor position by arrowing, it will accept the inline 
    // autocompletion, which would put us in case 1.)  In case 3, there is no
    // meaningful current cursor position; the correct default behavior is to
    // simply claim the cursor is at the end of the input.
    cursor_position = input_text.length();
  }

  ...

     // Cursor position is equivalent to the current selection's end.
     size_t start;
     view_->GetSelectionBounds(&start, &cursor_position);
     // For keyword searches, the text that AutocompleteInput expects is of the
     // form "<keyword> <query>", where our query is |user_text_|.  So if we're
     // in keyword mode, we need to adjust the cursor position forward by the
     // length of "<keyword> ".  If we're just entering keyword mode, though, we
     // have to avoid making this adjustment, because we haven't actually updated
     // |user_text_| yet, but the caller has already cleared |is_keyword_hint_|,
     // so MaybePrependKeyword() will believe we are already in keyword mode, and
     // will thus mis-adjust the cursor position.
     if (!entering_keyword_mode)
       cursor_position += input_text.length() - user_text_.length();
   } else {
     // There are some cases where StartAutocomplete() may be called
-    // with non-empty |inline_autocomplete_text_|.  In such cases, we cannot
-    // use the current selection, because it could result with the cursor
-    // position past the last character from the user text.  Instead,
-    // we assume that the cursor is simply at the end of input.
-    // One example is when user presses Ctrl key while having a highlighted
-    // inline autocomplete text.
+    // when |inline_autocomplete_text_| is nonempty, or when
+    // view_->GetText().length() > input_text.length().  In such
+    // cases, we cannot use the current selection, because it could
+    // result with the cursor position past the last character from
+    // the user text.  Instead, we assume that the cursor is simply at
+    // the end of input.  One example is when user presses Ctrl key
+    // while having a highlighted inline autocomplete text.
     // TODO: Rethink how we are going to handle this case to avoid
-    // inconsistent behavior when user presses Ctrl key.
-    // See http://crbug.com/165961 and http://crbug.com/165968 for more details.
+    // inconsistent behavior when user presses Ctrl key.  See
+    // http://crbug.com/165961 and http://crbug.com/165968 for more
+    // details.
     cursor_position = input_text.length();
   }
 
   GURL current_url;
   if (client_->CurrentPageExists())
     current_url = client_->GetURL();
   input_ = AutocompleteInput(
       input_text, cursor_position, std::string(), current_url, ClassifyPage(),
       prevent_inline_autocomplete || just_deleted_text_ ||
           (has_selected_text && inline_autocomplete_text_.empty()) ||
@@ skipping 989 matching lines @@
   // Update state and notify view if the omnibox has focus and the caret
   // visibility changed.
   const bool was_caret_visible = is_caret_visible();
   focus_state_ = state;
   if (focus_state_ != OMNIBOX_FOCUS_NONE &&
       is_caret_visible() != was_caret_visible)
     view_->ApplyCaretVisibility();
 
   client_->OnFocusChanged(focus_state_, reason);
 }