Pass real tilt information to PointerEvents.

Pass real tilt information to PointerEvents.

This extends platform touch point and pointer event handlers to pass
tilt information to pointer events.

This CL is a part of a patch series:
 1. https://codereview.chromium.org/1253183005/
    for new WebPointerProperties fields
 2. https://codereview.chromium.org/1260693003/
    for eventSender web pointer property support
 3. https://codereview.chromium.org/1192563002/ (this)
 4. https://codereview.chromium.org/1187273004/
    for handling tilt in Chromium event handlers

BUG=514360
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199817

[USE eero AT chromium.org, 2015-06-17 13:39:08]

e.hakkinen@samsung.com changed reviewers:
+ mustaq@chromium.org, rbyers@chromium.org

[USE eero AT chromium.org, 2015-06-17 13:39:09]

PTAL.

Also, please see https://codereview.chromium.org/1187273004/#msg2 about how to
represent tilt.

This should probably wait for https://codereview.chromium.org/1144313003/ to be
landed so that I can squash a two-liner patch passing tiltX and tiltY to
PointerEvent objects into this CL.

[mustaq, 2015-06-17 19:43:28]

https://codereview.chromium.org/1192563002/diff/1/Source/web/WebInputEventCon...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/1192563002/diff/1/Source/web/WebInputEventCon...
Source/web/WebInputEventConversion.cpp:434: if (point.tilt) {
Yes, representing the 3D tilt with two dedicated scalar angles is best solution
IMO, thanks. Since we can't really fix MotionEvent (as we discussed in
crrev.com/1147083005), we should fix the other relevant types in chromium
(WebTouchPoint & PlatformTouchPoint).

Among your 3 pairs of parameter names, I strongly prefer the last pair (tiltX &
tiltY) because these names are clearer in conveying the "3D-ness" of the
parameters, plus they would make it easier to rely on the PE spec for corner
cases.

Please use these names in both WebTouchPoint & PlatformTouchPoint, and move the
angle calculations below to CreateWebTouchPoint() in blink_event_util.cc (in
your other CL crrev.com/1187273004).

https://codereview.chromium.org/1192563002/diff/1/public/web/WebTouchPoint.h
File public/web/WebTouchPoint.h (right):

https://codereview.chromium.org/1192563002/diff/1/public/web/WebTouchPoint.h#...
public/web/WebTouchPoint.h:81: float tiltX;
I think we should use type 'int' for tiltX and tiltY, to mimic the the PE spec.
The precision of 1-degree should be good enough for tilt in any use case, right?

[USE eero AT chromium.org, 2015-06-18 11:34:08]

https://codereview.chromium.org/1192563002/diff/1/Source/web/WebInputEventCon...
File Source/web/WebInputEventConversion.cpp (right):

https://codereview.chromium.org/1192563002/diff/1/Source/web/WebInputEventCon...
Source/web/WebInputEventConversion.cpp:434: if (point.tilt) {
On 2015/06/17 19:43:28, mustaq wrote:
> Yes, representing the 3D tilt with two dedicated scalar angles is best
solution
> IMO, thanks. Since we can't really fix MotionEvent (as we discussed in
> crrev.com/1147083005), we should fix the other relevant types in chromium
> (WebTouchPoint & PlatformTouchPoint).
> 
> Among your 3 pairs of parameter names, I strongly prefer the last pair (tiltX
&
> tiltY) because these names are clearer in conveying the "3D-ness" of the
> parameters, plus they would make it easier to rely on the PE spec for corner
> cases.
> 
> Please use these names in both WebTouchPoint & PlatformTouchPoint, and move
the
> angle calculations below to CreateWebTouchPoint() in blink_event_util.cc (in
> your other CL crrev.com/1187273004).

Done.

https://codereview.chromium.org/1192563002/diff/1/public/web/WebTouchPoint.h
File public/web/WebTouchPoint.h (right):

https://codereview.chromium.org/1192563002/diff/1/public/web/WebTouchPoint.h#...
public/web/WebTouchPoint.h:81: float tiltX;
On 2015/06/17 19:43:28, mustaq wrote:
> I think we should use type 'int' for tiltX and tiltY, to mimic the the PE
spec.
> The precision of 1-degree should be good enough for tilt in any use case,
right?

Done.

[Rick Byers, 2015-06-18 17:10:35]

