Fixed panel callout widgets showing up in the overview.

Fixed panel callout widgets showing up in the overview.

Add hide/show functions for the callout widgets on the panel manager, and use
them from WindowSelectorPanels to control them.

BUG=377775
TEST=WindowSelectorTest.WindowOverviewHidesCalloutWidgets

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

[Nina, 2014-06-10 19:39:28]

Hi Terry, can you please review this patch? Thanks! BTW the reason I'm storing a
pointer to the panel manager is because that way we can rely on having it even
if the windows are destroyed before the panel item is destroyed. Just saying.

[tdanderson, 2014-06-10 19:59:08]

Is this the design you discussed with Rob earlier? No comments come to mind, so
lgtm.

[Nina, 2014-06-10 21:15:16]

Kind of. Rob, can you give this patch a look? Thanks

[flackr, 2014-06-10 23:10:17]

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (left):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:32: virtual
~ScopedTransformPanelWindow();
Should still define the destructor even if just an empty function.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:92:
window->parent()->layout_manager());
Probably better to pass the root window in the WindowSelectorPanels constructor
and DCHECK that every added panel belongs to that root window.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.h:49: PanelLayoutManager* panel_manager_;
nit: panel_layout_manager_

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

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:265: weak_factory_(this) {
Initialize show_callout_widgets_.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:329: if (show_callout_widgets_) {
nit: early return instead.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:340: if (!show_callout_widgets_) {
nit: early return instead.

[Nina, 2014-06-11 13:45:32]

Rob, can you give this patch another look? Thanks

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (left):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:32: virtual
~ScopedTransformPanelWindow();
On 2014/06/10 23:10:16, flackr wrote:
> Should still define the destructor even if just an empty function.

Done.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:92:
window->parent()->layout_manager());
On 2014/06/10 23:10:16, flackr wrote:
> Probably better to pass the root window in the WindowSelectorPanels
constructor
> and DCHECK that every added panel belongs to that root window.

Refactored the code to use the root window, and get the layout manager from the
container.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/10005/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.h:49: PanelLayoutManager* panel_manager_;
On 2014/06/10 23:10:16, flackr wrote:
> nit: panel_layout_manager_

Done. Actually, removed.

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

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:265: weak_factory_(this) {
On 2014/06/10 23:10:16, flackr wrote:
> Initialize show_callout_widgets_.

Done.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:329: if (show_callout_widgets_) {
On 2014/06/10 23:10:16, flackr wrote:
> nit: early return instead.

Done.

https://codereview.chromium.org/327873005/diff/10005/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:340: if (!show_callout_widgets_) {
On 2014/06/10 23:10:16, flackr wrote:
> nit: early return instead.

Done.

[flackr, 2014-06-11 14:05:22]

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:95: Shell::GetContainer(root_window_,
kShellWindowId_PanelContainer)->
nit: indent 4.

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:96:
layout_manager())->ShowCalloutWidgets();
What happens if the root window goes away? I.e. it's on a second display which
is removed while overview is up. Is this safe?

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.h:24: WindowSelectorPanels(aura::Window*
panels_parent_window);
nit: explicit
Also, just call the param root_window, it's not actually the panels parent
window :-).

[Nina, 2014-06-11 14:20:43]

Rob, please take a look at the comments. Cheers!

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:95: Shell::GetContainer(root_window_,
kShellWindowId_PanelContainer)->
On 2014/06/11 14:05:22, flackr wrote:
> nit: indent 4.

Eclipse keeps indenting these cases wrong when copy-pasting. Fixed.

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:96:
layout_manager())->ShowCalloutWidgets();
On 2014/06/11 14:05:22, flackr wrote:
> What happens if the root window goes away? I.e. it's on a second display which
> is removed while overview is up. Is this safe?

The overview mode is cancelled and we all enjoy a happy, crash-free ending.
There's a test for this btw.

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.h:24: WindowSelectorPanels(aura::Window*
panels_parent_window);
On 2014/06/11 14:05:22, flackr wrote:
> nit: explicit
> Also, just call the param root_window, it's not actually the panels parent
> window :-).

