Add panels as a single group of windows per display for overview window cycling.

Add panels as a single group of windows per display for overview window cycling.

BUG=264734
TEST=WindowSelectorTest.*, WindowTest.IsVisibleRespectsSetVisible

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

[flackr, 2013-09-13 15:39:02]

Hi Scott,

Sorry for the large review. Adding a selector item that contained multiple
windows (panels) required refactoring window_selector_window into a class which
manages a windows transform restoring it on exiting overview and two
implementations of a selector item, one which contains multiple attached panels
and one which contains a single window.

Please take a look! If you'd like I can split the refactoring into a separate CL
but many of the choices in this refactoring are only justified by having an
implementation with multiple windows.

Thanks!

[sky, 2013-09-13 21:46:44]

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/scoped_transf...
File ash/wm/overview/scoped_transform_overview_window.h (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/scoped_transf...
ash/wm/overview/scoped_transform_overview_window.h:39: const gfx::Rect& rect,
const gfx::Rect& bounds);
nit: one param per line when you wrap.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_item.cc:17: fit_bounds_ = target_bounds;
I recommend you make this non-virtual and implement a protected method that does
the actual applying. Less error prone that way.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_item.h:54: const gfx::Rect& bounds() { return
fit_bounds_; }
either name this fit_bounds() to match member (style guide says this) or rename
member to bounds.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_panels.cc:88: }
// namespace

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc#newcode707
ui/aura/window.cc:707: // visibility state.
I think this is wrong. The reason you're seeing the behavior you are is you're
changing the visibility at the layer level directly. You need to go through
window otherwise things get out of sync.

https://codereview.chromium.org/23654037/diff/1/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/23654037/diff/1/ui/aura/window_unittest.cc#ne...
ui/aura/window_unittest.cc:1543: EXPECT_FALSE(!w1->layer());
ASSERT (otherwise you're going to crash on 1547).

[flackr, 2013-09-13 22:28:37]

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/scoped_transf...
File ash/wm/overview/scoped_transform_overview_window.h (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/scoped_transf...
ash/wm/overview/scoped_transform_overview_window.h:39: const gfx::Rect& rect,
const gfx::Rect& bounds);
On 2013/09/13 21:46:44, sky wrote:
> nit: one param per line when you wrap.

Done.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_item.cc:17: fit_bounds_ = target_bounds;
On 2013/09/13 21:46:44, sky wrote:
> I recommend you make this non-virtual and implement a protected method that
does
> the actual applying. Less error prone that way.

Done.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_item.h:54: const gfx::Rect& bounds() { return
fit_bounds_; }
On 2013/09/13 21:46:44, sky wrote:
> either name this fit_bounds() to match member (style guide says this) or
rename
> member to bounds.

Done.

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/23654037/diff/1/ash/wm/overview/window_select...
ash/wm/overview/window_selector_panels.cc:88: }
On 2013/09/13 21:46:44, sky wrote:
> // namespace

Done.

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc#newcode707
ui/aura/window.cc:707: // visibility state.
On 2013/09/13 21:46:44, sky wrote:
> I think this is wrong. The reason you're seeing the behavior you are is you're
> changing the visibility at the layer level directly. You need to go through
> window otherwise things get out of sync.

I should provide some context. PanelLayoutManager::MinimizePanel it calls
Window::Hide and then in PanelLayoutManager::Relayout expects Window::IsVisible
to return false. Both of these calls are made on the Window and I think should
return a consistent result even though the layer is animating as described in
the comment in Window::IsVisible, but let me know what you think.

https://codereview.chromium.org/23654037/diff/1/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

