Handle Esc key press event in Local NTP.

Handle Esc key press event in Local NTP.

If the user press 'Esc' key before 3 seconds timeout, we now send an
onkeypress(ESC) to the page. Local NTP handles the key press event and reloads the page contents.

BUG=231744, 238065
TEST=none

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

[kmadhusu, 2013-05-04 00:46:04]

PTAL.

Thanks.

[samarth, 2013-05-04 01:04:16]

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
chrome/browser/resources/local_ntp/local_ntp.js:1331: hideActiveSuggestions();
There are two cases here too :)
1) You type something and hit escape.  That works
2) You type something, arrow down and then hit escape.

#2 should only reset the selection state and not close the dropdown.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
chrome/browser/resources/local_ntp/local_ntp.js:1333: onThemeChange();
Not sure if this is the right way to go back to the NTP. I'll defer to Jeremy
for that.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:368: if (UsingLocalPage())
Let's send the right event for both local and remote.  We can fix the
server-side code too.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:369: OnEscKeyPressed();
Just call instant_tab_->OnEscKeyPressed() here. No need for the other function
below.

[kmadhusu, 2013-05-04 03:34:38]

Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
chrome/browser/resources/local_ntp/local_ntp.js:1331: hideActiveSuggestions();
On 2013/05/04 01:04:17, samarth wrote:
> There are two cases here too :)
> 1) You type something and hit escape.  That works
> 2) You type something, arrow down and then hit escape.
> 
> #2 should only reset the selection state and not close the dropdown.

Done.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/resources/loc...
chrome/browser/resources/local_ntp/local_ntp.js:1333: onThemeChange();
On 2013/05/04 01:04:17, samarth wrote:
> Not sure if this is the right way to go back to the NTP. I'll defer to Jeremy
> for that.

This call reloads the background theme if one exists. We actually show the NTP
by removing the HIDE_NTP class.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:368: if (UsingLocalPage())
On 2013/05/04 01:04:17, samarth wrote:
> Let's send the right event for both local and remote.  We can fix the
> server-side code too.

Done.

https://codereview.chromium.org/14562006/diff/10/chrome/browser/ui/search/ins...
chrome/browser/ui/search/instant_controller.cc:369: OnEscKeyPressed();
On 2013/05/04 01:04:17, samarth wrote:
> Just call instant_tab_->OnEscKeyPressed() here. No need for the other function
> below.

Done.

[samarth, 2013-05-04 16:37:35]

Working great!  Two requests:
(1) There is already a test for this but it's currently disabled because it was
in a bad state.  Please re-enable the test (and update if necessary).
(2) Please also update your commit description to mention that we now send an
onkeypress(ESC) to the page in this case.

Thanks,
Samarth

https://codereview.chromium.org/14562006/diff/12001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/12001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:361: if (full_text.empty())
Add a TODO here for sending an escape key press in both cases.

[jeremycho, 2013-05-04 23:07:06]

