Update ui::PointerEvent to support mouse wheel and capture change events.

Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests

Committed: https://crrev.com/70fa9571cb0b8081e259749baf210876b211363e
Cr-Commit-Position: refs/heads/master@{#414812}

[riajiang, 2016-08-18 22:52:52]

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

[commit-bot: I haz the power, 2016-08-18 22:54:15]

Dry run: 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-19 00:08:49]

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

[commit-bot: I haz the power, 2016-08-19 00:08:52]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[riajiang, 2016-08-19 00:29:27]

Description was changed from

==========
Update PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests;views_mus_unittests
==========

to

==========
Update PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

[riajiang, 2016-08-19 01:44:26]

Description was changed from

==========
Update PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

to

==========
Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

[yiyix, 2016-08-19 02:05:47]

Patchset #1 (id:1) has been deleted

[riajiang, 2016-08-19 02:52:56]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[riajiang, 2016-08-19 02:52:57]

PTAL. Thanks!

[sadrul, 2016-08-19 04:18:27]

https://codereview.chromium.org/2256343003/diff/20001/ash/wm/immersive_fullsc...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/2256343003/diff/20001/ash/wm/immersive_fullsc...
ash/wm/immersive_fullscreen_controller.cc:282: nullptr);
Do you need this special block? You have already updated the
MouseEvent(PointerEvent) to do the right thing for CAPTURE_CHANGED events. So
the current code should just work?

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:314: void
EventDispatcher::ProcessLocatedEvent(const ui::LocatedEvent& event) {
May we can now call this ProcessPointerEvent(const ui::PointerEvent&)?

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h#newco...
ui/events/event.h:455: int offset_y = 0;
Maybe use a gfx::Vector2dF instead?

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h#newco...
ui/events/event.h:728: // Returns true if |event| is a MouseEvent or TouchEvent.
Something more like: Returns true if a PointerEvent can be constructed from
|event|. Currently, only mouse and touch events can be converted to pointer
events.

[riajiang, 2016-08-19 16:17:59]

Patchset #2 (id:40001) has been deleted

[riajiang, 2016-08-19 16:19:39]

https://codereview.chromium.org/2256343003/diff/20001/ash/wm/immersive_fullsc...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/2256343003/diff/20001/ash/wm/immersive_fullsc...
ash/wm/immersive_fullscreen_controller.cc:282: nullptr);
On 2016/08/19 04:18:27, sadrul wrote:
> Do you need this special block? You have already updated the
> MouseEvent(PointerEvent) to do the right thing for CAPTURE_CHANGED events. So
> the current code should just work?