https://codereview.chromium.org/23654037/diff/1/ui/aura/window_unittest.cc#ne...
ui/aura/window_unittest.cc:1543: EXPECT_FALSE(!w1->layer());
On 2013/09/13 21:46:44, sky wrote:
> ASSERT (otherwise you're going to crash on 1547).

Done.

[sky, 2013-09-16 17:03:35]

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc#newcode707
ui/aura/window.cc:707: // visibility state.
On 2013/09/13 22:28:38, flackr wrote:
> On 2013/09/13 21:46:44, sky wrote:
> > I think this is wrong. The reason you're seeing the behavior you are is
you're
> > changing the visibility at the layer level directly. You need to go through
> > window otherwise things get out of sync.
> 
> I should provide some context. PanelLayoutManager::MinimizePanel it calls
> Window::Hide and then in PanelLayoutManager::Relayout expects
Window::IsVisible
> to return false. Both of these calls are made on the Window and I think should
> return a consistent result even though the layer is animating as described in
> the comment in Window::IsVisible, but let me know what you think.

725 updates visibility, so how come PanelLayoutManager is not seeing the correct
visible state?

[flackr, 2013-09-17 15:20:17]

On 2013/09/16 17:03:35, sky wrote:
> https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
> https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc#newcode707
> ui/aura/window.cc:707: // visibility state.
> On 2013/09/13 22:28:38, flackr wrote:
> > On 2013/09/13 21:46:44, sky wrote:
> > > I think this is wrong. The reason you're seeing the behavior you are is
> you're
> > > changing the visibility at the layer level directly. You need to go
through
> > > window otherwise things get out of sync.
> > 
> > I should provide some context. PanelLayoutManager::MinimizePanel it calls
> > Window::Hide and then in PanelLayoutManager::Relayout expects
> Window::IsVisible
> > to return false. Both of these calls are made on the Window and I think
should
> > return a consistent result even though the layer is animating as described
in
> > the comment in Window::IsVisible, but let me know what you think.
> 
> 725 updates visibility, so how come PanelLayoutManager is not seeing the
correct
> visible state?

It's hitting the early exit that the target visibility is already false, so the
call to Hide which calls SetVisibility(false) doesn't change visibility_. I
don't know yet what is setting the target visibility without calling hide but I
will look into it. At a high level though, shouldn't IsVisible always update to
reflect the last SetVisible call even if the underlying layer was changed
earlier?

In the meantime without this change this CL still works, and only results in
slightly incorrect panel layout when you restore with some minimized and some
regular panels. Would you be okay landing everything but the change to window.cc
and I can investigate this separately?

[flackr, 2013-09-17 15:44:09]

On 2013/09/17 15:20:17, flackr wrote:
> On 2013/09/16 17:03:35, sky wrote:
> > https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc
> > File ui/aura/window.cc (right):
> > 
> > https://codereview.chromium.org/23654037/diff/1/ui/aura/window.cc#newcode707
> > ui/aura/window.cc:707: // visibility state.
> > On 2013/09/13 22:28:38, flackr wrote:
> > > On 2013/09/13 21:46:44, sky wrote:
> > > > I think this is wrong. The reason you're seeing the behavior you are is
> > you're
> > > > changing the visibility at the layer level directly. You need to go
> through
> > > > window otherwise things get out of sync.
> > > 
> > > I should provide some context. PanelLayoutManager::MinimizePanel it calls
> > > Window::Hide and then in PanelLayoutManager::Relayout expects
> > Window::IsVisible
> > > to return false. Both of these calls are made on the Window and I think
> should
> > > return a consistent result even though the layer is animating as described
> in
> > > the comment in Window::IsVisible, but let me know what you think.
> > 
> > 725 updates visibility, so how come PanelLayoutManager is not seeing the
> correct
> > visible state?
> 
> It's hitting the early exit that the target visibility is already false, so
the
> call to Hide which calls SetVisibility(false) doesn't change visibility_. I
> don't know yet what is setting the target visibility without calling hide but
I
> will look into it. At a high level though, shouldn't IsVisible always update
to
> reflect the last SetVisible call even if the underlying layer was changed
> earlier?
> 
> In the meantime without this change this CL still works, and only results in
> slightly incorrect panel layout when you restore with some minimized and some
> regular panels. Would you be okay landing everything but the change to
window.cc
> and I can investigate this separately?

I found the visibility update on the layer, it was in
scoped_transform_overview_window.cc. I've changed that, removed the window.cc
change and it all works now. PTAL, thanks.

[sky, 2013-09-17 16:05:24]

LGTM

[commit-bot: I haz the power, 2013-09-17 17:12:04]

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

[commit-bot: I haz the power, 2013-09-17 20:09:31]

Change committed as 223665