mash: Make system tray bubble close when another window is activated

mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes

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

[James Cook, 2016-04-06 17:58:08]

Description was changed from

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
==========

to

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes
==========

[James Cook, 2016-04-06 17:58:26]

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

[James Cook, 2016-04-06 17:58:27]

sky, please take a look.

[msw, 2016-04-06 18:18:55]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2016-04-06 18:18:56]

minor driveby nit q

https://codereview.chromium.org/1863523006/diff/20001/ash/mus/sysui_applicati...
File ash/mus/sysui_application.cc (right):

https://codereview.chromium.org/1863523006/diff/20001/ash/mus/sysui_applicati...
ash/mus/sysui_application.cc:69: return mash::wm::mojom::Container::COUNT;
nit q: is the final return statement inside a switch's default case okay?

[James Cook, 2016-04-06 18:21:28]

https://codereview.chromium.org/1863523006/diff/20001/ash/mus/sysui_applicati...
File ash/mus/sysui_application.cc (right):

https://codereview.chromium.org/1863523006/diff/20001/ash/mus/sysui_applicati...
ash/mus/sysui_application.cc:69: return mash::wm::mojom::Container::COUNT;
On 2016/04/06 18:18:56, msw wrote:
> nit q: is the final return statement inside a switch's default case okay?

Dunno what we prefer from a style perspective, but the compiler knows that all
code paths return a value and doesn't complain.

[James Cook, 2016-04-06 18:22:14]

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

[commit-bot: I haz the power, 2016-04-06 18:22:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523006/20001

[James Cook, 2016-04-06 19:31:45]

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

[commit-bot: I haz the power, 2016-04-06 19:32:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523006/40001

[commit-bot: I haz the power, 2016-04-06 21:43:56]

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

[commit-bot: I haz the power, 2016-04-06 21:43:59]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-04-07 15:52:05]

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

[commit-bot: I haz the power, 2016-04-07 15:52:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523006/60001

[commit-bot: I haz the power, 2016-04-07 16:34:29]

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

[commit-bot: I haz the power, 2016-04-07 16:34:31]

Dry run: This issue passed the CQ dry run.

[sky, 2016-04-07 17:55:10]

https://codereview.chromium.org/1863523006/diff/60001/components/mus/ws/focus...
File components/mus/ws/focus_controller.cc (right):

https://codereview.chromium.org/1863523006/diff/60001/components/mus/ws/focus...
components/mus/ws/focus_controller.cc:78: // TODO(jamescook): Failures to set
focus should be propagated back to the
Nuke this TODO. I have a patch out to fix.

https://codereview.chromium.org/1863523006/diff/60001/mash/wm/public/interfac...
File mash/wm/public/interfaces/container.mojom (right):

https://codereview.chromium.org/1863523006/diff/60001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:8: // stacked in this order. See
RootWindowController::CreateContainers() for the
I think it would be easier to see if the list is ordered based on z-order. Is
that possible?

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus_unittest.cc (right):

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:34: // A Widget that tracks
activation changes.
Rather than creating a widget subclass can you use WidgetFocusChangeListener and
WidgetObserver?

It looks like widget_interactive_uitests provides coverage of what you're after
here. Can we run that test in views_mus_unittests?

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:98: TestWidget*
CreateWidget(TestWidgetDelegate* delegate) {
Make this return a scoped_ptr

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:119:
native_widget->OnActivationChanged(true);
If you call widget->Activate() does it call this?

[James Cook, 2016-04-07 20:43:43]

sky, please take another look.

https://codereview.chromium.org/1863523006/diff/60001/components/mus/ws/focus...
File components/mus/ws/focus_controller.cc (right):

https://codereview.chromium.org/1863523006/diff/60001/components/mus/ws/focus...
components/mus/ws/focus_controller.cc:78: // TODO(jamescook): Failures to set
focus should be propagated back to the
On 2016/04/07 17:55:10, sky wrote:
> Nuke this TODO. I have a patch out to fix.

Done.

https://codereview.chromium.org/1863523006/diff/60001/mash/wm/public/interfac...
File mash/wm/public/interfaces/container.mojom (right):

https://codereview.chromium.org/1863523006/diff/60001/mash/wm/public/interfac...
mash/wm/public/interfaces/container.mojom:8: // stacked in this order. See
RootWindowController::CreateContainers() for the
On 2016/04/07 17:55:10, sky wrote:
> I think it would be easier to see if the list is ordered based on z-order. Is
> that possible?

Actually, the list is in z-order, but I thought it was important to point out
there is a hierarchy. I didn't try to replicate the hierarchy here (via
indenting or comments) because I didn't want the information to be in two
places. Updated the comment.

(Would you prefer I indent the various enum values to match the hierarchy? It's
a lot easier to see than the code in RootWindowController, but OTOH it
duplicates information.)

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus_unittest.cc (right):

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:34: // A Widget that tracks
activation changes.
On 2016/04/07 17:55:10, sky wrote:
> Rather than creating a widget subclass can you use WidgetFocusChangeListener
and
> WidgetObserver?
> 
> It looks like widget_interactive_uitests provides coverage of what you're
after
> here. Can we run that test in views_mus_unittests?

Switched to using TestWidgetFocusChangeListener and a WidgetObserver.

I poked at running widget_interactive_uitests in views_mus_unittests but didn't
have much luck. :-(

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:98: TestWidget*
CreateWidget(TestWidgetDelegate* delegate) {
On 2016/04/07 17:55:10, sky wrote:
> Make this return a scoped_ptr

Done.

https://codereview.chromium.org/1863523006/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus_unittest.cc:119:
native_widget->OnActivationChanged(true);
On 2016/04/07 17:55:10, sky wrote:
> If you call widget->Activate() does it call this?

Yes. Changed test to start the widget in the shown/active state, then use
widget->Deactivate() and Activate().

[sky, 2016-04-07 23:08:20]

LGTM

[James Cook, 2016-04-08 16:47:48]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-04-08 16:48:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1863523006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1863523006/80001

[commit-bot: I haz the power, 2016-04-08 17:37:44]

Description was changed from

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes
==========

to

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes
==========

[commit-bot: I haz the power, 2016-04-08 17:37:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-08 17:40:10]

Description was changed from

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes
==========

to

==========
mash: Make system tray bubble close when another window is activated

This is one of two code paths by which the system tray bubble closes, and is
a common mechanism for other bubbles to close.

* Introduce a container for bubbles in mash::wm, similar to the Ash bubble
container. Place bubbles there, instead of in the menus container.
* Allow that container to have activatable windows as children.
* Propagate widget activation changes from NativeWidgetMus to Widget.

Note: This does not entirely fix the problem, as the mus::Window::drawn_ state
is incorrect. Once that is fixed the bubbles will close properly. For now this
can be forced true by changing the mus::Window constructor.

BUG=599142
TEST=added to views_mus_unittests, also open system tray bubble then press
alt-tab to activate a window, bubble closes

Committed: https://crrev.com/d80523229c7a5b7b48647d0ca24c61ad6376c354
Cr-Commit-Position: refs/heads/master@{#386118}
==========

[commit-bot: I haz the power, 2016-04-08 17:40:12]

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