Demo is looking good, except for one corner case - 
If I start on the NTP and keep typing until the dropdown closes and then hit
enter, the bookmark bar pops in but the NTP isn't reshown.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1172: /**
nit: move this alongside the other selection functions.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1334: * Makes keys navigate
through suggestions.
Update comment.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1349: if
(activeBox.selectedIndex() != -1) {
Make this check a function and use it elsewhere.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1353:
document.body.classList.remove(CLASSES.HIDE_NTP);
Can you factor the show/hide logic into a helper function?

[jeremycho, 2013-05-05 19:51:11]

Sorry, by 'enter' I mean 'esc'.

On 2013/05/04 23:07:06, jeremycho wrote:
> Demo is looking good, except for one corner case - 
> If I start on the NTP and keep typing until the dropdown closes and then hit
> enter, the bookmark bar pops in but the NTP isn't reshown.
> 
>
https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
> File chrome/browser/resources/local_ntp/local_ntp.js (right):
> 
>
https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:1172: /**
> nit: move this alongside the other selection functions.
> 
>
https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:1334: * Makes keys navigate
> through suggestions.
> Update comment.
> 
>
https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:1349: if
> (activeBox.selectedIndex() != -1) {
> Make this check a function and use it elsewhere.
> 
>
https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:1353:
> document.body.classList.remove(CLASSES.HIDE_NTP);
> Can you factor the show/hide logic into a helper function?

[samarth, 2013-05-05 22:32:52]

+palmer for new IPC

Sorry for the last-minute review but hopefully this one should be simpler than
the last few :)

Thanks!
Samarth

[kmadhusu, 2013-05-05 22:44:54]

Addressed review comments. Enabled the interactive ui test.

PTAL.

Thanks.

https://codereview.chromium.org/14562006/diff/12001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/12001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:361: if (full_text.empty())
On 2013/05/04 16:37:35, samarth wrote:
> Add a TODO here for sending an escape key press in both cases.

Done.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1172: /**
On 2013/05/04 23:07:06, jeremycho wrote:
> nit: move this alongside the other selection functions.

Done.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1334: * Makes keys navigate
through suggestions.
On 2013/05/04 23:07:06, jeremycho wrote:
> Update comment.

Done.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1349: if
(activeBox.selectedIndex() != -1) {
On 2013/05/04 23:07:06, jeremycho wrote:
> Make this check a function and use it elsewhere.

Done.

https://codereview.chromium.org/14562006/diff/19001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1353:
document.body.classList.remove(CLASSES.HIDE_NTP);
On 2013/05/04 23:07:06, jeremycho wrote:
> Can you factor the show/hide logic into a helper function?

Done. Added showNtp().

[jeremycho, 2013-05-05 22:59:09]

Looking better!  One more issue -

If I start from the NTP, type a query, arrow down, arrow up into the omnibox
then press escape, the NTP is reshown but text remains in the omnibox.

[sreeram, 2013-05-05 23:03:50]

On 2013/05/05 22:59:09, jeremycho wrote:
> If I start from the NTP, type a query, arrow down, arrow up into the omnibox
> then press escape, the NTP is reshown but text remains in the omnibox.

This is not the local NTP's fault. It happens with the Google page as well. You
have to hit Escape twice:
https://code.google.com/p/chromium/issues/detail?id=175758#c5

[kmadhusu, 2013-05-05 23:40:15]

On 2013/05/05 23:03:50, sreeram wrote:
> On 2013/05/05 22:59:09, jeremycho wrote:
> > If I start from the NTP, type a query, arrow down, arrow up into the omnibox
> > then press escape, the NTP is reshown but text remains in the omnibox.
> 
> This is not the local NTP's fault. It happens with the Google page as well.
You
> have to hit Escape twice:
> https://code.google.com/p/chromium/issues/detail?id=175758#c5

sreeram@: Thanks for the details.
samarth@: New test failed on mac_rel try bot. I am not sure if it is flaky. If
it is failing consistently on Mac, can I disable it for now and re-enable it
after the branch cut?

[samarth, 2013-05-05 23:49:30]

Do you have a link to the failing test output?  I also want to get this in
before the cut but we have to wait for a security review so we have some time to
figure out the test :)

Thanks!
Samarth

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1934: // Use
only the local overlay.
This shouldn't be necessary given the call to SetInstantEnabled below.  Also,
why force the local only page at all?  We can't really test (or want to test)
its behavior in this test.

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1962:
EXPECT_EQ("", GetOmniboxText());
This should be true, but isn't really testing everything.  We should test that
we sent a keypress event.  Generally we do this in tests by counting the IPCs in
the test js file.  See UpdateSearchState() and the test instant file
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/i...).

[kmadhusu, 2013-05-06 00:11:16]

samarth@: Test timed out on Mac_rel try bot. Link:
http://build.chromium.org/p/tryserver.chromium/builders/mac_rel/builds/125419...

I will investigate more.

Thanks.

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1934: // Use
only the local overlay.
On 2013/05/05 23:49:30, samarth wrote:
> This shouldn't be necessary given the call to SetInstantEnabled below.  Also,
> why force the local only page at all?  We can't really test (or want to test)
> its behavior in this test.

hmm. No specific reason to force this test for local only page. I will
investigate more and update this test.

https://codereview.chromium.org/14562006/diff/31001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:1962:
EXPECT_EQ("", GetOmniboxText());
On 2013/05/05 23:49:30, samarth wrote:
> This should be true, but isn't really testing everything.  We should test that
> we sent a keypress event.  Generally we do this in tests by counting the IPCs
in
> the test js file.  See UpdateSearchState() and the test instant file
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/i...).

I completely agree. This test can be improved. I will refer to
UpdateSearchState() and will update the test.

[palmer, 2013-05-06 18:47:57]

lgtm

[kmadhusu, 2013-05-06 19:01:54]

samarth@: Fixed interactive ui test. PTAL.

Thanks.

[samarth, 2013-05-06 19:13:37]

Please wait for Jeremy to look at the JS changes.  But instant and the test
LGTM.

Thanks,
Samarth

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:386:
instant_tab_->Update(string16(), 0, 0, true);
Please add a comment for this call: because that we want to clear the query for
the page at this point.

[jeremycho, 2013-05-06 19:57:10]

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1684: var value =
searchboxApiHandle.value;
This shouldn't be needed anymore.  Instead, you should be able to just register
an empty handler to inform Chrome that the NTP is instant-extended capable.

[sreeram, 2013-05-06 20:00:32]

lgtm

[kmadhusu, 2013-05-06 20:24:44]

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1684: var value =
searchboxApiHandle.value;
On 2013/05/06 19:57:10, jeremycho wrote:
> This shouldn't be needed anymore.  Instead, you should be able to just
register
> an empty handler to inform Chrome that the NTP is instant-extended capable.

Done.

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:386:
instant_tab_->Update(string16(), 0, 0, true);
On 2013/05/06 19:13:37, samarth wrote:
> Please add a comment for this call: because that we want to clear the query
for
> the page at this point.

Done.

[jeremycho, 2013-05-06 20:32:34]

lgtm

https://codereview.chromium.org/14562006/diff/68001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (left):

https://codereview.chromium.org/14562006/diff/68001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1643:
searchboxApiHandle.onsubmit = function() {
Can you add a comment explaining this is needed to inform Chrome that the NTP is
instant-extended capable?

[kmadhusu, 2013-05-06 21:11:44]

https://codereview.chromium.org/14562006/diff/68001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (left):

https://codereview.chromium.org/14562006/diff/68001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1643:
searchboxApiHandle.onsubmit = function() {
On 2013/05/06 20:32:34, jeremycho wrote:
> Can you add a comment explaining this is needed to inform Chrome that the NTP
is
> instant-extended capable?

Done.

[samarth, 2013-05-06 21:16:26]

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1684: var value =
searchboxApiHandle.value;
On 2013/05/06 20:24:44, kmadhusu wrote:
> On 2013/05/06 19:57:10, jeremycho wrote:
> > This shouldn't be needed anymore.  Instead, you should be able to just
> register
> > an empty handler to inform Chrome that the NTP is instant-extended capable.
> 
> Done.

Isn't this necessary to hide the dropdown right away when you hit enter on a
URL.

[jeremycho, 2013-05-06 21:24:07]

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1684: var value =
searchboxApiHandle.value;
On 2013/05/06 21:16:26, samarth wrote:
> On 2013/05/06 20:24:44, kmadhusu wrote:
> > On 2013/05/06 19:57:10, jeremycho wrote:
> > > This shouldn't be needed anymore.  Instead, you should be able to just
> > register
> > > an empty handler to inform Chrome that the NTP is instant-extended
capable.
> > 
> > Done.
> 
> Isn't this necessary to hide the dropdown right away when you hit enter on a
> URL.

Yeah, sorry I thought it was handled elsewhere.  Can you revert but update the
comment?

[kmadhusu, 2013-05-06 21:47:32]

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14562006/diff/58001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1684: var value =
searchboxApiHandle.value;
On 2013/05/06 21:24:07, jeremycho wrote:
> On 2013/05/06 21:16:26, samarth wrote:
> > On 2013/05/06 20:24:44, kmadhusu wrote:
> > > On 2013/05/06 19:57:10, jeremycho wrote:
> > > > This shouldn't be needed anymore.  Instead, you should be able to just
> > > register
> > > > an empty handler to inform Chrome that the NTP is instant-extended
> capable.
> > > 
> > > Done.
> > 
> > Isn't this necessary to hide the dropdown right away when you hit enter on a
> > URL.
> 
> Yeah, sorry I thought it was handled elsewhere.  Can you revert but update the
> comment?

Reverted the changes.

[kmadhusu, 2013-05-06 21:57:38]

(repeating my offline conversation with samarth for reference):
InstantExtendedTest.EscapeClearsOmnibox is timing out on all try bots. I am
going to disable this test for now. Will investigate and re-enable it after the
branch cut.

[commit-bot: I haz the power, 2013-05-06 21:58:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/14562006/79002

[commit-bot: I haz the power, 2013-05-07 03:02:04]

Change committed as 198598