mash: Don't close system tray bubble for clicks in menus/notifications

mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570, 599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/

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

[James Cook, 2016-05-06 17:20:03]

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

[James Cook, 2016-05-06 17:20:03]

sky, please take a look.

I realized after we talked that moving PointerWatcher after event dispatch is
undesirable, as the event dispatch may cause its target to be closed/deleted.

This is a way to do it at the views::Widget level, and also gives us a mechanism
to do window container checks in mus ash.

[sadrul, 2016-05-06 17:28:25]

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

[sadrul, 2016-05-06 17:28:26]

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:32: virtual void OnTouchPressed(const gfx::Point&
location_in_screen,
I would still keep these as Events if possible. Observers often want additional
info about the specific event (e.g. was the shift key down, was the
middle-button clicked, and so on)

[James Cook, 2016-05-06 17:46:35]

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:32: virtual void OnTouchPressed(const gfx::Point&
location_in_screen,
On 2016/05/06 17:28:26, sadrul wrote:
> I would still keep these as Events if possible. Observers often want
additional
> info about the specific event (e.g. was the shift key down, was the
> middle-button clicked, and so on)

I intentionally removed the |event| because I'm concerned that some of the
fields are not valid in PointerWatchers, in particular event.target, and I
didn't want PointerWatcher users to have to think about which fields are valid
and which aren't. (I'm not sure event.root_location is currently valid either.)
My intent was to add event flag/modifier key parameters to the PointerWatcher
callbacks as I found cases that needed them.

How strongly do you feel about this?  (It's trivial for me to add the parameter
back.)

[sky, 2016-05-06 21:20:47]

https://codereview.chromium.org/1957653003/diff/20001/ash/mus/container_deleg...
File ash/mus/container_delegate_mus.cc (right):

https://codereview.chromium.org/1957653003/diff/20001/ash/mus/container_deleg...
ash/mus/container_delegate_mus.cc:25: mus::Window* window =
Use GetMusWindow, that way you don't need casts. Also, I think you want
widget->GetTopLevelWidget().

[sadrul, 2016-05-06 21:46:30]

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:32: virtual void OnTouchPressed(const gfx::Point&
location_in_screen,
On 2016/05/06 17:46:35, James Cook wrote:
> On 2016/05/06 17:28:26, sadrul wrote:
> > I would still keep these as Events if possible. Observers often want
> additional
> > info about the specific event (e.g. was the shift key down, was the
> > middle-button clicked, and so on)
> 
> I intentionally removed the |event| because I'm concerned that some of the
> fields are not valid in PointerWatchers, in particular event.target, and I
> didn't want PointerWatcher users to have to think about which fields are valid
> and which aren't.

I don't really think this is going to be an issue.

> (I'm not sure event.root_location is currently valid either.)
> My intent was to add event flag/modifier key parameters to the PointerWatcher
> callbacks as I found cases that needed them.
> 
> How strongly do you feel about this?  (It's trivial for me to add the
parameter
> back.)

I think sending the whole event is better, and I would prefer if we did that
from the start. Updating these to later include additional information about the
event as separate parameters would need to update a lot of existing code, and
isn't that nice API-wise either.

A cleaner option may be to separate out the dispatch-related info from events
(i.e. target, phase, result), since that's the part that's not entirely
available here, but honestly, I don't think not having these data in Event is
going to be a problem.

[James Cook, 2016-05-06 22:46:24]

sky, please take another look

https://codereview.chromium.org/1957653003/diff/20001/ash/mus/container_deleg...
File ash/mus/container_delegate_mus.cc (right):

https://codereview.chromium.org/1957653003/diff/20001/ash/mus/container_deleg...
ash/mus/container_delegate_mus.cc:25: mus::Window* window =
On 2016/05/06 21:20:46, sky wrote:
> Use GetMusWindow, that way you don't need casts. Also, I think you want
> widget->GetTopLevelWidget().

Done.

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watcher.h
File ui/views/pointer_watcher.h (right):

https://codereview.chromium.org/1957653003/diff/20001/ui/views/pointer_watche...
ui/views/pointer_watcher.h:32: virtual void OnTouchPressed(const gfx::Point&
location_in_screen,
On 2016/05/06 21:46:30, sadrul wrote:
> On 2016/05/06 17:46:35, James Cook wrote:
> > On 2016/05/06 17:28:26, sadrul wrote:
> > > I would still keep these as Events if possible. Observers often want
> > additional
> > > info about the specific event (e.g. was the shift key down, was the
> > > middle-button clicked, and so on)
> > 
> > I intentionally removed the |event| because I'm concerned that some of the
> > fields are not valid in PointerWatchers, in particular event.target, and I
> > didn't want PointerWatcher users to have to think about which fields are
valid
> > and which aren't.
> 
> I don't really think this is going to be an issue.
> 
> > (I'm not sure event.root_location is currently valid either.)
> > My intent was to add event flag/modifier key parameters to the
PointerWatcher
> > callbacks as I found cases that needed them.
> > 
> > How strongly do you feel about this?  (It's trivial for me to add the
> parameter
> > back.)
> 
> I think sending the whole event is better, and I would prefer if we did that
> from the start. Updating these to later include additional information about
the
> event as separate parameters would need to update a lot of existing code, and
> isn't that nice API-wise either.
> 
> A cleaner option may be to separate out the dispatch-related info from events
> (i.e. target, phase, result), since that's the part that's not entirely
> available here, but honestly, I don't think not having these data in Event is
> going to be a problem.

Done.

[sky, 2016-05-07 16:50:59]

LGTM

[James Cook, 2016-05-09 16:04:02]

Description was changed from

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
==========

to

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/
==========

[James Cook, 2016-05-09 16:04:13]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-05-09 16:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1957653003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-09 16:04:43]

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

[commit-bot: I haz the power, 2016-05-09 18:22:24]

Description was changed from

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/
==========

to

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/
==========

[commit-bot: I haz the power, 2016-05-09 18:22:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-09 18:24:45]

Description was changed from

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/
==========

to

==========
mash: Don't close system tray bubble for clicks in menus/notifications

* Plumb the hit mus::Window through the client WindowManagerConnection event
  observers
* Add the hit widget to PointerWatcher.
* Remove the event from PointerWatcher, since the target isn't valid on mus
  and the client cannot set handled or stop propagation.
* Add ContainerDelegate to ash to allow checking if a given widget is in a
  particular window container.
* Convert TrayEventFilter to use the new container delegate.

BUG=608570,599142
TEST=existing ash_unittests, views_mus_unittests
TBR=ben@chromium.org for no-op method signature change in content/renderer/mus/

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

[commit-bot: I haz the power, 2016-05-09 18:24:48]

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