Change overview mode to so that docked panel windows are not grouped together.

Change overview mode to so that docked panel windows are not grouped together.

BUG=448381
TEST=Manual

Committed: https://crrev.com/d7cf765b7add6a32fa2bb90e27587913f95c2339
Cr-Commit-Position: refs/heads/master@{#312246}

[bruthig, 2015-01-14 20:50:51]

bruthig@chromium.org changed reviewers:
+ jonross@chromium.org

[bruthig, 2015-01-14 20:51:44]

jonross@ can you please do a preliminary review of this before I send it to some
owners?

[jonross, 2015-01-15 13:37:47]

https://codereview.chromium.org/844763006/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/844763006/diff/20001/ash/shell.cc#newcode751
ash/shell.cc:751: // WindowSelectorController docmentation for more info.
nit: documentation

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:319: (*iter)->OnWindowDestroyed();
Remove this.

Instead WindowSelectorItem should have three possible shutdown scenarios:
  - Window closed, no animation.
  - Window activate, an animation to restore the window
  - Selection has ended, animation to the previous state.

For this:
  - WindowSelectorItem's deconstructor should no longer trigger an animation by
default
  - WindowSelectorItem::RestoreWindowOnExit should continue to set a default
transform/opacity
  - WindowSelectorItem needs the current deconstructor's animation to be moved
to a separate method. WindowSelector::CancelSelection can then notify each
WindowSelectorItem to perform this.

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

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:285: for (ScopedVector<WindowGrid>::iterator
iter = grid_list_.begin();
switch to for(auto iter : grid_list_)

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.h:42: // NOTE: Overview mode must not
be active when destructing |this|.
Could you add a DCHECK to the deconstructor to confirm this.

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:83: using
aura::WindowObserver::OnWindowDestroyed;
Remove this once OnWindowDestroyed above has been removed.

[bruthig, 2015-01-15 18:27:25]

jonross@ can you please take another look?

https://codereview.chromium.org/844763006/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/844763006/diff/20001/ash/shell.cc#newcode751
ash/shell.cc:751: // WindowSelectorController docmentation for more info.
On 2015/01/15 13:37:47, jonross wrote:
> nit: documentation

Done.

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:319: (*iter)->OnWindowDestroyed();
On 2015/01/15 13:37:47, jonross wrote:
> Remove this.
> 
> Instead WindowSelectorItem should have three possible shutdown scenarios:
>   - Window closed, no animation.
>   - Window activate, an animation to restore the window
>   - Selection has ended, animation to the previous state.
> 
> For this:
>   - WindowSelectorItem's deconstructor should no longer trigger an animation
by
> default
>   - WindowSelectorItem::RestoreWindowOnExit should continue to set a default
> transform/opacity
>   - WindowSelectorItem needs the current deconstructor's animation to be moved
> to a separate method. WindowSelector::CancelSelection can then notify each
> WindowSelectorItem to perform this.

Done.

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

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:285: for (ScopedVector<WindowGrid>::iterator
iter = grid_list_.begin();
On 2015/01/15 13:37:47, jonross wrote:
> switch to for(auto iter : grid_list_)

Done.

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.h:42: // NOTE: Overview mode must not
be active when destructing |this|.
On 2015/01/15 13:37:47, jonross wrote:
> Could you add a DCHECK to the deconstructor to confirm this.

Done.

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/844763006/diff/20001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.h:83: using
aura::WindowObserver::OnWindowDestroyed;
On 2015/01/15 13:37:47, jonross wrote:
> Remove this once OnWindowDestroyed above has been removed.

Done.

[jonross, 2015-01-15 19:39:42]

https://codereview.chromium.org/844763006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/shell.cc#newcode753
ash/shell.cc:753: window_selector_controller_->ToggleOverview();
I'd like to avoid this if possible. Can we check what happens when shutdown
occurs while in Overview mode?

Especially since we removed animations caused by destructors.

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:72: // NOTE: The work done in Init() and
Shutdown() is not done in the
Could this information be worked into the existing docs for init/shutdown?

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:79: // Perform cleanup that cannot be done in
the destructor.
nit: space between non-override methods that have comments

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:153: aura::Window* window = GetWindow();
Previously we were removing Window from ScopedTransformOverviewMode to block
animations. That is no longer the case.

Can now: GetWindow()->RemoveObserver(this);

https://codereview.chromium.org/844763006/diff/40001/ash/wm/panels/panel_layo...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:617: if (in_layout_ ||
(overview_mode_active_ && !Shell::GetInstance()->
Instead of tracking this, can we poll IsSelecting() here?

[bruthig, 2015-01-16 19:54:43]

jonross@ can you please have another look?

https://codereview.chromium.org/844763006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/shell.cc#newcode753
ash/shell.cc:753: window_selector_controller_->ToggleOverview();
On 2015/01/15 19:39:42, jonross wrote:
> I'd like to avoid this if possible. Can we check what happens when shutdown
> occurs while in Overview mode?
> 
> Especially since we removed animations caused by destructors.

Done.

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:72: // NOTE: The work done in Init() and
Shutdown() is not done in the
On 2015/01/15 19:39:42, jonross wrote:
> Could this information be worked into the existing docs for init/shutdown?

Moved these 'implementation' specific comments to the cc file.

Done.

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.h:79: // Perform cleanup that cannot be done in
the destructor.
On 2015/01/15 19:39:42, jonross wrote:
> nit: space between non-override methods that have comments

Done.

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:153: aura::Window* window = GetWindow();
On 2015/01/15 19:39:42, jonross wrote:
> Previously we were removing Window from ScopedTransformOverviewMode to block
> animations. That is no longer the case.
> 
> Can now: GetWindow()->RemoveObserver(this);

Done.

https://codereview.chromium.org/844763006/diff/40001/ash/wm/panels/panel_layo...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/844763006/diff/40001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:617: if (in_layout_ ||
(overview_mode_active_ && !Shell::GetInstance()->
On 2015/01/15 19:39:42, jonross wrote:
> Instead of tracking this, can we poll IsSelecting() here?

Done.

[jonross, 2015-01-16 20:10:20]

A few minor tweaks in the comments.

Please add owners.

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

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:373: for (std::set<aura::Window*>::iterator
iter = observed_windows_.begin();
for(auto)

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.h:42: // NOTE: Overview mode must not
be active when destructing |this|.
nit: comment no longer valid

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:326: CHECK(window);
This is the only spot where we are doing this check. I don't think it is
necessary and the method will fail quickly if it is false anyways.

[bruthig, 2015-01-16 20:18:26]

bruthig@chromium.org changed reviewers:
+ oshima@chromium.org

[bruthig, 2015-01-16 20:18:27]

oshima@, In flackr@'s absence can you please review this?  Thanks :)

[bruthig, 2015-01-16 20:22:46]

I forgot to fix some minor nits from patch set 4 before adding oshima@. 
Everything is ready for review now.

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

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:373: for (std::set<aura::Window*>::iterator
iter = observed_windows_.begin();
On 2015/01/16 20:10:20, jonross wrote:
> for(auto)

Done.

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_controller.h:42: // NOTE: Overview mode must not
be active when destructing |this|.
On 2015/01/16 20:10:20, jonross wrote:
> nit: comment no longer valid

Done.

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:326: CHECK(window);
On 2015/01/16 20:10:20, jonross wrote:
> This is the only spot where we are doing this check. I don't think it is
> necessary and the method will fail quickly if it is false anyways.

Done.

[jonross, 2015-01-16 20:23:31]

On 2015/01/16 20:22:46, bruthig wrote:
> I forgot to fix some minor nits from patch set 4 before adding oshima@. 
> Everything is ready for review now.
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> File ash/wm/overview/window_selector.cc (right):
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> ash/wm/overview/window_selector.cc:373: for (std::set<aura::Window*>::iterator
> iter = observed_windows_.begin();
> On 2015/01/16 20:10:20, jonross wrote:
> > for(auto)
> 
> Done.
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> File ash/wm/overview/window_selector_controller.h (right):
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> ash/wm/overview/window_selector_controller.h:42: // NOTE: Overview mode must
not
> be active when destructing |this|.
> On 2015/01/16 20:10:20, jonross wrote:
> > nit: comment no longer valid
> 
> Done.
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> File ash/wm/overview/window_selector_item.cc (right):
> 
>
https://codereview.chromium.org/844763006/diff/60001/ash/wm/overview/window_s...
> ash/wm/overview/window_selector_item.cc:326: CHECK(window);
> On 2015/01/16 20:10:20, jonross wrote:
> > This is the only spot where we are doing this check. I don't think it is
> > necessary and the method will fail quickly if it is false anyways.
> 
> Done.

LGTM

[oshima, 2015-01-16 22:36:37]

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:289: for (auto window_item : window_list_) {
const auto*, or just const aura::Window* as it doesn't save much space.

tip: auto is permitted, but not recommended. use full type
when it makes the code more readable.

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

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:286: for (auto window_grid : grid_list_) {
ditto

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:341: for (auto window_selector_item :
window_grid->window_list())
ditto

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:373: for (auto window : observed_windows_) {
dutto

nuke {}

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:117: aura::Window* window)
is it possible to just use window->GetRootWindow()?

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:156: aura::Window*
WindowSelectorItem::GetWindow() const {
a method should be non const if the method returns non const object that is a
part of internal state.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:222: window->RemoveObserver(this);
transform_window_ will now have a stale pointer to this window.
Don't you have to invalidate it somehow?

[bruthig, 2015-01-17 17:35:07]

oshima@ can you take another look?

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_g...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_g...
ash/wm/overview/window_grid.cc:289: for (auto window_item : window_list_) {
On 2015/01/16 22:36:37, oshima wrote:
> const auto*, or just const aura::Window* as it doesn't save much space.
> 
> tip: auto is permitted, but not recommended. use full type
> when it makes the code more readable.

Done. [Thanks for the tip ;) ]

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

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:286: for (auto window_grid : grid_list_) {
On 2015/01/16 22:36:37, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:341: for (auto window_selector_item :
window_grid->window_list())
On 2015/01/16 22:36:37, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector.cc:373: for (auto window : observed_windows_) {
On 2015/01/16 22:36:37, oshima wrote:
> dutto
> 
> nuke {}

Done.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:117: aura::Window* window)
On 2015/01/16 22:36:37, oshima wrote:
> is it possible to just use window->GetRootWindow()?

The WindowGrid ensures that the root_window_ is window's root window so yes.  I
choose to keep the root_window_ member variable and perform a DCHECK in
SetItemBounds to ensure window's root window doesn't change inadvertently.

Done.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:156: aura::Window*
WindowSelectorItem::GetWindow() const {
On 2015/01/16 22:36:37, oshima wrote:
> a method should be non const if the method returns non const object that is a
> part of internal state.

Done.

https://codereview.chromium.org/844763006/diff/80001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_item.cc:222: window->RemoveObserver(this);
On 2015/01/16 22:36:37, oshima wrote:
> transform_window_ will now have a stale pointer to this window.
> Don't you have to invalidate it somehow?

Done.

[oshima, 2015-01-20 18:41:04]

lgtm

discussed offline: JS should be able to move a window to another root window,
but it's not working right now, so
approving for now.

I'll look into the above issue separately.

[bruthig, 2015-01-20 18:44:34]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-01-20 18:45:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/844763006/100001

[commit-bot: I haz the power, 2015-01-20 19:33:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-20 19:33:55]

Patchset 6 (id:??) landed as
https://crrev.com/d7cf765b7add6a32fa2bb90e27587913f95c2339
Cr-Commit-Position: refs/heads/master@{#312246}