[ash-md] Cancels app-list (launcher) before entering overview

[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive

Committed: https://crrev.com/738eca8c2bf6f6c0646408d9b970d5da83b53ba3
Cr-Commit-Position: refs/heads/master@{#411049}

[varkha, 2016-08-09 00:52:39]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 00:53:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 01:01:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 01:01:20]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[varkha, 2016-08-09 01:13:46]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 01:14:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 01:23:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 01:23:10]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[varkha, 2016-08-09 01:51:41]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 01:52:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-08-09 01:58:38]

Patchset #1 (id:1) has been deleted

[varkha, 2016-08-09 01:59:09]

varkha@chromium.org changed reviewers:
+ tdanderson@chromium.org

[varkha, 2016-08-09 01:59:09]

tdanderson@, can you please take a look? This seems a bit simpler than resetting
the activation after creating the text filter.

[varkha, 2016-08-09 02:14:07]

Description was changed from

==========
[ash-md] - Makes accelerators to not reach an active window in overview

This change deactivates any window active before overview proactively so
that in overview mode there is no active window and the key
accellerators targeting an active window are effectively disabled.

BUG=631956
==========

to

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates any window active before overview proactively so
that in overview mode there is no active window and the key
accellerators targeting an active window are effectively disabled.

BUG=631956
==========

[varkha, 2016-08-09 02:14:37]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 02:15:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 02:50:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 02:50:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[varkha, 2016-08-09 13:14:34]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 13:14:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 13:52:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 13:52:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tdanderson, 2016-08-09 14:21:04]

lgtm with some nits and a suggestion for testing

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
File ash/common/wm/overview/window_selector.cc (right):

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Suggested shorter wording for CL description: "This change deactivates the
currently-active window upon entering overview mode in order to prevent
accelerators from targeting an active window while in overview mode."

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:253: // Deactivate the previously
active window to prevent accelerators targeting
nit: "Deactivate the previously-active window to prevent accelerators from
targeting an active window and triggering actions during overview."

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:257: active_window->Deactivate();
If possible it would be good to have a test for this. Ideally you could add one
that mimics the repro steps you showed me (entering overview while something
like the launcher is open) but I wouldn't be surprised if that was difficult to
simulate in a test. At minimum, consider adding a simple test that enters
overview and just checks that the text filter is the active window.

[varkha, 2016-08-09 16:25:18]

Description was changed from

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates any window active before overview proactively so
that in overview mode there is no active window and the key
accellerators targeting an active window are effectively disabled.

BUG=631956
==========

to

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates the currently-active window upon entering overview
mode in order to prevent accelerators from targeting an active window while
in overview mode.

BUG=631956
==========

[varkha, 2016-08-09 16:47:28]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 16:48:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-08-09 16:50:14]

Thanks for all the good suggestions. I have checked that the new test would have
failed without the change in the text filter code as well as that all the checks
would succeed without the change in CreateAndActivateTextFilter() - without the
call to ToggleAppList(). So indeed we have a scenario that we weer assuming
always worked before but we have found that it did not with the app list and we
have a fix and a test for it. Yay!

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
File ash/common/wm/overview/window_selector.cc (right):

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2016/08/09 14:21:03, tdanderson wrote:
> Suggested shorter wording for CL description: "This change deactivates the
> currently-active window upon entering overview mode in order to prevent
> accelerators from targeting an active window while in overview mode."

Done.

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:253: // Deactivate the previously
active window to prevent accelerators targeting
On 2016/08/09 14:21:03, tdanderson wrote:
> nit: "Deactivate the previously-active window to prevent accelerators from
> targeting an active window and triggering actions during overview."

Done.

https://chromiumcodereview.appspot.com/2225683008/diff/20001/ash/common/wm/ov...
ash/common/wm/overview/window_selector.cc:257: active_window->Deactivate();
On 2016/08/09 14:21:03, tdanderson wrote:
> If possible it would be good to have a test for this. Ideally you could add
one
> that mimics the repro steps you showed me (entering overview while something
> like the launcher is open) but I wouldn't be surprised if that was difficult
to
> simulate in a test. At minimum, consider adding a simple test that enters
> overview and just checks that the text filter is the active window.

Done.

[varkha, 2016-08-09 16:51:19]

Description was changed from

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates the currently-active window upon entering overview
mode in order to prevent accelerators from targeting an active window while
in overview mode.

BUG=631956
==========

to

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates the currently-active window upon entering overview
mode in order to prevent accelerators from targeting an active window while
in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

[commit-bot: I haz the power, 2016-08-09 17:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 17:27:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[varkha, 2016-08-10 00:36:35]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 00:37:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-08-10 00:41:04]

varkha@chromium.org changed reviewers:
+ jamescook@chromium.org

[varkha, 2016-08-10 00:41:04]

jamescook@, can you please take a look? The approach I have in the last PS is
the one I have initially frowned upon because it makes AppListPresenterDelegate
aware of the overview mode but after trying to solve it after the fact I think
this yields an easier and more understandable flow.
In particular the flow where creating and setting focus to the text filter had a
side effect of cancelling the app-list which then had to be reversed by
deactivating that previously activated top level window and reactivating the
text filter wasn't something simple or understandable.
If you have a different idea how this could be solved without coupling app list
and overview I'd love to hear that!
Thanks!

[commit-bot: I haz the power, 2016-08-10 02:45:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 02:45:27]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-10 15:55:59]

This seems fine to me. LGTM.

https://codereview.chromium.org/2225683008/diff/60001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/2225683008/diff/60001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:475: }
Nice test. Easy to read.

[varkha, 2016-08-10 16:04:54]

Thanks!

https://codereview.chromium.org/2225683008/diff/60001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_unittest.cc (right):

https://codereview.chromium.org/2225683008/diff/60001/ash/wm/overview/window_...
ash/wm/overview/window_selector_unittest.cc:475: }
On 2016/08/10 15:55:59, James Cook wrote:
> Nice test. Easy to read.

Acknowledged.

[varkha, 2016-08-10 16:05:02]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-08-10 16:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2225683008/#ps60001
(title: "[ash-md] - Prevents accelerators from reaching an active window in
overview (handle in app-list)")

[commit-bot: I haz the power, 2016-08-10 16:05:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[varkha, 2016-08-10 16:05:33]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2016-08-10 16:09:19]

Description was changed from

==========
[ash-md] Prevents accelerators from reaching an active window in overview

This change deactivates the currently-active window upon entering overview
mode in order to prevent accelerators from targeting an active window while
in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

to

==========
[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

[varkha, 2016-08-10 16:09:31]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2016-08-10 16:09:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 16:13:13]

Description was changed from

==========
[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

to

==========
[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

[commit-bot: I haz the power, 2016-08-10 16:13:14]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-10 16:14:17]

Description was changed from

==========
[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive
==========

to

==========
[ash-md] Cancels app-list (launcher) before entering overview

This change proactively dismisses the app-list (launcher) when
overview mode is about to start. This prevents one of the top
level windows from being activated and disables accelerators that
target an active window while in overview mode.

BUG=631956
TEST=WindowSelectorTest.TextFilterActive

Committed: https://crrev.com/738eca8c2bf6f6c0646408d9b970d5da83b53ba3
Cr-Commit-Position: refs/heads/master@{#411049}
==========

[commit-bot: I haz the power, 2016-08-10 16:14:18]

Patchset 3 (id:??) landed as
https://crrev.com/738eca8c2bf6f6c0646408d9b970d5da83b53ba3
Cr-Commit-Position: refs/heads/master@{#411049}