Fix failing DCHECK in AutoCompleteInput()

Fix failing DCHECK in AutoCompleteInput()

BUG=656209
R=pkasting@chromium.org
TEST=OmniboxViewTest.FocusSearchLongUrl

Committed: https://crrev.com/2d3f670692d05b5b8937d286a7abb8cb04bc53c4
Cr-Commit-Position: refs/heads/master@{#426340}

[Tom (Use chromium acct), 2016-10-18 00:49:53]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 00:50:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 02:13:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 02:13:56]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2425703003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2425703003/diff/1/components/omnibox/browser/...
components/omnibox/browser/omnibox_edit_model.cc:415: if (user_text_ ==
view_->GetText()) {
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();
  }

  ...

[Tom (Use chromium acct), 2016-10-19 19:06:17]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 19:06:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom (Use chromium acct), 2016-10-19 19:08:53]

On 2016/10/19 05:32:06, Peter Kasting wrote:
>
https://codereview.chromium.org/2425703003/diff/1/components/omnibox/browser/...
> File components/omnibox/browser/omnibox_edit_model.cc (right):
> 
>
https://codereview.chromium.org/2425703003/diff/1/components/omnibox/browser/...
> components/omnibox/browser/omnibox_edit_model.cc:415: if (user_text_ ==
> view_->GetText()) {
> 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();
>   }
> 
>   ...

I've replaced StartAutocomplete with your suggested code, PTAL

[Peter Kasting, 2016-10-19 19:45:54]

LGTM

Thanks for the investigation here.  I'm not sure I would have thought of just
comparing the user text to the visible text had I tried to debug this. 
Everything I did beyond that was just tweaking :)

[commit-bot: I haz the power, 2016-10-19 21:05:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 21:05:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom (Use chromium acct), 2016-10-19 23:20:10]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2016-10-19 23:21:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 23:43:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:13:24]

Description was changed from

==========
Fix failing DCHECK in AutoCompleteInput()

BUG=656209
R=pkasting@chromium.org
TEST=OmniboxViewTest.FocusSearchLongUrl
==========

to

==========
Fix failing DCHECK in AutoCompleteInput()

BUG=656209
R=pkasting@chromium.org
TEST=OmniboxViewTest.FocusSearchLongUrl

Committed: https://crrev.com/2d3f670692d05b5b8937d286a7abb8cb04bc53c4
Cr-Commit-Position: refs/heads/master@{#426340}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:25]

Patchset 2 (id:??) landed as
https://crrev.com/2d3f670692d05b5b8937d286a7abb8cb04bc53c4
Cr-Commit-Position: refs/heads/master@{#426340}