Removed. I was not getting the right target and location in
PointerWatcherDelegateAura::NotifyWatchers so it was not working without this
block.

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:314: void
EventDispatcher::ProcessLocatedEvent(const ui::LocatedEvent& event) {
On 2016/08/19 04:18:27, sadrul wrote:
> May we can now call this ProcessPointerEvent(const ui::PointerEvent&)?

Right now ScrollEvent is handled by MouseWheelEvent (and then PointerEvent
because of this CL) (crbug.com/602859). Are we going to convert ScrollEvent to
PointerEvent in the future as well? If not then this function needs to process
both PointerEvent and ScrollEvent?

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h#newco...
ui/events/event.h:455: int offset_y = 0;
On 2016/08/19 04:18:27, sadrul wrote:
> Maybe use a gfx::Vector2dF instead?

Done.

https://codereview.chromium.org/2256343003/diff/20001/ui/events/event.h#newco...
ui/events/event.h:728: // Returns true if |event| is a MouseEvent or TouchEvent.
On 2016/08/19 04:18:27, sadrul wrote:
> Something more like: Returns true if a PointerEvent can be constructed from
> |event|. Currently, only mouse and touch events can be converted to pointer
> events.

Done.

[sadrul, 2016-08-19 16:38:59]

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2256343003/diff/20001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:314: void
EventDispatcher::ProcessLocatedEvent(const ui::LocatedEvent& event) {
On 2016/08/19 16:19:39, riajiang wrote:
> On 2016/08/19 04:18:27, sadrul wrote:
> > May we can now call this ProcessPointerEvent(const ui::PointerEvent&)?
> 
> Right now ScrollEvent is handled by MouseWheelEvent (and then PointerEvent
> because of this CL) (crbug.com/602859). Are we going to convert ScrollEvent to
> PointerEvent in the future as well?

Yes. ScrollEvents should be treated identically to wheel events. (see
crbug.com/355659)

> If not then this function needs to process
> both PointerEvent and ScrollEvent?

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

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:91: : GetLocationInScreen(original_event);
Why do you need a valid location for CAPTURE_CHANGED? The CAPTURE_CHANGED events
currently generated (in aura::WindowEventDispatcher) do not have a correct
location. Are there code somewhere that expects these events to carry the
correct location?

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:94: : GetTargetWidget(original_event);
Why special case for CAPTURE_CHANGED? Can we fix GetTargetWidget to return null
if event.target() is null instead?

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h#newco...
ui/events/event.h:417: PointerDetails(EventPointerType pointer_type,
gfx::Vector2d offset)
const gfx::Vector2d&

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h#newco...
ui/events/event.h:453: gfx::Vector2d offset = gfx::Vector2d();
Don't need this. Just 'gfx::Vector2d offset;' (although any reason to not use
Vector2dF?)

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:95: event,
display::Screen::GetScreen()->GetCursorScreenPoint(),
Why not gfx::Point()?

[riajiang, 2016-08-19 17:23:42]

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

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:91: : GetLocationInScreen(original_event);
On 2016/08/19 16:38:58, sadrul wrote:
> Why do you need a valid location for CAPTURE_CHANGED? The CAPTURE_CHANGED
events
> currently generated (in aura::WindowEventDispatcher) do not have a correct
> location. Are there code somewhere that expects these events to carry the
> correct location?

https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller.c...
this code here gets the location, and since there's no OnMouseCaptureChanged()
anymore, I moved it here before calling OnPointerEventObserved (it was in that
special block for capture events). I tried without this once but
https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller_u...
was failing. Did I miss something?

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h#newco...
ui/events/event.h:453: gfx::Vector2d offset = gfx::Vector2d();
On 2016/08/19 16:38:59, sadrul wrote:
> Don't need this. Just 'gfx::Vector2d offset;' (although any reason to not use
> Vector2dF?)

https://cs.chromium.org/chromium/src/ui/events/event.h?q=mousewheeleve&sq=pac...
it says it will be multiples of kWheelData (an integer)?

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:95: event,
display::Screen::GetScreen()->GetCursorScreenPoint(),
On 2016/08/19 16:38:59, sadrul wrote:
> Why not gfx::Point()?

(same as the first reply)

[sadrul, 2016-08-22 14:55:19]

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

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:91: : GetLocationInScreen(original_event);
On 2016/08/19 17:23:42, riajiang wrote:
> On 2016/08/19 16:38:58, sadrul wrote:
> > Why do you need a valid location for CAPTURE_CHANGED? The CAPTURE_CHANGED
> events
> > currently generated (in aura::WindowEventDispatcher) do not have a correct
> > location. Are there code somewhere that expects these events to carry the
> > correct location?
> 
>
https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller.c...
> this code here gets the location, and since there's no OnMouseCaptureChanged()
> anymore, I moved it here before calling OnPointerEventObserved (it was in that
> special block for capture events). I tried without this once but
>
https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller_u...
> was failing. Did I miss something?

