Fix pressing enter key crashes overview mode without a selection.

Fix pressing enter key crashes overview mode without a 
selection. 
This change adds a check before trying to select 
the window with the enter key that verifies that the 
selection is active. If it is not, pressing it has no 
effect.

BUG=381811
TEST=WindowSelectorTest.SelectWindowWithReturnKey

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

[Nina, 2014-06-06 19:43:44]

Hi Rob, Terry,

Mind giving this patch a look? Thanks!

[tdanderson, 2014-06-06 21:15:18]

Nico, please see my comments below.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:221: if
(grid_list_[selected_grid_index_]->is_selecting()) {
A short comment would be nice to say something like "ignore if no item is
selected"

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:224: }
I think you will want to set handled = false; in the case where we're just
ignoring the VKEY_RETURN keypress.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:866: gfx::Rect bounds1(0, 0, 100,
100);
nit: bounds, not bounds1.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:867: scoped_ptr<aura::Window>
window2(CreateWindow(bounds1));
nit: declare window1 before window2

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:878: ASSERT_FALSE(IsSelecting());
EXPECT_FALSE? (ASSERT_FALSE will crash the test at this point if it fails. Is
that intentional?)

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:886:
EXPECT_TRUE(wm::IsActiveWindow(window2.get()));
Can you also add an EXPECT for IsSelecting() here too?

[Nina, 2014-06-06 21:44:33]

OK, took care of comments and changed SetHandled for StopPropagation in the key
event hander because the "enter" keypress was going through to the window.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:221: if
(grid_list_[selected_grid_index_]->is_selecting()) {
On 2014/06/06 21:15:17, tdanderson wrote:
> A short comment would be nice to say something like "ignore if no item is
> selected"

Done.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:224: }
On 2014/06/06 21:15:17, tdanderson wrote:
> I think you will want to set handled = false; in the case where we're just
> ignoring the VKEY_RETURN keypress.

Done.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:866: gfx::Rect bounds1(0, 0, 100,
100);
On 2014/06/06 21:15:18, tdanderson wrote:
> nit: bounds, not bounds1.

Done.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:867: scoped_ptr<aura::Window>
window2(CreateWindow(bounds1));
On 2014/06/06 21:15:18, tdanderson wrote:
> nit: declare window1 before window2

The reason the declaration is in that order is so that, in overview mode, they
are ordered:
+-------+ +-------+
|window1| |window2|
+-------+ +-------+

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:878: ASSERT_FALSE(IsSelecting());
On 2014/06/06 21:15:17, tdanderson wrote:
> EXPECT_FALSE? (ASSERT_FALSE will crash the test at this point if it fails. Is
> that intentional?)

I want the test to stop if that fails as the outcome of the following 
assertions would be pointless.

https://codereview.chromium.org/324493004/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_unittest.cc:886:
EXPECT_TRUE(wm::IsActiveWindow(window2.get()));
On 2014/06/06 21:15:17, tdanderson wrote:
> Can you also add an EXPECT for IsSelecting() here too?

Sure.

[tdanderson, 2014-06-06 21:49:59]

lgtm, over to Rob.

[flackr, 2014-06-06 22:01:04]

lgtm with nit: I'd prefer to see stop propagation when handled rather than
setting the bool which eventually calls stop propagation.

Your comment about why you're using stop propagation suggests there's going to
be an open bug with the window handling other key events and that the window is
still focused. Can you file a bug and investigate.

[Nina, 2014-06-06 22:17:21]

Don't worry flackr, this only happens for the enter key, when it selects the
window, because it's being focused and then gets the event. It doesn't happen
with other keys because the windows don't have focus.

[tdanderson, 2014-06-06 22:19:11]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-06 22:20:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nsatragno@chromium.org/324493004/60001

[commit-bot: I haz the power, 2014-06-07 03:53:06]

Change committed as 275604