Wasn't the plan to do this in WebMouseEvent (which we can eventually rename to
WebPointerEvent) rather than WebTouchEvent?  Eg. this isn't really helpful
unless we have a pointerType field somewhere too, right?  I'm not sure it makes
sense to add 'pointer type' to WebTouchEvent, but since pointer events ARE mouse
events, extending WebMouseEvent to become WebPointerEvent does make sense to
me...

I've updated a couple notes in the PE impl doc
(https://docs.google.com/document/d/1keeTmtqTEFS6aQJuEDNpWwsR9qdBfYl8teE35fiTv...).
 Happy to discuss alternate designs tough.

[Rick Byers, 2015-06-18 17:42:13]

On 2015/06/18 17:10:35, Rick Byers (Slow until 6-22) wrote:
> Wasn't the plan to do this in WebMouseEvent (which we can eventually rename to
> WebPointerEvent) rather than WebTouchEvent?  Eg. this isn't really helpful
> unless we have a pointerType field somewhere too, right?  I'm not sure it
makes
> sense to add 'pointer type' to WebTouchEvent, but since pointer events ARE
mouse
> events, extending WebMouseEvent to become WebPointerEvent does make sense to
> me...
> 
> I've updated a couple notes in the PE impl doc
>
(https://docs.google.com/document/d/1keeTmtqTEFS6aQJuEDNpWwsR9qdBfYl8teE35fiTv...).
>  Happy to discuss alternate designs tough.

Ah, just saw https://codereview.chromium.org/1192463008/ - let's discuss further
there.

[mustaq, 2015-06-18 17:55:18]

mustaq@chromium.org changed reviewers:
+ jdduke@chromium.org

[mustaq, 2015-06-18 17:55:46]

On 2015/06/18 17:55:18, mustaq wrote:

Yes, pointerType makes most sense in WebMouseEvent (and hence our future
WebPointerEvent).

Do you have any thoughts about the 3D tilt information we are getting through
ME? As the first stylus data source, ME needs some attention here too. With
current event flow for MEs, WebTouchPoint (or WebTouchEvent) will need to store
both the tilt data & the touch-vs-stylus distinction coming from the Android
side. I was thinking we could "divert" WebTouchEvents somewhere in Blink based
on its type, to the pointer/mouse-event-path for stylus events, and to the
touch-event-path otherwise. After your comment, I now see that we can "divert"
the incoming MEs even at the UI level, to WebMouseEvent/WebPointerEvent and
WebTouchEvent for stylus and non-stylus events respectively.

There could be other possibilities out there.

I have added Jared in this discussion, his idea of deferring browser-side
changes is relevant here.

[Rick Byers, 2015-06-18 21:31:51]

On 2015/06/18 17:55:46, mustaq wrote:
> On 2015/06/18 17:55:18, mustaq wrote:
> 
> Yes, pointerType makes most sense in WebMouseEvent (and hence our future
> WebPointerEvent).
> 
> Do you have any thoughts about the 3D tilt information we are getting through
> ME? As the first stylus data source, ME needs some attention here too. With
> current event flow for MEs, WebTouchPoint (or WebTouchEvent) will need to
store
> both the tilt data & the touch-vs-stylus distinction coming from the Android
> side. I was thinking we could "divert" WebTouchEvents somewhere in Blink based
> on its type, to the pointer/mouse-event-path for stylus events, and to the
> touch-event-path otherwise. After your comment, I now see that we can "divert"
> the incoming MEs even at the UI level, to WebMouseEvent/WebPointerEvent and
> WebTouchEvent for stylus and non-stylus events respectively.

I was originally thinking we'd add the pointertype and tilt to WebMouseEvent
(since that will eventually be renamed to WebPointerEvent).   But it's a bit
more complicated than I thought.  Which ever one (or maybe both) that get
pointertype should also get tilt, etc.  Let's continue the discussion on
https://codereview.chromium.org/1192463008/  since that's the conceptually
bigger change.

> 
> There could be other possibilities out there.
> 
> I have added Jared in this discussion, his idea of deferring browser-side
> changes is relevant here.

[USE eero AT chromium.org, 2015-07-27 11:47:48]

PTAL. Now tilt{X,Y} are in WebPointerProperties.

[Rick Byers, 2015-07-27 14:14:19]

Looks good, thanks!

The BUG # doesn't look right though.  Do we have a bug for stylus support for
Pointer Events on Android? If not, we should probably file one so that all the
"touch-like" stylus work for pointer events can be grouped together.

Also we'll need to add some tests before we can land all of this.  You can
probably add something simple to WebInputConversionTest.  But to test the
plumbing out through PointerEvent we'll probably need a layout test and support
in event_sender.cc.  Since PointerEvents is still at experimental status, I'm OK
with relaxing the normal "land tests in the same CL as the feature" rule here if
it makes it a little easier for you.  I.e. you can land this CL, then
immediately land an event_sender change in chromium to add some way to pass
'tilt' information.  Then land new layout tests in blink that test the tilt on
pointer events.

Alternately the way we'd normally do this is just land a CL that extends only
WebPointerProperties, then land event_sender, then land all the blink changes +
layout tests.

Rick

https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
File public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
public/platform/WebPointerProperties.h:37: int tiltX;
Since these are new, can you please add some comments here briefly describing
their semantics (units, range, etc.)?

[Rick Byers, 2015-07-27 14:16:35]

By the way, check out this cool demo:
https://twitter.com/patrick_h_lauke/status/620727568090558464

You should see if you can make this exact demo work on Chrome on Android :-)

