Wires up WmShellMus::Add/RemovePointerWatcher

Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/1cbe94865300052a06c16fa253cedf0e7490dcb8
Cr-Commit-Position: refs/heads/master@{#411740}

[sky, 2016-08-12 03:27:39]

Description was changed from

==========
es th# Enter a description of the change.
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org
==========

[sky, 2016-08-12 03:28:04]

I was tempted to convert wants_moves to an enum and/or bitmask, but I held back.

[James Cook, 2016-08-12 16:08:54]

+CC sammiequon as FYI

I see you have a dependent patch, feel free to ping me on IM when you want me to
look again

https://codereview.chromium.org/2235363003/diff/20001/ash/aura/wm_shell_aura.cc
File ash/aura/wm_shell_aura.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:239: Shell::GetInstance()->AddPointerWatcher(watcher,
wants_moves);
not for this CL: at some point I think we should get rid of
ash::Shell::AddPointerWatcher() and inline its code here. I can look into that
if you like, but it won't be until after next week (I'm build gardener).

https://codereview.chromium.org/2235363003/diff/20001/ash/common/shelf/overfl...
File ash/common/shelf/overflow_bubble.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/common/shelf/overfl...
ash/common/shelf/overflow_bubble.cc:22: const bool wants_moves = false;
Hooray for giving it a name!

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:81: pointer_watcher_event_router_.reset(
optional: MakeUnique?

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

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:52: if (event->type() ==
ui::ET_MOUSE_CAPTURE_CHANGED) {
Move above 47 (merge should handle that I think)

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:60: if
(!ui::PointerEvent::CanConvertFrom(*event))
DCHECK here instead? event must be pressed or release at this point.

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
File ash/pointer_watcher_delegate_aura.h (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.h:54: // The true parameter to ObserverList
indicates the list Must be empty on
nit: Must -> must and rewrap lines

Thanks for the nice docs, btw.

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.h:59:
base::ObserverList<views::PointerWatcher, true> pointer_watchers_;
optional: Maybe non_move_pointer_watchers_?  If that's too long, move_watchers_
and non_move_watchers_?

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.h:60:
base::ObserverList<views::PointerWatcher, true> move_pointer_watchers_;
Or maybe this one should be called |pointer_watchers_| and the other one
|non_move_pointer_watchers_|? I got briefly confused reading NotifyWatchers
about why both observerlists were being notified, and I think it was due to
names.

[sky, 2016-08-12 17:55:35]

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

[commit-bot: I haz the power, 2016-08-12 17:56:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-08-12 17:58:07]

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

[commit-bot: I haz the power, 2016-08-12 17:58:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-08-12 17:59:37]

The other patch landed, so this patch is no longer relative.

https://codereview.chromium.org/2235363003/diff/20001/ash/aura/wm_shell_aura.cc
File ash/aura/wm_shell_aura.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/aura/wm_shell_aura....
ash/aura/wm_shell_aura.cc:239: Shell::GetInstance()->AddPointerWatcher(watcher,
wants_moves);
On 2016/08/12 16:08:53, James Cook wrote:
> not for this CL: at some point I think we should get rid of
> ash::Shell::AddPointerWatcher() and inline its code here. I can look into that
> if you like, but it won't be until after next week (I'm build gardener).

If you have the time that would be great.

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:81: pointer_watcher_event_router_.reset(
On 2016/08/12 16:08:53, James Cook wrote:
> optional: MakeUnique?

I tend to feel that having to create a temp just to assign is overkill. WDYT? By
that I mean this code becomes:

pointer_watcher_event_router_ =
base::MakeUnique<views::PointerWatcherEventRouter>(window_tree_client);

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

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:52: if (event->type() ==
ui::ET_MOUSE_CAPTURE_CHANGED) {
On 2016/08/12 16:08:53, James Cook wrote:
> Move above 47 (merge should handle that I think)

Ya, sorry, I fixed it after you pointed the error out in the last review and
updated a patch while you reviewing.

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:60: if
(!ui::PointerEvent::CanConvertFrom(*event))
On 2016/08/12 16:08:53, James Cook wrote:
> DCHECK here instead? event must be pressed or release at this point.

Turns out mouse wheel events aren't pointer events either (this is why I
suggested keeping the name mouseevents in the last review vs
OnMouseCaptureChanged). So, this has to be a conditional.

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
File ash/pointer_watcher_delegate_aura.h (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.h:54: // The true parameter to ObserverList
indicates the list Must be empty on
On 2016/08/12 16:08:53, James Cook wrote:
> nit: Must -> must and rewrap lines
> 
> Thanks for the nice docs, btw.

Done.

https://codereview.chromium.org/2235363003/diff/20001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.h:59:
base::ObserverList<views::PointerWatcher, true> pointer_watchers_;
On 2016/08/12 16:08:53, James Cook wrote:
> optional: Maybe non_move_pointer_watchers_?  If that's too long,
move_watchers_
> and non_move_watchers_?

I like non_move_watchers_ and move_watchers_. Done.

[James Cook, 2016-08-12 18:26:12]

LGTM

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2235363003/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:81: pointer_watcher_event_router_.reset(
On 2016/08/12 17:59:37, sky wrote:
> On 2016/08/12 16:08:53, James Cook wrote:
> > optional: MakeUnique?
> 
> I tend to feel that having to create a temp just to assign is overkill. WDYT?
By
> that I mean this code becomes:
> 
> pointer_watcher_event_router_ =
> base::MakeUnique<views::PointerWatcherEventRouter>(window_tree_client);

OK, makes sense.

[commit-bot: I haz the power, 2016-08-12 18:38:35]

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

[commit-bot: I haz the power, 2016-08-12 18:38:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-08-12 19:18:55]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-08-12 19:19:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 19:48:32]

Description was changed from

==========
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-08-12 19:48:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-12 19:51:02]

Description was changed from

==========
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Wires up WmShellMus::Add/RemovePointerWatcher

Also makes the aura side support the wants_moves boolean.

BUG=548435
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/1cbe94865300052a06c16fa253cedf0e7490dcb8
Cr-Commit-Position: refs/heads/master@{#411740}
==========

[commit-bot: I haz the power, 2016-08-12 19:51:03]

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