[Search] Avoid crash when hitting Esc in the Omnibox

Avoid crashing when a suggestion from Instant is set in the Omnibox and ESC is hit.

Now checking for is_temporary_text_set_by_instant when hitting ESC. If suggestion was set by a REPLACE event from Instant, do not check the AutocompleteController as it may not necessarily be running (cause of the crash).

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

[Mathieu, 2013-01-04 19:08:04]

Hi Samarth, please take a look.

I've tested this and behavior is fine (i.e. ESC still reverts text, but no CHECK
is hit).

[samarth, 2013-01-07 17:47:14]

This looks good if it fixes the crash.

However, escape behavior is currently inconsistent and broken. In non-1993 if
you arrow down, the first escape restores your original typed text and the
second escape cancels the search completely.

With 1993:
(a) If you're on non-search page and arrow down to a suggestion, the first
escape stops autocomplete.
(b) If you're on a search page, sometimes the first escape just moves the cursor
and the second escape cancels. Other times it works as in (a).

The two-phased escape behavior is nice but I'd be happy at first if the 1993
escape behavior was consistent.

Thanks,
Samarth

[MAD, 2013-01-09 02:15:58]

I validated that the fix resolved the crash, but I'm not sure if it changes the
behavior or if Samarth's comment above is about the current 1993 behavior that
should be changed, or if it's a side effect of this fix. If it is not a side
effect of this fix, than we should commit the fix and resolve the other escape
issues as part of another CL.

[gideonwald1, 2013-01-09 06:56:53]

Samarth, can you comment? We can also wait for Mathieu to be back next week
if that's easier.


On Tue, Jan 8, 2013 at 6:15 PM, <mad@chromium.org> wrote:

> I validated that the fix resolved the crash, but I'm not sure if it
> changes the
> behavior or if Samarth's comment above is about the current 1993 behavior
> that
> should be changed, or if it's a side effect of this fix. If it is not a
> side
> effect of this fix, than we should commit the fix and resolve the other
> escape
> issues as part of another CL.
>
>
>
https://codereview.chromium.**org/11644050/<https://codereview.chromium.org/1...
>

[MAD, 2013-01-09 20:57:30]

Ping?

[samarth, 2013-01-09 21:03:58]

LGTM

Sorry, missed your comment yesterday.  This looks OK to me but you should
probably an omnibox OWNER review as well.

Samarth

[MAD, 2013-01-10 21:00:13]

D'Ho! I wasn't listed as a reviewer. so I never got your answer...

Adding pkasting@ for an OWNERS review...

Thanks!

BYE
MAD

[Mathieu, 2013-01-11 15:40:08]

Adding sky@ as pkasting@ seems to be out.

This indeed only fixes the crash and does not change behavior, for which other
bugs are filed.

Thanks!

[sky, 2013-01-11 17:17:53]

Can you upload again, the side by side diff doesn't show up.

[Mathieu, 2013-01-11 18:00:15]

Done. Thanks.

[sky, 2013-01-11 18:58:10]

LGTM - how about a test?

[Mathieu, 2013-01-11 19:26:11]

On 2013/01/11 18:58:10, sky wrote:
> LGTM - how about a test?

Certainly, Samarth is writing a test framework as we speak for Instant Extended
(https://code.google.com/p/chromium/issues/detail?id=167359). Once it's done
I'll write tests for my features.

Thanks!

[commit-bot: I haz the power, 2013-01-11 19:27:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mathp@chromium.org/11644050/24001

[commit-bot: I haz the power, 2013-01-12 15:57:27]

Change committed as 176531