[USE eero AT chromium.org, 2015-07-27 17:53:36]

On 2015/07/27 14:14:19, Rick Byers wrote:
> The BUG # doesn't look right though.  Do we have a bug for stylus support for
> Pointer Events on Android? If not, we should probably file one so that all the
> "touch-like" stylus work for pointer events can be grouped together.

Is http://code.google.com/p/chromium/issues/detail?id=508283 better?

> Alternately the way we'd normally do this is just land a CL that extends only
> WebPointerProperties, then land event_sender, then land all the blink changes
+
> layout tests.

As everything is needed, order is not so important. So I created two new CLs:
 * https://codereview.chromium.org/1253183005/
 * https://codereview.chromium.org/1260693003/

https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
File public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
public/platform/WebPointerProperties.h:37: int tiltX;
On 2015/07/27 14:14:19, Rick Byers wrote:
> Since these are new, can you please add some comments here briefly describing
> their semantics (units, range, etc.)?

Done.

[Rick Byers, 2015-07-27 20:44:56]

On 2015/07/27 17:53:36, e_hakkinen wrote:
> On 2015/07/27 14:14:19, Rick Byers wrote:
> > The BUG # doesn't look right though.  Do we have a bug for stylus support
for
> > Pointer Events on Android? If not, we should probably file one so that all
the
> > "touch-like" stylus work for pointer events can be grouped together.
> 
> Is http://code.google.com/p/chromium/issues/detail?id=508283 better?

Mustaq filed that for the specific long-term issue of how we unify WebMouseEvent
and WebTouchEvent into a single thing (won't solve that anytime soon).  I just
filed http://crbug.com/514360 - let's use that.

> > Alternately the way we'd normally do this is just land a CL that extends
only
> > WebPointerProperties, then land event_sender, then land all the blink
changes
> +
> > layout tests.
> 
> As everything is needed, order is not so important. So I created two new CLs:
>  * https://codereview.chromium.org/1253183005/
>  * https://codereview.chromium.org/1260693003/

Ok, will review there.  Please cross-link the CLs in the CL descriptions for the
sake of sheriffs who may need to understand the dependencies at a glance.  I.e.
"Depends on: http://crrev.com/1253183005, blocking http://crrev.com/1260693003".

>
https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
> File public/platform/WebPointerProperties.h (right):
> 
>
https://codereview.chromium.org/1192563002/diff/60001/public/platform/WebPoin...
> public/platform/WebPointerProperties.h:37: int tiltX;
> On 2015/07/27 14:14:19, Rick Byers wrote:
> > Since these are new, can you please add some comments here briefly
describing
> > their semantics (units, range, etc.)?
> 
> Done.

[Rick Byers, 2015-07-30 17:53:15]

Nice test.  So we don't need to add an expected output file in
virtual/pointerevents/, since it's a testharness.js test - right?  I kicked off
a linux_blink_dbg test run (now before your necessary eventsender changes have
landed) just to double check that the test will indeed fail (the lack of
expected output for passing testharness.js tests always makes me a little
paranoid).

LGTM (but you of course won't be able to land until your eventsender changes
land in chromium).

[Rick Byers, 2015-07-30 17:53:45]

On 2015/07/30 17:53:15, Rick Byers (Out until Aug 4th) wrote:
> Nice test.  So we don't need to add an expected output file in
> virtual/pointerevents/, since it's a testharness.js test - right?  I kicked
off
> a linux_blink_dbg test run (now before your necessary eventsender changes have
> landed) just to double check that the test will indeed fail (the lack of
> expected output for passing testharness.js tests always makes me a little
> paranoid).
> 
> LGTM (but you of course won't be able to land until your eventsender changes
> land in chromium).

Oh, Mustaq did you have any other feedback on this?

[mustaq, 2015-07-30 19:38:17]

On 2015/07/30 17:53:45, Rick Byers (Out until Aug 4th) wrote:
> On 2015/07/30 17:53:15, Rick Byers (Out until Aug 4th) wrote:
> > Nice test.  So we don't need to add an expected output file in
> > virtual/pointerevents/, since it's a testharness.js test - right?  I kicked
> off
> > a linux_blink_dbg test run (now before your necessary eventsender changes
have
> > landed) just to double check that the test will indeed fail (the lack of
> > expected output for passing testharness.js tests always makes me a little
> > paranoid).
> > 
> > LGTM (but you of course won't be able to land until your eventsender changes
> > land in chromium).
> 
> Oh, Mustaq did you have any other feedback on this?

I think virtual tests need separate expectations, even for testharness.js based
tests. Eero, could you please confirm?

[mustaq, 2015-07-30 20:18:21]

https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html
(right):

https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html:58: },
"Peceived pointer events");
Typo: "Received..."