Done.

[flackr, 2014-06-11 15:26:35]

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
File ash/wm/overview/window_selector_panels.cc (right):

https://codereview.chromium.org/327873005/diff/50001/ash/wm/overview/window_s...
ash/wm/overview/window_selector_panels.cc:96:
layout_manager())->ShowCalloutWidgets();
On 2014/06/11 14:20:43, Nicolás wrote:
> On 2014/06/11 14:05:22, flackr wrote:
> > What happens if the root window goes away? I.e. it's on a second display
which
> > is removed while overview is up. Is this safe?
> 
> The overview mode is cancelled and we all enjoy a happy, crash-free ending.
> There's a test for this btw.

Thanks for verifying, as long as there's a test to cover this it's good since at
some point removing a display may not exit overview.

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

https://codereview.chromium.org/327873005/diff/90001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:837: if (icon_bounds.IsEmpty() ||
!panel->layer()->GetTargetVisibility() ||
Add check for show_callout_widgets_ here, then you can remove the condition on
line 764 and just call UpdateCallouts on HideCalloutWidgets.

[Nina, 2014-06-11 15:44:46]

What do you think of it now?

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

https://codereview.chromium.org/327873005/diff/90001/ash/wm/panels/panel_layo...
ash/wm/panels/panel_layout_manager.cc:837: if (icon_bounds.IsEmpty() ||
!panel->layer()->GetTargetVisibility() ||
On 2014/06/11 15:26:35, flackr wrote:
> Add check for show_callout_widgets_ here, then you can remove the condition on
> line 764 and just call UpdateCallouts on HideCalloutWidgets.

Done. I also refactored Show and Hide into one function.

[flackr, 2014-06-11 15:55:28]

LGTM with nits.

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
ash/wm/overview/window_selector_panels.h:16: class PanelLayoutManager;
Remove as not referenced here.

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
ash/wm/overview/window_selector_panels.h:49: aura::Window* root_window_;
Sorry, I just realized the WindowSelectorItem has a root_window_. Even though it
doesn't technically conflict, should update this to something unique to avoid
confusion, perhaps windows_root_window_ or source_root_window_? Update the
parameter on the constructor to match.

https://codereview.chromium.org/327873005/diff/110001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/327873005/diff/110001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:752: 
nit: Remove blank line between these.

[Nina, 2014-06-11 16:03:57]

Got the nits covered. Terry, mind checking the commit bit?

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_panels.h (right):

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
ash/wm/overview/window_selector_panels.h:16: class PanelLayoutManager;
On 2014/06/11 15:55:28, flackr wrote:
> Remove as not referenced here.

Done.

https://codereview.chromium.org/327873005/diff/110001/ash/wm/overview/window_...
ash/wm/overview/window_selector_panels.h:49: aura::Window* root_window_;
On 2014/06/11 15:55:28, flackr wrote:
> Sorry, I just realized the WindowSelectorItem has a root_window_. Even though
it
> doesn't technically conflict, should update this to something unique to avoid
> confusion, perhaps windows_root_window_ or source_root_window_? Update the
> parameter on the constructor to match.

Done.

https://codereview.chromium.org/327873005/diff/110001/ash/wm/panels/panel_lay...
File ash/wm/panels/panel_layout_manager.cc (right):

https://codereview.chromium.org/327873005/diff/110001/ash/wm/panels/panel_lay...
ash/wm/panels/panel_layout_manager.cc:752: 
On 2014/06/11 15:55:28, flackr wrote:
> nit: Remove blank line between these.

Done.

[tdanderson, 2014-06-11 17:07:04]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:11:55]

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

[commit-bot: I haz the power, 2014-06-11 20:31:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:35:24]

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

[commit-bot: I haz the power, 2014-06-11 20:35:26]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[tdanderson, 2014-06-12 13:35:55]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-06-12 13:38:45]

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

[commit-bot: I haz the power, 2014-06-12 21:51:49]

Change committed as 276813