Nope. Good catch. I am beginning to think if we should remove screen_location
from the observer interface altogether. I will start a thread about it
separately. In the meantime, can you update GetLocationInScreen() instead to
return the location from display::Screen for CAPTURE_CHANGED?

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:95: event,
display::Screen::GetScreen()->GetCursorScreenPoint(),
On 2016/08/19 17:23:42, riajiang wrote:
> On 2016/08/19 16:38:59, sadrul wrote:
> > Why not gfx::Point()?
> 
> (same as the first reply)

OK. Get the location first, before calling the observers.

[riajiang, 2016-08-22 16:03:05]

PTAL. I'm still getting event location in this CL. I'll remove that param in
PointerWatcher in another CL.

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

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:91: : GetLocationInScreen(original_event);
On 2016/08/22 14:55:19, sadrul wrote:
> On 2016/08/19 17:23:42, riajiang wrote:
> > On 2016/08/19 16:38:58, sadrul wrote:
> > > Why do you need a valid location for CAPTURE_CHANGED? The CAPTURE_CHANGED
> > events
> > > currently generated (in aura::WindowEventDispatcher) do not have a correct
> > > location. Are there code somewhere that expects these events to carry the
> > > correct location?
> > 
> >
>
https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller.c...
> > this code here gets the location, and since there's no
OnMouseCaptureChanged()
> > anymore, I moved it here before calling OnPointerEventObserved (it was in
that
> > special block for capture events). I tried without this once but
> >
>
https://cs.chromium.org/chromium/src/ash/wm/immersive_fullscreen_controller_u...
> > was failing. Did I miss something?
> 
> Nope. Good catch. I am beginning to think if we should remove screen_location
> from the observer interface altogether. I will start a thread about it
> separately. In the meantime, can you update GetLocationInScreen() instead to
> return the location from display::Screen for CAPTURE_CHANGED?

Done.

https://codereview.chromium.org/2256343003/diff/60001/ash/pointer_watcher_del...
ash/pointer_watcher_delegate_aura.cc:94: : GetTargetWidget(original_event);
On 2016/08/19 16:38:58, sadrul wrote:
> Why special case for CAPTURE_CHANGED? Can we fix GetTargetWidget to return
null
> if event.target() is null instead?

Apparently GetTargetWidget already does the right thing for target :)

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h#newco...
ui/events/event.h:417: PointerDetails(EventPointerType pointer_type,
gfx::Vector2d offset)
On 2016/08/19 16:38:59, sadrul wrote:
> const gfx::Vector2d&

Done.

https://codereview.chromium.org/2256343003/diff/60001/ui/events/event.h#newco...
ui/events/event.h:453: gfx::Vector2d offset = gfx::Vector2d();
On 2016/08/19 16:38:59, sadrul wrote:
> Don't need this. Just 'gfx::Vector2d offset;' (although any reason to not use
> Vector2dF?)

Changed to gfx::Vector2d offset;

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
File ui/views/mus/pointer_watcher_event_router.cc (right):

https://codereview.chromium.org/2256343003/diff/60001/ui/views/mus/pointer_wa...
ui/views/mus/pointer_watcher_event_router.cc:95: event,
display::Screen::GetScreen()->GetCursorScreenPoint(),
On 2016/08/22 14:55:19, sadrul wrote:
> On 2016/08/19 17:23:42, riajiang wrote:
> > On 2016/08/19 16:38:59, sadrul wrote:
> > > Why not gfx::Point()?
> > 
> > (same as the first reply)
> 
> OK. Get the location first, before calling the observers.

Done.

[sadrul, 2016-08-22 17:15:57]

