Pass MotionEvent tilt angles to Blink on Android.

Moved: https://codereview.chromium.org/1417803002/

[USE eero AT chromium.org, 2015-06-17 13:35:54]

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

[USE eero AT chromium.org, 2015-06-17 13:35:55]

PTAL.

This still needs a decision on how to pass tilt information to Blink i.e. where
to convert spherical tilt angles to tiltX and tiltY (used by PEs).

1) It is possible to do the conversion in MotionEventAndroid and pass only tiltX
and tiltY to Blink.

2) It is possible not to convert anything in Chromium and pass tilt and tilt
orientation as radians to Blink so that Blink can do the conversion. That would
result in some WebTouchPoint angles to be degrees and some to be in radians,
though.

3) It is possible to convert tilt and tilt orientation from radians to degrees
and pass tilt and tilt orientation as degrees to Blink. But that is kind of
useless as nothing on Blink side needs tilt or tilt orientation as degrees.

As conversion from spherical angles to tiltX and tiltY is non-trivial, it would
be nice if MotionEvent and WebTouchPoint used the same representation.

[mustaq, 2015-06-17 20:01:27]

https://codereview.chromium.org/1187273004/diff/1/ui/events/blink/blink_event...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/1187273004/diff/1/ui/events/blink/blink_event...
ui/events/blink/blink_event_util.cc:106: if (event.GetToolType(pointer_index) ==
MotionEvent::TOOL_TYPE_STYLUS) {
We should convert MotionEvent orientation+tilt to WebTouchPoint tiltx+tilty
here, see my comment in crrev.com/1192563002.

https://codereview.chromium.org/1187273004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/1187273004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/motion_event.h:67: virtual float GetTilt(size_t
pointer_index) const = 0;
Since the discussion in crrev.com/1147083005 showed a strong support to keep the
MotionEvent interface as close to the Android spec as possible, I would add only
GetTilt() here, and untangle the overloaded orientation angle while converting
to other types, as I commented in blink_event_util.cc.

Sorry for my changed position in this regard.

Do you know if blink_event_util.cc is the only place to convert from MotionEvent
to other point-types?

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

