Fixing ESC in instant-extended.

Fixing ESC in instant-extended.

This ensures Chrome sends an onkeypress VKEY_ESCAPE to the instant extended page when the user presses ESC after having arrowed down in the suggestion list.

This patch also ensures that no Update is fired when the user presses ESC. It also revisit the fix for crbug.com/172382 .

The JS still needs to reset the selection to the default suggestion this is tracked in b/8172172 .

BUG=164449
TEST=Enable instant extended, type a search query (ie "flowe") arrow down in the suggestions, hit ESC. The popup should close and "flowe" should remain in the omnibox.

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

[beaudoin, 2013-01-14 02:39:08]

Even though it looks like a revert of https://codereview.chromium.org/11644050
I've verified that this not reintroduce crbug.com/167005 fixed by mathp. (I
believe the problem was calling popup_->ResetToDefaultMatch on an unexisting
popup.)

The biggest problem with the current patch is the workaround for the missing
ResetToDefaultMatch in the instant API. Please discuss it on the issue.

[beaudoin, 2013-02-07 22:34:59]

+asvitkine because I changed your fix of crbug.com/172382 by ensuring
OmniboxEditModel::OnChanged is never called when an INSTANT_COMPLETE_REPLACE
comes in. Please check it makes sense. :)

David, Sreeram:

This patch ensures that, whenever ESC is pressed after the user arrowed down in
the suggestion list:
1) Chrome resets the omnibox to the initial user_text
2) Chrome DOES NOT call Update on the JS
3) Chrome sends an onkeypress with VKEY_ESCAPE down to the JS.

Doing (2) required me to add a parameter to a bunch of platform-specific
omnibox_view code otherwise it was impossible to know the call to
OnTemporaryTextMaybeChanged or OnRevertTemporaryText should not trigger an
OnUpdate (Alexei's fix of checking some boolean state variables did not work in
this case, since these variables are reset prior to the call).

This does not currently lead to very nice behavior because the prod JS closes
the popup when it receives VKEY_ESCAPE (instead of leaving it open and resetting
the selection), and the JS and Chrome state get out of sync. Once the JS is
fixed to just reset to the first selection it should be fine.

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (left):

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:604: TextChanged();
Shamelessly getting rid of that (see explanation in omnibox_view_views.cc).

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1257: 
This basically reverts Alexei's fix of crbug.com/172382

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:223: save_original_selection,
false);
This is my new way of fixing the problem Alexei solved in crbug.com/172382

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11889003/diff/12002/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:611: TextChanged();
Shamelessly getting rid of that. OnRevertTemporaryText only changes the
selection range and it's only ever called from OmniboxEditModel so there is no
point in letting OmniboxEditModel know of the change. Moreover, the mac version
never called TextChanged().

This was required otherwise an undesired call to OnUpdate would be triggered.

[beaudoin, 2013-02-11 15:16:54]

Reviewers:
asvitkine forgot you last time around (pls read previous message)