[mustaq, 2015-07-30 21:30:07]

On 2015/07/30 20:18:21, mustaq wrote:
>
https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
> File LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html
> (right):
> 
>
https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
> LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html:58:
},
> "Peceived pointer events");
> Typo: "Received..."

Just ran the test with PE enabled, seems working fine.

LGTM modulo my 2 comments above.

[USE eero AT chromium.org, 2015-07-31 09:37:12]

On 2015/07/30 17:53:15, Rick Byers (Out until Aug 4th) wrote:
> Nice test.  So we don't need to add an expected output file in
> virtual/pointerevents/, since it's a testharness.js test - right?

That would be the case if there was only passing expected output files
but that does not hold in this case.

On 2015/07/30 19:38:17, mustaq wrote:
> I think virtual tests need separate expectations, even for testharness.js
based
> tests. Eero, could you please confirm?

You are right. Because there is extected output file in fast/, virtual
layout test fails without extected output file in virtual/pointerevents/fast/.
If there were no extected output file in fast/, virtual layout test would
pass and non-virtual fail would fail (regardless of whether there were
extected output file in virtual/pointerevents/fast/).

So a added one.

https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
File LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html
(right):

https://codereview.chromium.org/1192563002/diff/120001/LayoutTests/fast/event...
LayoutTests/fast/events/pointerevents/touch-pointer-event-properties.html:58: },
"Peceived pointer events");
On 2015/07/30 20:18:21, mustaq wrote:
> Typo: "Received..."

Done.

[USE eero AT chromium.org, 2015-07-31 09:37:31]

The CQ bit was checked by e.hakkinen@samsung.com

[USE eero AT chromium.org, 2015-07-31 09:37:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1192563002/#ps140001
(title: "Spelling, passing expected.txt")

[commit-bot: I haz the power, 2015-07-31 09:37:47]

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

[commit-bot: I haz the power, 2015-07-31 09:42:46]

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

[commit-bot: I haz the power, 2015-07-31 09:42:47]

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

[USE eero AT chromium.org, 2015-07-31 10:40:47]

e.hakkinen@samsung.com changed reviewers:
+ bokan@chromium.org, eae@chromium.org, haraken@chromium.org

[USE eero AT chromium.org, 2015-07-31 10:44:47]

+@bokan, +@haraken: Source/web/WebInputEventConversion.cpp
+@eae, +@haraken: Source/platform/PlatformTouchPoint.h

[haraken, 2015-07-31 10:58:53]

On 2015/07/31 10:44:47, e_hakkinen wrote:
> +@bokan, +@haraken: Source/web/WebInputEventConversion.cpp
> +@eae, +@haraken: Source/platform/PlatformTouchPoint.h

web/ and platform/ LGTM

[USE eero AT chromium.org, 2015-07-31 11:32:40]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2015-07-31 11:33:05]

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

[commit-bot: I haz the power, 2015-07-31 11:36:36]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199817