Fix handling of inline autocomplete text vis-a-vis Instant.

Fix handling of inline autocomplete text vis-a-vis Instant.

Consider the following scenario:
1. Type "g" in the omnibox.
--> The omnibox inline autocompletes "oogle.com".
2. Hit Tab.
--> You're now in keyword mode.

Here, inline_autocomplete_text_ is still "oogle.com", though the display text
is a blank string. The previous code caused a crash. Now, we check properly
that the inline text is a suffix of the display text.

Arguably, the inline text should always be a suffix of the display text, say by
clearing it in OmniboxEditModel::AcceptKeyword(): http://crbug.com/141890

R=pkasting@google.com
BUG=141878
TEST=Scenario in CL description above shouldn't crash.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151146

[sreeram, 2012-08-10 18:37:33]

Peter, it turns out I can't use user_text_ (or DisplayTextFromUserText) for
sending the query to Instant, because when the user arrows up/down through query
suggestions in the drop down list, user_text_ doesn't change; only the
view_->GetText() changes.

[Peter Kasting, 2012-08-10 19:01:57]

This still uses GetText().  I don't think you should do that.

I'm not clear on what this code is trying to accomplish.  Can you give me a
clearer picture?  Is seems like using some variant of
DisplayTextFromUserText(user_text_ + inline_autocomplete_text_) might be more
what you want.

See my newest comments on bug 141878 for how you could probably try and address
the inline_autocomplete_text_ part of this.

[Peter Kasting, 2012-08-10 19:04:39]

On 2012/08/10 18:37:33, sreeram wrote:
> Peter, it turns out I can't use user_text_ (or DisplayTextFromUserText) for
> sending the query to Instant, because when the user arrows up/down through
query
> suggestions in the drop down list, user_text_ doesn't change; only the
> view_->GetText() changes.

Why do we want to send anything to instant when arrowing?  That seems kind of
wrong?

GetText() is not the only thing that changes here, you can look at the
temporary_text-related state variables.

[Peter Kasting, 2012-08-10 19:07:37]

In fact in general you should be checking those anyway.  If you have temporary
text, the inline_autocomplete_text_ and the user_text_ are both irrelevant
because they relate to the original typing and not the visible match.

But as noted, if you have temporary text, why are we talking to instant anyway?

[sreeram, 2012-08-10 19:21:47]

On 2012/08/10 19:01:57, Peter Kasting wrote:
> I'm not clear on what this code is trying to accomplish.  Can you give me a
> clearer picture?  Is seems like using some variant of
> DisplayTextFromUserText(user_text_ + inline_autocomplete_text_) might be more
> what you want.

We are trying to send the display text to Instant, separated out into two pieces
- the portion before any inline autocompletion, and the inline completion
itself. Historically and currently, Instant just combines them and does a
search. However, in the extended API, we explicitly want to know the two pieces.
I don't see how to split off the inline_autcomplete_text_ using the method you
mentioned.

> This still uses GetText().  I don't think you should do that.
> GetText() is not the only thing that changes here, you can look at the
> temporary_text-related state variables.

We've always used GetText() here, right from the days of the original Instant
implementation. It might be possible to look at the temporary text variables
instead, but I don't see what's terribly wrong with using GetText()? Various
places in OmniboxEditModel also use GetText(), when they could also just as
arguably use the model's knowledge of temporary text.

On 2012/08/10 19:04:39, Peter Kasting wrote:
> Why do we want to send anything to instant when arrowing?  That seems kind of
> wrong?

If the user arrows up/down to a query, we want to show Instant results preview
for that query. This is how Instant has always worked and will work in the
future as well.

[Peter Kasting, 2012-08-10 20:12:24]

FWIW I think other places in the model that use GetText() may also be wrong.

I'd suggest the following pattern here:

* If |has_temporary_text_| is true, use the temporary text (probably passed
through DisplayTextFromUserText(), I'm not sure whether that will be necessary
to strip the keyword out).  There will never be inline autocompletion when this
bit is set.
* Otherwise use user_text_ and inline_autocomplete_text_.  See the pattern in
OnPopupDataChanged() where we pass both the user_text_ alone and with the inline
autocompletion to DisplayTextFromUserText().

Note that it's probably not safe to pass |inline_autocomplete_text_| directly to
instant.  It's much safer to pass the display text with and without completion
as in the second bullet above.  This is because we may mangle the text,
stripping off slashes and various other stuff, and you want instant to match
what the user is actually seeing.  So I strongly suggest making the instant API
take the args this way instead of taking an isolated inline autocompletion
string.

[sreeram, 2012-08-10 21:36:16]

On 2012/08/10 20:12:24, Peter Kasting wrote:
> I'd suggest the following pattern here:
> 
> * If |has_temporary_text_| is true, use the temporary text (probably passed
> through DisplayTextFromUserText(), I'm not sure whether that will be necessary
> to strip the keyword out).  There will never be inline autocompletion when
this
> bit is set.
> * Otherwise use user_text_ and inline_autocomplete_text_.  See the pattern in
> OnPopupDataChanged() where we pass both the user_text_ alone and with the
inline
> autocompletion to DisplayTextFromUserText().

Done. PTAL.

> Note that it's probably not safe to pass |inline_autocomplete_text_| directly
to
> instant.  It's much safer to pass the display text with and without completion
> as in the second bullet above.  This is because we may mangle the text,
> stripping off slashes and various other stuff, and you want instant to match
> what the user is actually seeing.  So I strongly suggest making the instant
API
> take the args this way instead of taking an isolated inline autocompletion
> string.

As discussed offline, I'll do this in a separate CL, since it involves changing
the API to Instant a little.

> FWIW I think other places in the model that use GetText() may also be wrong.

I'll leave this cleanup to you or somebody you delegate to. Thanks for the
review!

[Peter Kasting, 2012-08-10 21:39:10]

LGTM

http://codereview.chromium.org/10824261/diff/3002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10824261/diff/3002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1129: // If the user has
selected some other text (say from the drop down), use
Nit: Change to:

If there's temporary text, that overrides the user_text.  In this case we should
ignore any inline_autocomplete_text_, because it won't be visible.

[sreeram, 2012-08-10 22:34:13]

http://codereview.chromium.org/10824261/diff/3002/chrome/browser/ui/omnibox/o...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/10824261/diff/3002/chrome/browser/ui/omnibox/o...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1129: // If the user has
selected some other text (say from the drop down), use
On 2012/08/10 21:39:10, Peter Kasting wrote:
> Nit: Change to:
> 
> If there's temporary text, that overrides the user_text.  In this case we
should
> ignore any inline_autocomplete_text_, because it won't be visible.

Done.