Add window overview mode behind --ash-enable-overview-mode flag to F5 key.

Add window overview mode behind --ash-enable-overview-mode flag to F5 key.

BUG=263477
TEST=WindowSelector.*
TEST=Enable --ash-enable-overview-mode and open several windows. Press F5 ([ ]] key) and you should see overview of all windows. Click a win
dow to activate it.

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

[flackr, 2013-07-25 18:42:59]

This adds a window overview mode behind a flag. Can you take a look? Thanks!

[sky, 2013-07-25 21:18:36]

https://codereview.chromium.org/20415002/diff/1/ash/wm/window_cycle_controller.h
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/20415002/diff/1/ash/wm/window_cycle_controlle...
ash/wm/window_cycle_controller.h:45: public WindowSelectorDelegate {
Is there a reason for baking the overview into this class? I tend to think the
code you care about here should be refactored so that both WindowCycleController
and overview code can use it.

[flackr, 2013-07-25 21:34:33]

https://codereview.chromium.org/20415002/diff/1/ash/wm/window_cycle_controller.h
File ash/wm/window_cycle_controller.h (right):

https://codereview.chromium.org/20415002/diff/1/ash/wm/window_cycle_controlle...
ash/wm/window_cycle_controller.h:45: public WindowSelectorDelegate {
On 2013/07/25 21:18:37, sky wrote:
> Is there a reason for baking the overview into this class? I tend to think the
> code you care about here should be refactored so that both
WindowCycleController
> and overview code can use it.

Eventually, we'll use overview (WindowSelector) for alt tab cycling as well
(http://crbug.com/263481) which is why I was baking it in here. But I think the
point still stands even then. I'll try to refactor out the common high level
behavior, into something used for selection regardless of whether it was by alt
tab or overview.

[flackr, 2013-07-26 15:24:27]

I refactored out the MRU window tracking in WindowCycleController and use it
from both WindowCycleController and WindowSelectorController. Can you take
another look? Thanks!

[flackr, 2013-07-26 15:28:28]

I could use WindowCycleController::BuildWindowList from WindowSelector in this
CL and save the somewhat large refactoring for a different CL if it would be
preferable.

[sky, 2013-07-26 19:08:18]

Any chance you could do the refactoring now and in a separate patch?

[flackr, 2013-07-29 15:38:37]

This can be done as an orthogonal change to the refactor while still not being
backed into the WindowCycleController class. PTAL, thanks.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
File ash/wm/window_selector_controller.cc (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
ash/wm/window_selector_controller.cc:34:
WindowCycleController::BuildWindowList(NULL, false);
This will change to the window tracker MRU list when
https://codereview.chromium.org/20708005/ lands.

[sky, 2013-07-29 16:41:04]

20708005 is close. Should I wait for it to land before reviewing? I'm equally
fine if you make this build on 20708005 before it actually lands. Your call.

[flackr, 2013-07-30 04:21:39]

On 2013/07/29 16:41:04, sky wrote:
> 20708005 is close. Should I wait for it to land before reviewing? I'm equally
> fine if you make this build on 20708005 before it actually lands. Your call.

This CL will barely change with 20708005, just the call to build the window
list. Please review when you have time, thanks.

[sky, 2013-07-30 16:39:19]

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc
File ash/wm/window_selector.cc (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:39: std::min((float)target_bounds.width() /
bounds.width(),
static_cast

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:48: window->layer()->SetTransform(transform);
Why go through layer directly? Additionally, what if someone changes the
transform out from under you? Or the bounds, which would effect the transform?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:100: if (event->type() != ui::ET_MOUSE_RELEASED)
I believe this code means you can mouse down on one and release on another and
have it take effect. That seems a bit wrong.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:114: if (iter != windows_.end()) {
Shouldn't this be a DCHECK?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:118: }
What if windows_ goes empty here?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:137: for (size_t j = 0; j < windows_.size(); ++j) {
nit: 2 thoughts here, its up to you if you want to change it at all.

1: doubly nested loops are a bit error prone. I would extract the inner loop
into a helper function.

2 maintain a map<root,vector>. Iterate over the windows putting them in the map
then iterate through root_windows invoking PositionWindowsOnRoot with the
contents of the vector.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:138: if (windows_[j]->GetRootWindow() ==
root_window_list[i]) {
nit: no {}

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:142: PositionWindowsOnRoot(root_window_list[i],
windows_in_root);
Do you want to invoke this if windows_in_root is empty?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:158: for (;; ++windows_per_row) {
Seems like there has to be a better way to calculate this rather iterating
through a loop trying each size.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
File ash/wm/window_selector_controller.cc (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
ash/wm/window_selector_controller.cc:50: wm::ActivateWindow(window);
Does this do the right thing if window was minimized?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
File ash/wm/window_selector_delegate.h (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
ash/wm/window_selector_delegate.h:20: virtual void SelectWindow(aura::Window*
window) = 0;
Maybe OnWindowSelected?

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
ash/wm/window_selector_delegate.h:28: #endif  // ASH_WM_WINDOW_SELECTOR_H_
_DELEGATE

[flackr, 2013-07-31 20:06:12]

I found out that minimized windows weren't getting events because they thought
they were still not visible. I've moved to using a structure to track the
original transform (to restore minimized windows to where they faded out to) and
minimized state and restoring the window while showing the overview. PTAL,
thanks!

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc
File ash/wm/window_selector.cc (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:39: std::min((float)target_bounds.width() /
bounds.width(),
On 2013/07/30 16:39:19, sky wrote:
> static_cast

Done.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:48: window->layer()->SetTransform(transform);
On 2013/07/30 16:39:19, sky wrote:
> Why go through layer directly? Additionally, what if someone changes the
> transform out from under you? Or the bounds, which would effect the transform?

Calling SetTransform on Window now, done.

If someone changes the transform or bounds out from under this it will certainly
be wrong. I've added a TODO, and assuming you're okay leaving this for another
CL, I will change this soon to hide the windows and replace them with a copy of
the layer (similar to dragging across displays). This will also be necessary to
display windows from another display for alt-tab cycling.

Unfortunately my understanding is that we currently have no way of re-using the
textures so it wouldn't be a live view (I'd also like to investigate a way to
share textures separately).

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:100: if (event->type() != ui::ET_MOUSE_RELEASED)
On 2013/07/30 16:39:19, sky wrote:
> I believe this code means you can mouse down on one and release on another and
> have it take effect. That seems a bit wrong.

It seems that we send the released event to the window which received the down
event because on the pressed event we set the target to be the pressed handler
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/root_windo...)
and target all subsequent events to that window
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/root_windo...).
I've added a hit test to make sure the mouse is still over the window the click
was started on.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:114: if (iter != windows_.end()) {
On 2013/07/30 16:39:19, sky wrote:
> Shouldn't this be a DCHECK?

Done.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:118: }
On 2013/07/30 16:39:19, sky wrote:
> What if windows_ goes empty here?

Bad things happen, since you have no more targets and WindowSelector is
consuming events. I've added OnSelectionCanceled to WindowSelectorDelegate so
the controller can stop window selection in this case, and added a test.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:137: for (size_t j = 0; j < windows_.size(); ++j) {
On 2013/07/30 16:39:19, sky wrote:
> nit: 2 thoughts here, its up to you if you want to change it at all.
> 
> 1: doubly nested loops are a bit error prone. I would extract the inner loop
> into a helper function.
> 
> 2 maintain a map<root,vector>. Iterate over the windows putting them in the
map
> then iterate through root_windows invoking PositionWindowsOnRoot with the
> contents of the vector.

I suspect this will change somewhat with alt-tab cycling, but for now I've moved
construction of the window list for the particular root to
PositionWindowsOnRoot.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:138: if (windows_[j]->GetRootWindow() ==
root_window_list[i]) {
On 2013/07/30 16:39:19, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:142: PositionWindowsOnRoot(root_window_list[i],
windows_in_root);
On 2013/07/30 16:39:19, sky wrote:
> Do you want to invoke this if windows_in_root is empty?

Added early exit in PositionWindowsOnRoot.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector.cc...
ash/wm/window_selector.cc:158: for (;; ++windows_per_row) {
On 2013/07/30 16:39:19, sky wrote:
> Seems like there has to be a better way to calculate this rather iterating
> through a loop trying each size.

Done.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
File ash/wm/window_selector_controller.cc (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_co...
ash/wm/window_selector_controller.cc:50: wm::ActivateWindow(window);
On 2013/07/30 16:39:19, sky wrote:
> Does this do the right thing if window was minimized?

Added call to Show to set correct state (consistent with WindowCycleList::Step
used for alt-tab). Also, there were issues with handling events on "minimized"
windows so I've temporarily restored windows, remembering that they were
minimized.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
File ash/wm/window_selector_delegate.h (right):

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
ash/wm/window_selector_delegate.h:20: virtual void SelectWindow(aura::Window*
window) = 0;
On 2013/07/30 16:39:19, sky wrote:
> Maybe OnWindowSelected?

Done.

https://codereview.chromium.org/20415002/diff/21001/ash/wm/window_selector_de...
ash/wm/window_selector_delegate.h:28: #endif  // ASH_WM_WINDOW_SELECTOR_H_
On 2013/07/30 16:39:19, sky wrote:
> _DELEGATE

Done.

[sky, 2013-07-31 23:15:37]

LGTM

[commit-bot: I haz the power, 2013-08-01 00:17:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/20415002/37001

[commit-bot: I haz the power, 2013-08-01 02:10:50]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-01 12:13:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/20415002/63001

[commit-bot: I haz the power, 2013-08-01 15:14:28]

Change committed as 215037