https://codereview.chromium.org/1187273004/diff/1/ui/events/blink/blink_event...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/1187273004/diff/1/ui/events/blink/blink_event...
ui/events/blink/blink_event_util.cc:106: if (event.GetToolType(pointer_index) ==
MotionEvent::TOOL_TYPE_STYLUS) {
On 2015/06/17 20:01:27, mustaq wrote:
> We should convert MotionEvent orientation+tilt to WebTouchPoint tiltx+tilty
> here, see my comment in crrev.com/1192563002.

Done.

https://codereview.chromium.org/1187273004/diff/1/ui/events/gesture_detection...
File ui/events/gesture_detection/motion_event.h (right):

https://codereview.chromium.org/1187273004/diff/1/ui/events/gesture_detection...
ui/events/gesture_detection/motion_event.h:67: virtual float GetTilt(size_t
pointer_index) const = 0;
On 2015/06/17 20:01:27, mustaq wrote:
> Since the discussion in crrev.com/1147083005 showed a strong support to keep
the
> MotionEvent interface as close to the Android spec as possible, I would add
only
> GetTilt() here, and untangle the overloaded orientation angle while converting
> to other types, as I commented in blink_event_util.cc.

Done.

Initially I thought that WebTouchPoint to MotionEventWeb conversion very
complex,
but that conversion is actually used only for touch events and even if it would
be extended to all pointer events, conversion would not be that complex.
It would be enough to check signs of tiltX and tiltY. Trigonometry would not be
needed.

> Do you know if blink_event_util.cc is the only place to convert from
MotionEvent
> to other point-types? 

Seems to be so.

[USE eero AT chromium.org, 2015-07-31 13:43:45]

The CQ bit was checked by e.hakkinen@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-31 13:43:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1187273004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1187273004/80001

[USE eero AT chromium.org, 2015-07-31 13:47:53]

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

[commit-bot: I haz the power, 2015-07-31 14:48:10]

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

[commit-bot: I haz the power, 2015-07-31 14:48:12]

Dry run: This issue passed the CQ dry run.

[USE eero AT chromium.org, 2015-07-31 14:51:13]

PTAL. Blink side CLs have finally landed (and rolled into DEPS).

[jdduke (slow), 2015-07-31 15:00:49]

Just some nits. Is there an outstanding patch to add tilt support for
non-Android platforms? You might want to make a note in the description that
this passing is for Android only currently.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:42: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
Nit: might as well give the different pointers different tilt values, just to
make sure we're getting indexing right in the getter/setter.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:199: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
Same here, let's give the different pointers different tilt values.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:153: return 0.f;
Hmm, can't we get this value from the WebTouchEvent?

[USE eero AT chromium.org, 2015-07-31 18:18:15]

On 2015/07/31 15:00:49, jdduke wrote:
> Just some nits. Is there an outstanding patch to add tilt support for
> non-Android platforms?

Not yet.

> You might want to make a note in the description that
> this passing is for Android only currently.

I changed the description.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:42: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
On 2015/07/31 15:00:49, jdduke wrote:
> Nit: might as well give the different pointers different tilt values, just to
> make sure we're getting indexing right in the getter/setter.

Done.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_android_unittest.cc:199: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
On 2015/07/31 15:00:49, jdduke wrote:
> Same here, let's give the different pointers different tilt values.

Done.

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:153: return 0.f;
On 2015/07/31 15:00:49, jdduke wrote:
> Hmm, can't we get this value from the WebTouchEvent?

Yes, although it is a bit complex. Also this member function will not be called
yet as the GetToolType member function does not return real tool type yet (but
only after https://crrev.com/1190013002/). But let's implement this now, thus
done.

[mustaq, 2015-08-04 14:47:55]

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

[mustaq, 2015-08-04 14:47:55]

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
content/browser/renderer_host/input/motion_event_web.cc:177: float r_x =
tilt_x_r * tilt_y_z;
Just out of curiosity: do you have any sketch to explain this math? This works,
but I am personally trying to figure out the math behind the "scaling" for z.

[Rick Byers, 2015-08-04 15:31:16]

On 2015/07/31 at 18:18:15, e.hakkinen wrote:
> On 2015/07/31 15:00:49, jdduke wrote:
> > Just some nits. Is there an outstanding patch to add tilt support for
> > non-Android platforms?
> 
> Not yet.

Note that there's a pretty fundamental difference between "touch-like stylus
support" (i.e. where the stylus uses our GR) and "mouse-like stylus support"
(i.e. where the stylus will trigger mouse behavior like text selection).  I'm OK
with us investing in the former during Q3 and leaving the latter for later,
which is almost the same as saying we're doing only Android stylus work for now
(which is what our Q3 OKR says) - except that http://crbug.com/448467 indicates
Ozone on-screen stylus should be the same too.  So one way or another, I agree
we should be working on "touch-like stylus support" generally, not
Android-specific.  I'll follow up in bug 448467 to figure out the plan for
Ozone.
 
> > You might want to make a note in the description that
> > this passing is for Android only currently.
> 
> I changed the description.
> 
>
https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/input/motion_event_android_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_android_unittest.cc:42: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
> On 2015/07/31 15:00:49, jdduke wrote:
> > Nit: might as well give the different pointers different tilt values, just
to
> > make sure we're getting indexing right in the getter/setter.
> 
> Done.
> 
>
https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_android_unittest.cc:199: 1,
13.7f, -7.13f, 5.3f, 1.2f, 0.1f, 0.2f, kAndroidToolTypeFinger);
> On 2015/07/31 15:00:49, jdduke wrote:
> > Same here, let's give the different pointers different tilt values.
> 
> Done.
> 
>
https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
> File content/browser/renderer_host/input/motion_event_web.cc (right):
> 
>
https://codereview.chromium.org/1187273004/diff/80001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_web.cc:153: return 0.f;
> On 2015/07/31 15:00:49, jdduke wrote:
> > Hmm, can't we get this value from the WebTouchEvent?
> 
> Yes, although it is a bit complex. Also this member function will not be
called yet as the GetToolType member function does not return real tool type yet
(but only after https://crrev.com/1190013002/). But let's implement this now,
thus done.

[USE eero AT chromium.org, 2015-08-05 15:12:21]

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
content/browser/renderer_host/input/motion_event_web.cc:177: float r_x =
tilt_x_r * tilt_y_z;
On 2015/08/04 14:47:55, mustaq wrote:
> Just out of curiosity: do you have any sketch to explain this math? This
works,
> but I am personally trying to figure out the math behind the "scaling" for z.

Not a simplest conversion.

Let's take a unit normal vector n = (0, 0, 1).
Let's tilt the normal from XZ-plane around X-axis along YZ-plane with tiltY.
That results in
  u = (0, tilt_y_r * n_z, tilt_y_z * n_z)
    = (0, tilt_y_r, tilt_y_z).
In order to avoid tilting along a tilted plane (which is complicated),
let's project the result back to XZ-plane. That results in
  v = (u_x, 0, u_z)
    = (0, 0, tilt_y_z)
Let's tilt the result from YZ-plane around Y-axis along XZ-plane with tiltX.
That results in
  w = (tilt_x_r * v_z, 0, tilt_x_z * v_z)
    = (tilt_x_r * tilt_y_z, 0, tilt_x_z * tilt_y_z)
Thus
  r_x = tilt_x_r * tilt_y_z
  r_x = tilt_y_r * tilt_x_z  # Analogically.
  z   = tilt_x_z * tilt_y_z

[USE eero AT chromium.org, 2015-08-12 16:10:06]

Is there something I can do to help this to get reviewed?

[jdduke (slow), 2015-08-12 17:00:13]

https://codereview.chromium.org/1187273004/diff/120001/content/browser/render...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/120001/content/browser/render...
content/browser/renderer_host/input/motion_event_web.cc:140: 
I think we'll want some basic test coverage for this. Doesn't need to be
exhaustive, but we should have at least a couple smoke test cases.

https://codereview.chromium.org/1187273004/diff/120001/content/browser/render...
content/browser/renderer_host/input/motion_event_web.cc:170: 
Same here, a few basic test cases would be nice. We haven't added tests for this
class until now as most of the getters are trivial, but it's probably time we
add some coverage.

Ideally we'd have a case where we construct a MotionEventGeneric with some
tilt/orientation values, we run that through
CreateWebTouchEventFromMotionEvent(), then create a MotionEventWeb from the
result. The result of GetTilt/GetOrientation should be the same.

https://codereview.chromium.org/1187273004/diff/120001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/1187273004/diff/120001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.cc:137: // A stylus points to a direction
specified by orientation and tilts to
Same here, I think we want some basic smoke test coverage.

[mustaq, 2015-08-13 18:03:29]

Rubberstamp lgtm for the math.

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1187273004/diff/100001/content/browser/render...
content/browser/renderer_host/input/motion_event_web.cc:177: float r_x =
tilt_x_r * tilt_y_z;
On 2015/08/05 15:12:21, e_hakkinen wrote:
> On 2015/08/04 14:47:55, mustaq wrote:
> > Just out of curiosity: do you have any sketch to explain this math? This
> works,
> > but I am personally trying to figure out the math behind the "scaling" for
z.
> 
> Not a simplest conversion.
> 
> Let's take a unit normal vector n = (0, 0, 1).
> Let's tilt the normal from XZ-plane around X-axis along YZ-plane with tiltY.
> That results in
>   u = (0, tilt_y_r * n_z, tilt_y_z * n_z)
>     = (0, tilt_y_r, tilt_y_z).
> In order to avoid tilting along a tilted plane (which is complicated),
> let's project the result back to XZ-plane. That results in
>   v = (u_x, 0, u_z)
>     = (0, 0, tilt_y_z)
> Let's tilt the result from YZ-plane around Y-axis along XZ-plane with tiltX.
> That results in
>   w = (tilt_x_r * v_z, 0, tilt_x_z * v_z)
>     = (tilt_x_r * tilt_y_z, 0, tilt_x_z * tilt_y_z)
> Thus
>   r_x = tilt_x_r * tilt_y_z
>   r_x = tilt_y_r * tilt_x_z  # Analogically.
>   z   = tilt_x_z * tilt_y_z

Just verified the correctness of your math, thanks. Sorry for the delay, the
last two lines gave me hard times. I will dump my explanation here, for future
reference.

The vector (0,0,1) after the two rotations lands in two different locations:
- rotate around X-axis then around Y-axis: (xr*yz, yr, xz*yz),
- rotate around Y-axis then around X-axis: (xr, yr*xz, yz*xz).

The intersection of the lines (xr*yz, y, xz*yz) and (x, yr*xz, yz*xz) is enough
for us here because any point on the first line is on the plane defined by angle
tiltX, and any point on the second line is on the plane defined by angle tiltY.

[jdduke (slow), 2015-10-05 15:41:33]

Any updates here?

[e_hakkinen, 2015-10-09 13:29:15]

On 2015/10/05 15:41:33, jdduke wrote:
> Any updates here?

Sorry for a delay caused by an embargo.
I am back to work and will prioritise finishing this change.

[e_hakkinen, 2015-10-21 08:13:35]

As the owner of an issua cannot be changed, let's continue review in here:
  https://codereview.chromium.org/1417803002/

Sorry for any inconvenient caused by that.

[USE eero AT chromium.org, 2015-10-21 08:15:04]

Description was changed from

==========
Pass MotionEvent tilt angles to Blink on Android.

This extends event handlers to pass tilt angles from Android MotionEvent
to Blink touch points, so that on the Blink side, PointerEvent tiltX and
tiltY can be filled with real tilt angles.

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/
    for handling tilt in Blink event handlers
 4. https://codereview.chromium.org/1187273004/ (this)

BUG=514360
==========

to

==========
Moved: https://codereview.chromium.org/1417803002/
==========

[USE eero AT chromium.org, 2015-10-21 08:15:05]

e.hakkinen@samsung.com changed reviewers:
- jdduke@chromium.org, mustaq@chromium.org, sadrul@chromium.org