lgtm

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:314: void
EventDispatcher::ProcessLocatedEvent(const ui::LocatedEvent& event) {
ProcessPointerEvent(const ui::PointerEvent&)?

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:346: const int32_t pointer_id =
PointerId(event);
Just use |event.pointer_id()| (since |event| will be PointerEvent)

[riajiang, 2016-08-22 20:16:40]

riajiang@chromium.org changed reviewers:
+ dcheng@chromium.org

[riajiang, 2016-08-22 20:16:41]

+dcheng@, could you take a look at mojo/? Thanks.

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:314: void
EventDispatcher::ProcessLocatedEvent(const ui::LocatedEvent& event) {
On 2016/08/22 17:15:57, sadrul wrote:
> ProcessPointerEvent(const ui::PointerEvent&)?

Sorry I forgot. Done.

https://codereview.chromium.org/2256343003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:346: const int32_t pointer_id =
PointerId(event);
On 2016/08/22 17:15:57, sadrul wrote:
> Just use |event.pointer_id()| (since |event| will be PointerEvent)

Done.

[sky, 2016-08-22 20:51:45]

LGTM

https://codereview.chromium.org/2256343003/diff/100001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (left):

https://codereview.chromium.org/2256343003/diff/100001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:38: // PointerWatcher is informed of capture changes
as it's common to create a
Please update the description to indicate capture is sent through
OnPointerEventObserved, and that for capture the target is always null.

[riajiang, 2016-08-22 22:00:19]

https://codereview.chromium.org/2256343003/diff/100001/ui/views/pointer_watch...
File ui/views/pointer_watcher.h (left):

https://codereview.chromium.org/2256343003/diff/100001/ui/views/pointer_watch...
ui/views/pointer_watcher.h:38: // PointerWatcher is informed of capture changes
as it's common to create a
On 2016/08/22 20:51:45, sky wrote:
> Please update the description to indicate capture is sent through
> OnPointerEventObserved, and that for capture the target is always null.

Done.

[jonross, 2016-08-26 14:24:13]

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

[jonross, 2016-08-26 14:24:13]

https://codereview.chromium.org/2256343003/diff/120001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/120001/ui/events/event.h#newc...
ui/events/event.h:141: bool IsMousePointerEvent() const;
Would it be possible to get a variant of this for IsMouseWheelPointerEvent?

It would be convenient for code working with ui/events/blink/web_input_event.h
rather than having the split checks that are currently in the mojo converter.

Heads up: I'm removing the converter, and redirecting CompositorMusConnection to
use the ui/events/blink code.

[dcheng, 2016-08-26 14:45:00]

lgtm

[jonross, 2016-08-26 14:59:05]

https://codereview.chromium.org/2256343003/diff/120001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2256343003/diff/120001/ui/events/event.h#newc...
ui/events/event.h:141: bool IsMousePointerEvent() const;
On 2016/08/26 14:24:13, jonross wrote:
> Would it be possible to get a variant of this for IsMouseWheelPointerEvent?
> 
> It would be convenient for code working with ui/events/blink/web_input_event.h
> rather than having the split checks that are currently in the mojo converter.
> 
> Heads up: I'm removing the converter, and redirecting CompositorMusConnection
to
> use the ui/events/blink code.

Discussed offline, not needed.

[riajiang, 2016-08-26 16:01:24]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-08-26 16:01:25]

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

[commit-bot: I haz the power, 2016-08-26 16:02:22]

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-26 17:25:48]

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

[commit-bot: I haz the power, 2016-08-26 17:25:50]

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

[riajiang, 2016-08-26 20:15:46]

The CQ bit was checked by riajiang@chromium.org

[commit-bot: I haz the power, 2016-08-26 20:16:48]

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-26 21:31:08]

Description was changed from

==========
Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

to

==========
Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

[commit-bot: I haz the power, 2016-08-26 21:31:09]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-08-26 21:33:51]

Description was changed from

==========
Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests
==========

to

==========
Update ui::PointerEvent to support mouse wheel and capture change events.

BUG=638630
TEST=mus_ws_unittests events_unittests blink_converters_unittests
     ash_unittests views_mus_unittests

Committed: https://crrev.com/70fa9571cb0b8081e259749baf210876b211363e
Cr-Commit-Position: refs/heads/master@{#414812}
==========

[commit-bot: I haz the power, 2016-08-26 21:33:53]

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