Owners:
sreeram   for c/b/instant/*  and  c/renderer/searchbox/*
pkasting  for c/b/ui/omnibox/*  and  c/b/ui/views/**
asvitvine for c/b/ui/cocoa/**
estade    for c/b/ui/gtk/**
palmer    for c/common/*  (security review)

[Alexei Svitkine (slow), 2013-02-11 17:48:37]

LGTM for cocoa/

please add a TEST= line to the CL description

[Alexei Svitkine (slow), 2013-02-11 17:54:43]

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: return false;
On my CL that added this block, Sreeram expressed concern that the logic in
OnChanged() should still run (hence why this if statement wasn't added to
OnChanged(), which was the case in an earlier patchset of mine).

Please double check with him that it makes sense not to run the rest of the
OnChanged() logic in the case where you're passing false now.

[beaudoin, 2013-02-11 18:00:53]

On 2013/02/11 17:54:43, Alexei Svitkine wrote:
>
https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):
> 
>
https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: return false;
> On my CL that added this block, Sreeram expressed concern that the logic in
> OnChanged() should still run (hence why this if statement wasn't added to
> OnChanged(), which was the case in an earlier patchset of mine).
> 
> Please double check with him that it makes sense not to run the rest of the
> OnChanged() logic in the case where you're passing false now.

My rationale here was that, before my fix in 
  https://chromiumcodereview.appspot.com/11827060
OnChanged wasn't called, and I don't think it caused any problem. (My fix just
made sure the view's state was updated.)

[Peter Kasting, 2013-02-11 19:54:54]

I don't understand how instant works well enough to review this :(

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:223: save_original_selection,
false);
Why false here?

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1185: // Update the view text,
but ensure
Comment deadends

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:611: TextChanged();
Why is removing this correct?

[jeremycho, 2013-02-11 22:29:11]

The value of chrome.searchBox.value doesn't seem to get updated on ESC. 
Expected?

E.g. if you type [flowe], arrow down to [flowers for algernon] then hit ESC,
it's still set to 'flowers for algernon'.

As a result when you commit the query, you get results for 'flowers for
algernon'.

[beaudoin, 2013-02-11 23:38:21]

@jeremycho Latest version no longer relies on the JS setting the value (the IPC
sets it), so it should solve your problem. I also set verbatim=true but I'm not
sure it's the right thing to do.

@pkasting Can you take a look at c/b/ui/view/omnibox/omnibox_view_win.* I forget
them in the previous round.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: return false;
On 2013/02/11 17:54:43, Alexei Svitkine wrote:
> On my CL that added this block, Sreeram expressed concern that the logic in
> OnChanged() should still run (hence why this if statement wasn't added to
> OnChanged(), which was the case in an earlier patchset of mine).
> 
> Please double check with him that it makes sense not to run the rest of the
> OnChanged() logic in the case where you're passing false now. 

See comment in main thread.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1185: // Update the view text,
but ensure
On 2013/02/11 19:54:54, Peter Kasting wrote:
> Comment deadends

Done.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:611: TextChanged();
On 2013/02/11 19:54:54, Peter Kasting wrote:
> Why is removing this correct?

OnRevertTemporaryText only changes the
selection range, and this only seem to happen following a real change that has
itself triggered TextChanged(). As far as I can tell the code under
TextChanged() does not care about a selection change. Also, the mac version of
OnRevertTemporaryText() never called TextChanged().

The alternative to removing it is adding a notify_text_changed boolean parameter
here too. I just thought that it was not required.

[Peter Kasting, 2013-02-11 23:53:08]

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:223: save_original_selection,
false);
On 2013/02/11 19:54:54, Peter Kasting wrote:
> Why false here?

Didn't answer this.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1185: // Update the view text,
but ensure
On 2013/02/11 23:38:22, beaudoin wrote:
> On 2013/02/11 19:54:54, Peter Kasting wrote:
> > Comment deadends
> 
> Done.

No, it wasn't.

[beaudoin, 2013-02-12 00:24:37]

Sorry Peter, comment update fell through the cracks as I switched it from Mac to
Linux. Should be all good now.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: return false;
On 2013/02/11 23:38:22, beaudoin wrote:
> On 2013/02/11 17:54:43, Alexei Svitkine wrote:
> > On my CL that added this block, Sreeram expressed concern that the logic in
> > OnChanged() should still run (hence why this if statement wasn't added to
> > OnChanged(), which was the case in an earlier patchset of mine).
> > 
> > Please double check with him that it makes sense not to run the rest of the
> > OnChanged() logic in the case where you're passing false now. 
> 
> See comment in main thread.

After some testing it reintroduced the bug I originally fixed, so I went back to
your fix.

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:223: save_original_selection,
false);
On 2013/02/11 23:53:09, Peter Kasting wrote:
> On 2013/02/11 19:54:54, Peter Kasting wrote:
> > Why false here?
> 
> Didn't answer this.

I've switched it back to true. I thought using false would let us get rid of an
awkward check we have in DoInstant but it reintroduced an old bug...

https://codereview.chromium.org/11889003/diff/12004/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1185: // Update the view text,
but ensure
On 2013/02/11 23:53:09, Peter Kasting wrote:
> On 2013/02/11 23:38:22, beaudoin wrote:
> > On 2013/02/11 19:54:54, Peter Kasting wrote:
> > > Comment deadends
> > 
> > Done.
> 
> No, it wasn't.

Sorry. Fell through the cracks. Done now, for real.

[jeremycho, 2013-02-12 00:49:10]

Thanks for making the value change.

However, I'm noticing an issue with setAutocompleteText.  When I call it after
arrowing down and hitting escape, it doesn't seem to update the grey Omnibox
text.  Any idea why this might be?

[jeremycho, 2013-02-12 01:33:34]

Another issue I'm seeing - If I arrow down to a query suggestion X, hit Escape
and then Enter, searchBox.value reverts back to suggestion X before the query is
committed.

[Peter Kasting, 2013-02-12 01:36:18]

OWNERS LGTM, but that's basically a rubber-stamp contingent on
instant-knowledgeable people OKing this and it not causing any problems.

[Evan Stade, 2013-02-12 03:27:01]

rubber stamp lgtm for gtk

[sreeram, 2013-02-12 18:12:29]

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:114: // Called when the user has
arrowed into the selections but want to cancel,
selections -> suggestions
want -> wants

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:116: // reverted to |user_text| by
Chrome prior to calling this.
Chrome -> the OmniboxEditModel

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:117: bool OnCancel(string16
user_text);
const string16&
The return value doesn't seem to be used by OmniboxEditModel, so why not "void"?

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:153: void CancelSelection(string16
user_text);
const string16&

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1188:
instant->OnCancel(user_text_);
It took me some time to figure out what's going on here. I think this warrants a
more detailed comment. Something like:

"""
Normally, popup_->ResetToDefaultMatch() will cause the view text to be updated.
In Instant Extended mode however, the popup_ is not used, so it won't do
anything. So, update the view ourselves. Even if Instant is not in extended mode
(i.e., it's enabled in non-extended mode, or disabled altogether), this is okay
to do, since the call to popup_->ResetToDefaultMatch() will just override
whatever we do here.

The two "false" arguments make sure that our shenanigans don't cause any
previously saved selection to be erased nor OnChanged() to be called.
"""

Which raises the questions:
1. Why not use view_->SetWindowTextAndCaretPos() instead of
OnTemporaryTextMaybeChanged() here (and then do an early return)?
2. Maybe do this only if "is_temporary_text_set_by_instant_" was true in the
first place?

[beaudoin, 2013-02-12 20:27:30]

Sreeram: PTAL.
Jeremy: I fixed the problem where committing the result after hitting ESC would
cause the previous suggestion to be used. I have not been able to repro the
problem where setAutocompleteText did not work. If you can't make it work ping
me and we'll try to see where the problem is.

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:114: // Called when the user has
arrowed into the selections but want to cancel,
On 2013/02/12 18:12:29, sreeram wrote:
> selections -> suggestions
> want -> wants

Done.

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:116: // reverted to |user_text| by
Chrome prior to calling this.
On 2013/02/12 18:12:29, sreeram wrote:
> Chrome -> the OmniboxEditModel

Done.

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:117: bool OnCancel(string16
user_text);
On 2013/02/12 18:12:29, sreeram wrote:
> const string16&
> The return value doesn't seem to be used by OmniboxEditModel, so why not
"void"?

Done.

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:153: void CancelSelection(string16
user_text);
On 2013/02/12 18:12:29, sreeram wrote:
> const string16&

Done.

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/12005/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1188:
instant->OnCancel(user_text_);
On 2013/02/12 18:12:29, sreeram wrote:
> It took me some time to figure out what's going on here. I think this warrants
a
> more detailed comment. Something like:
> 
> """
> Normally, popup_->ResetToDefaultMatch() will cause the view text to be
updated.
> In Instant Extended mode however, the popup_ is not used, so it won't do
> anything. So, update the view ourselves. Even if Instant is not in extended
mode
> (i.e., it's enabled in non-extended mode, or disabled altogether), this is
okay
> to do, since the call to popup_->ResetToDefaultMatch() will just override
> whatever we do here.
> 
> The two "false" arguments make sure that our shenanigans don't cause any
> previously saved selection to be erased nor OnChanged() to be called.
> """
> 
> Which raises the questions:
> 1. Why not use view_->SetWindowTextAndCaretPos() instead of
> OnTemporaryTextMaybeChanged() here (and then do an early return)?
> 2. Maybe do this only if "is_temporary_text_set_by_instant_" was true in the
> first place?

Now using your comment.
1) --> discussed over chat, leaving it like this.
2) I've gated that code on the original value of
is_temporary_text_set_by_instant_

[jeremycho, 2013-02-12 20:47:20]

On 2013/02/12 20:27:30, beaudoin wrote:
> Jeremy: I fixed the problem where committing the result after hitting ESC
would
> cause the previous suggestion to be used. I have not been able to repro the
> problem where setAutocompleteText did not work. If you can't make it work ping
> me and we'll try to see where the problem is.

Great - I've confirmed the previous suggestion fix.

The setAutocompleteText issue is only reproducible for me when the top
suggestion produces a blue highlight in the omnibox, not grey textahead.  E.g.

1. Type [face] - should see facebook.com as top suggestion with 'book.com' as
blue highlight.
2. Arrow down to 'facebook login' suggestion and hit ESC.
3. In console, type 'chrome.searchBox.setAutocompleteText('facebook.com', 2)'

Expected - the blue highlight should be recovered, but instead it's a no-op.

It works fine if the top suggestion was a query suggestion, e.g.:
1. Type [x] - see 'xbox' as top suggestion with 'box' as grey textahead.
2. Arrow down to 'xfinity' and hit ESC.
3. In console, type 'chrome.searchBox.setAutocompleteText('xbox', 1)'

Here I see the grey textahead.

Let me know if you're having trouble reproducing.  Thanks!

[sreeram, 2013-02-12 20:52:59]

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:520: last_match_was_search_ =
!user_text.empty();
This isn't totally correct. Consider: I type [face], and get an inline
autocompletion for [face|book.com], and then arrow down to something else and
hit Escape.

Hopefully, the page will come back and set the blue text again before the user
hits Enter, so it will work most of time.

Gah. Just add a "TODO(sreeram): Handle the case where user_text is now a URL".
I'm trying to make such problems go away completely in my rewrite.

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:604: TextChanged();
Why remove this? Won't this break non-extended mode?

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1186: // Mormally,
popup_->ResetToDefaultMatch() will cause the view text to be
Mormally? :)

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:611: TextChanged();
Same comment as the gtk file: Why remove this?

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:901: TextChanged();
-ditto-

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:234: void
SearchBox::OnCancelSelection(string16 query) {
const string16&

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:237: selection_start_ = selection_end_ =
query_.size();
Similarly, these are also somewhat wrong. Just add more TODOs to fix them later.

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:89: void OnCancelSelection(string16
query);
const string16&

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:145: // Takes one printf-style
replaceable values: key_code.
values -> value

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:53: suggestionIndex = -1;
Huh? It's already -1, isn't it?

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:71:
apiHandle.setAutocompleteText("helloab", 2)
???

[sreeram, 2013-02-12 20:56:51]

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1195:
view_->OnTemporaryTextMaybeChanged(user_text_, false, false);
You need to set this to user_text_ + inline_autocomplete_text_ to fix the blue
highlight issue Jeremy pointed out.

[beaudoin, 2013-02-12 21:34:41]

Jeremy: I was able to repro but I suspect the problem is related to the state
(either in SearchBox or InstantController) not being reset correctly for the
case where the suggestion is a URL. I suggest filing a separate bug for that
case, since Sreeram's refactor looks like it could solve that. What do you
think, Sreeram?

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:520: last_match_was_search_ =
!user_text.empty();
On 2013/02/12 20:52:59, sreeram wrote:
> This isn't totally correct. Consider: I type [face], and get an inline
> autocompletion for [face|book.com], and then arrow down to something else and
> hit Escape.
> 
> Hopefully, the page will come back and set the blue text again before the user
> hits Enter, so it will work most of time.
> 
> Gah. Just add a "TODO(sreeram): Handle the case where user_text is now a URL".
> I'm trying to make such problems go away completely in my rewrite.

Added TODO.

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/gtk/omn...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/gtk/omn...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:604: TextChanged();
On 2013/02/12 20:52:59, sreeram wrote:
> Why remove this? Won't this break non-extended mode?


OnRevertTemporaryText only changes the selection range, and this only seem to
happen following a real change that has itself triggered TextChanged(). As far
as I can tell the code under TextChanged() does not care about a selection
change. Also, the mac version of OnRevertTemporaryText() never called
TextChanged().

The alternative to removing it is adding a notify_text_changed boolean parameter
here too. I just thought that it was not required.

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1186: // Mormally,
popup_->ResetToDefaultMatch() will cause the view text to be
On 2013/02/12 20:52:59, sreeram wrote:
> Mormally? :)

:}
Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1195:
view_->OnTemporaryTextMaybeChanged(user_text_, false, false);
On 2013/02/12 20:56:51, sreeram wrote:
> You need to set this to user_text_ + inline_autocomplete_text_ to fix the blue
> highlight issue Jeremy pointed out.

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:611: TextChanged();
On 2013/02/12 20:52:59, sreeram wrote:
> Same comment as the gtk file: Why remove this?

Same answer. :)

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (left):

https://codereview.chromium.org/11889003/diff/15035/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:901: TextChanged();
On 2013/02/12 20:52:59, sreeram wrote:
> -ditto-

/cut-and-paste :)

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:234: void
SearchBox::OnCancelSelection(string16 query) {
On 2013/02/12 20:52:59, sreeram wrote:
> const string16&

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:237: selection_start_ = selection_end_ =
query_.size();
On 2013/02/12 20:52:59, sreeram wrote:
> Similarly, these are also somewhat wrong. Just add more TODOs to fix them
later.

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:89: void OnCancelSelection(string16
query);
On 2013/02/12 20:52:59, sreeram wrote:
> const string16&

Dammit! Forgot one.

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:145: // Takes one printf-style
replaceable values: key_code.
On 2013/02/12 20:52:59, sreeram wrote:
> values -> value

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
File chrome/test/data/instant_extended.html (right):

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:53: suggestionIndex = -1;
On 2013/02/12 20:52:59, sreeram wrote:
> Huh? It's already -1, isn't it?

Done.

https://codereview.chromium.org/11889003/diff/15035/chrome/test/data/instant_...
chrome/test/data/instant_extended.html:71:
apiHandle.setAutocompleteText("helloab", 2)
On 2013/02/12 20:52:59, sreeram wrote:
> ???

Debugging leftovers. :(

Done.

[sreeram, 2013-02-12 21:52:03]

On Tue, Feb 12, 2013 at 1:34 PM,  <beaudoin@chromium.org> wrote:
> Jeremy: I was able to repro but I suspect the problem is related to the
> state
> (either in SearchBox or InstantController) not being reset correctly for the
> case where the suggestion is a URL. I suggest filing a separate bug for that
> case, since Sreeram's refactor looks like it could solve that. What do you
> think, Sreeram?

There are a couple of things going on here. The blue highlight text
comes about because InstantController::SetSuggestions ends up in
SearchProvider::FinalizeInstantQuery. This is allowed to be called
only once per autocomplete run. So, when you initially got
"face|book.com", that's what happened.

Now, you arrow down and hit Escape. The page will try to
SetSuggestions() again. This will cause the InstantController to
update its last_omnibox_text_ (actually it doesn't do it correctly
now, but I'll fix that; perhaps you can too). However, the call to
FinalizeInstantQuery() will be rejected, because no new autocomplete
run has happened.

So, the omnibox still shows just "face" (which is the user_text_).
This is why I said that you have to use "user_text_ +
inline_autocomplete_text_".

There's another whole can of worms with keyword search mode. Have you
tried entering keyword mode, then typing something, and then hitting
Escape?

[beaudoin, 2013-02-12 22:20:14]

I think I was able to solve Jeremy's issue by using user_text_ +
inline_autocomplete_text_ in the call to view_->OnTemporaryTextMaybeChanged

I've only notices one small problem with keyword search:
- say you have "bgg" as a keyword
- arrow-down through the suggestions
- then hit ESC
You go back to the omnibox with "bgg" but you can no longer
tab-to-keyword-search (and you don't have the keyword hint either). I suggest
fixing this elsewhere. If you agree, I'll enter an issue for it.

[jeremycho, 2013-02-12 22:24:16]

On 2013/02/12 22:20:14, beaudoin wrote:
> I think I was able to solve Jeremy's issue by using user_text_ +
> inline_autocomplete_text_ in the call to view_->OnTemporaryTextMaybeChanged

Tried the latest patch and seems to be fixed.

[beaudoin, 2013-02-12 22:54:48]

Fixed an issue where typing "face|book.com", arrowing down, hitting ESC, then
hitting Enter to commit would search for facebook.com instead of navigating to
it.

[beaudoin, 2013-02-13 20:11:32]

On 2013/02/12 22:54:48, beaudoin wrote:
> Fixed an issue where typing "face|book.com", arrowing down, hitting ESC, then
> hitting Enter to commit would search for http://facebook.com instead of
navigating to
> it.

Friendly ping.

Sreeram: For instant logic
palmer: For c/common/* (security review)

[palmer, 2013-02-13 21:27:54]

lgtm

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:235: // TODO(sreeram): The state reset
below are somewhat wrong. Fix them.
I believe it is better to cite a bug # in TODOs.

[beaudoin, 2013-02-13 21:32:38]

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:235: // TODO(sreeram): The state reset
below are somewhat wrong. Fix them.
On 2013/02/13 21:27:54, Chris P. wrote:
> I believe it is better to cite a bug # in TODOs.

Done.

[sreeram, 2013-02-14 17:56:41]

lgtm

https://codereview.chromium.org/11889003/diff/32001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (left):

https://codereview.chromium.org/11889003/diff/32001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:525: 
Nit: Don't remove this blank line.

[commit-bot: I haz the power, 2013-02-14 21:16:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11889003/34002

[Peter Kasting, 2013-02-14 21:17:49]

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11889003/diff/28002/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:235: // TODO(sreeram): The state reset
below are somewhat wrong. Fix them.
On 2013/02/13 21:32:38, beaudoin wrote:
> On 2013/02/13 21:27:54, Chris P. wrote:
> > I believe it is better to cite a bug # in TODOs.
> 
> Done.

Actually, I ask all my reviewees to not cite bug numbers in comments.  The
comment should give the complete context for the issue on its own.  Linking to
bugs is hazardous, in particular when the bug metastasizes and someone trying to
read it isn't sure what the relevant portion is for the source code in question
any more, and even when that doesn't happen, it's inconvenient to have to go
find and read another bug rather than having all the data there (or to read the
other bug only to find it adds nothing to the comment).

[commit-bot: I haz the power, 2013-02-14 22:13:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11889003/34002

[commit-bot: I haz the power, 2013-02-14 23:46:20]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-15 14:43:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11889003/36008

[commit-bot: I haz the power, 2013-02-15 14:43:41]

Failed to apply patch for chrome/browser/instant/instant_page.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/instant/instant_page.cc
  Hunk #1 FAILED at 63.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/instant/instant_page.cc.rej

Patch:       chrome/browser/instant/instant_page.cc
Index: chrome/browser/instant/instant_page.cc
diff --git a/chrome/browser/instant/instant_page.cc
b/chrome/browser/instant/instant_page.cc
index
8343975f567aaa52896a14e50c1afb7072ca7874..68969ebd35dda8f1b9002f59d03bd3909666e5d2
100644
--- a/chrome/browser/instant/instant_page.cc
+++ b/chrome/browser/instant/instant_page.cc
@@ -63,6 +63,10 @@ void InstantPage::UpOrDownKeyPressed(int count) {
   Send(new ChromeViewMsg_SearchBoxUpOrDownKeyPressed(routing_id(), count));
 }
 
+void InstantPage::CancelSelection(const string16& user_text) {
+  Send(new ChromeViewMsg_SearchBoxCancelSelection(routing_id(), user_text));
+}
+
 void InstantPage::SearchModeChanged(const chrome::search::Mode& mode) {
   Send(new ChromeViewMsg_SearchBoxModeChanged(routing_id(), mode));
 }

[commit-bot: I haz the power, 2013-02-15 14:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/11889003/47001

[commit-bot: I haz the power, 2013-02-15 17:35:58]

Change committed as 182744