mus: Add PointerWatcher for passively observing mouse and touch events

mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests

Committed: https://crrev.com/214fcbde97eec5f468f0de0561a3f0eb26571a9f
Cr-Commit-Position: refs/heads/master@{#390267}

[James Cook, 2016-04-26 16:40:34]

Description was changed from

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL implements closing the tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests
==========

to

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the
tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests
==========

[James Cook, 2016-04-26 16:42:07]

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

[James Cook, 2016-04-26 16:42:08]

sky, please take a look.

Apologies for the length. I pulled the code to actually close the bubble into a
separate CL in an attempt to make this one smaller, but it's still pretty long.
Fortunately a fair amount of the length is the tests.

[sky, 2016-04-26 16:55:48]

https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
ash/pointer_watcher_delegate_aura.cc:16:
PointerWatcherDelegateAura::~PointerWatcherDelegateAura() {}
Remove pretargethandler if have observers?

https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h#newcod...
ash/shell_delegate.h:143: virtual PointerWatcherDelegate*
CreatePointerWatcherDelegate() = 0;
unique_ptr?

https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
ui/views/mus/window_manager_connection.cc:125: // ~WindowTreeConnection calls
back to us (we're the WindowTreeDelegate),
Deal with pointer_watchers_ having observers? Or DCHECK.

[sky, 2016-04-26 17:02:15]

On 2016/04/26 16:55:48, sky wrote:
>
https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
> File ash/pointer_watcher_delegate_aura.cc (right):
> 
>
https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
> ash/pointer_watcher_delegate_aura.cc:16:
> PointerWatcherDelegateAura::~PointerWatcherDelegateAura() {}
> Remove pretargethandler if have observers?
> 
> https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h
> File ash/shell_delegate.h (right):
> 
>
https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h#newcod...
> ash/shell_delegate.h:143: virtual PointerWatcherDelegate*
> CreatePointerWatcherDelegate() = 0;
> unique_ptr?
> 
>
https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
> File ui/views/mus/window_manager_connection.cc (right):
> 
>
https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
> ui/views/mus/window_manager_connection.cc:125: // ~WindowTreeConnection calls
> back to us (we're the WindowTreeDelegate),
> Deal with pointer_watchers_ having observers? Or DCHECK.


If you think the lists should be empty, then you can supply the type parameter
true to ObserverList and it will DCHECK for you.

[James Cook, 2016-04-26 17:42:45]

sky, thanks for the quick review. Please take another look.

https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/1921673005/diff/1/ash/pointer_watcher_delegat...
ash/pointer_watcher_delegate_aura.cc:16:
PointerWatcherDelegateAura::~PointerWatcherDelegateAura() {}
On 2016/04/26 16:55:48, sky wrote:
> Remove pretargethandler if have observers?

I went with ObserverList check_empty. I think that makes it more likely that a
developer will catch that he's leaking observers on shutdown (which I find
painful to debug).

https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/1921673005/diff/1/ash/shell_delegate.h#newcod...
ash/shell_delegate.h:143: virtual PointerWatcherDelegate*
CreatePointerWatcherDelegate() = 0;
On 2016/04/26 16:55:48, sky wrote:
> unique_ptr?

Done.

https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/1921673005/diff/1/ui/views/mus/window_manager...
ui/views/mus/window_manager_connection.cc:125: // ~WindowTreeConnection calls
back to us (we're the WindowTreeDelegate),
On 2016/04/26 16:55:48, sky wrote:
> Deal with pointer_watchers_ having observers? Or DCHECK.

Add ObserverList check_empty flag.

[sky, 2016-04-26 18:14:25]

LGTM

[James Cook, 2016-04-26 18:22:28]

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

[James Cook, 2016-04-26 18:22:29]

Ben, can I get OWNERS for the no-op interface implementation in
content/renderer/mus ?

[Ben Goodger (Google), 2016-04-26 18:26:50]

lgtm

[James Cook, 2016-04-26 18:33:43]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-04-26 18:33:44]

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/1921673005/#ps40001
(title: "fix ash_shell_with_content")

[commit-bot: I haz the power, 2016-04-26 18:34:24]

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

[commit-bot: I haz the power, 2016-04-26 19:58:42]

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

[commit-bot: I haz the power, 2016-04-26 19:58:43]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[sky, 2016-04-26 21:44:45]

Not LGTM. Sorry, one question here.

https://codereview.chromium.org/1921673005/diff/40001/ash/pointer_watcher_del...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/1921673005/diff/40001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:30: if
(!pointer_watchers_.might_have_observers()) {
One question here. This is named might for a reason. At certain times this can
return true even if there are no observers. Do you really need to use it?

[James Cook, 2016-04-26 23:19:05]

sky, please take another look.

https://codereview.chromium.org/1921673005/diff/40001/ash/pointer_watcher_del...
File ash/pointer_watcher_delegate_aura.cc (right):

https://codereview.chromium.org/1921673005/diff/40001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:30: if
(!pointer_watchers_.might_have_observers()) {
On 2016/04/26 21:44:45, sky wrote:
> One question here. This is named might for a reason. At certain times this can
> return true even if there are no observers. Do you really need to use it?

Discussed offline. I converted this one to always install itself via
AddPreTargetHandler() and the one in WindowManagerConnection to use
base::ObserverList<>::Iterator to check if the observer list is empty.

https://codereview.chromium.org/1921673005/diff/60001/ui/views/mus/window_man...
File ui/views/mus/window_manager_connection.cc (right):

https://codereview.chromium.org/1921673005/diff/60001/ui/views/mus/window_man...
ui/views/mus/window_manager_connection.cc:81: bool had_watcher =
HasPointerWatcher();
I believe you have to use iterator.GetNext() in both the add and remove cases,
because a PointerWatcher could both remove and re-add itself during
FOR_EACH_OBSERVER.

[James Cook, 2016-04-27 21:33:38]

sky, friendly ping?

[sky, 2016-04-27 23:45:50]

LGTM - sorry for the delay, I some how missed that you updated the patch.

[James Cook, 2016-04-28 00:09:43]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-04-28 00:09:43]

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

[commit-bot: I haz the power, 2016-04-28 00:10:17]

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

[commit-bot: I haz the power, 2016-04-28 01:16:21]

Description was changed from

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the
tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests
==========

to

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the
tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests
==========

[commit-bot: I haz the power, 2016-04-28 01:16:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:15:22]

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

[commit-bot: I haz the power, 2016-04-30 17:53:36]

Description was changed from

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the
tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests
==========

to

==========
mus: Add PointerWatcher for passively observing mouse and touch events

This will support closing the system tray bubble when clicking outside
its bounds. It also provides a path for converting existing ui::EventHandlers
that are used to observe events that occur outside of existing widgets.

Dependent CL https://codereview.chromium.org/1918183003 implements closing the
tray bubble.

BUG=599142
TEST=added to views_mus_unittests and mojo_view_manager_lib_unittests

Committed: https://crrev.com/214fcbde97eec5f468f0de0561a3f0eb26571a9f
Cr-Commit-Position: refs/heads/master@{#390267}
==========