Pass MotionEvent tilt angles to Blink on Android.

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/1417803002/
    (this)

This CL is based on the abandoned CL at
https://codereview.chromium.org/1187273004/

BUG=514360
Committed: https://crrev.com/b39a02032b11742cc467f43026a77a0aba33d5c1
Cr-Commit-Position: refs/heads/master@{#357471}

[e_hakkinen, 2015-10-20 20:55:03]

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

==========
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/1417803002/
    (this)

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

[e_hakkinen, 2015-10-20 20:55:03]

eero.hakkinen@intel.com changed reviewers:
+ jdduke@chromium.org, mustaq@chromium.org, sadrul@chromium.org

[eero, 2015-10-20 20:59:07]

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

[commit-bot: I haz the power, 2015-10-20 21:00:28]

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

[commit-bot: I haz the power, 2015-10-20 21:10:56]

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

[commit-bot: I haz the power, 2015-10-20 21:10:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[e_hakkinen, 2015-10-22 06:39:07]

PTAL. I added some unit tests as instructed in
   https://codereview.chromium.org/1187273004/
and sorted out compilation problems on different platforms.

[e_hakkinen, 2015-10-30 13:57:12]

eero.hakkinen@intel.com changed reviewers:
+ aelias@chromium.org, tdresser@chromium.org, tedchoc@chromium.org,
yfriedman@google.com
- jdduke@chromium.org

[e_hakkinen, 2015-10-30 14:01:09]

+@tedchoc,@yfriedman: content/{browser,public}/android/
+@aelias,@tdresser: content/{browser/renderer_host,common}/input/

PTAL.

[mustaq, 2015-10-30 14:58:15]

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
content/browser/renderer_host/input/motion_event_web.cc:173: float tilt_x_r =
sin(pointer.tiltX * M_PI / 180.f);
Rubberstamp LGTM on the correctness of the math here, as in the previous CL.

Please add a link to the previous CL in this CL's description, for future ref.

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
File content/browser/renderer_host/input/motion_event_web_unittest.cc (right):

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
content/browser/renderer_host/input/motion_event_web_unittest.cc:61: //
conversions only if there is a non-zero stylus tilt angle.
Please add a ref to crbug.com/493728 here. I think your tiltx/y calculation now
fully captures the stylus info in ME, nothing is lost really.

Let's discuss it in the bug, this shouldn't block the CL.

[e_hakkinen, 2015-10-30 15:09:10]

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/1417803002/
    (this)

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

to

==========
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/1417803002/
    (this)

This CL is based on the abandoned CL at
https://codereview.chromium.org/1187273004/

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

[e_hakkinen, 2015-10-30 15:45:25]

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
File content/browser/renderer_host/input/motion_event_web.cc (right):

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
content/browser/renderer_host/input/motion_event_web.cc:173: float tilt_x_r =
sin(pointer.tiltX * M_PI / 180.f);
On 2015/10/30 14:58:14, mustaq wrote:
> Please add a link to the previous CL in this CL's description, for future ref.

Done.

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
File content/browser/renderer_host/input/motion_event_web_unittest.cc (right):

https://codereview.chromium.org/1417803002/diff/2/content/browser/renderer_ho...
content/browser/renderer_host/input/motion_event_web_unittest.cc:61: //
conversions only if there is a non-zero stylus tilt angle.
On 2015/10/30 14:58:14, mustaq wrote:
> Please add a ref to crbug.com/493728 here. I think your tiltx/y calculation
now
> fully captures the stylus info in ME, nothing is lost really.
> 
> Let's discuss it in the bug, this shouldn't block the CL.

Done.

[aelias_OOO_until_Jul13, 2015-10-30 15:47:38]

lgtm

[sadrul, 2015-10-30 15:50:05]

ui/events/blink/ ui/events/test lgtm

[tdresser, 2015-11-02 14:44:07]

LGTM.

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

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:141: if (pointer.tiltY <
0) {
I think this would be easier to read if you explicitly check all four quadrants.

i.e.

if (pointer.tiltY < 0 && pointer.tiltX < 0)
  ...
else if(pointer.tiltY < 0 && pointer.tiltX > 0)
  ...
else if(pointer.tiltY > 0 && pointer.tiltX < 0)
  ...
else
  ...

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:154: // always seem to
be set to zero) unchanged.
While you're editing this, can you change:
"always seem" -> "always seems"?

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util_unittest.cc
(right):

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util_unittest.cc:28:
namespace content {
Leave space after namespace.

[e_hakkinen, 2015-11-02 16:27:17]

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

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:141: if (pointer.tiltY <
0) {
On 2015/11/02 14:44:07, tdresser wrote:
> I think this would be easier to read if you explicitly check all four
quadrants.

Yes, especially because the grouping was not actually entirely correct.
The first condition should be
  if (pointer.tiltY <= 0 && pointer.tiltX < 0)
instead of
  if (pointer.tiltY < 0 && pointer.tiltX < 0)
thus this code did not handle correctly the case when a stylus was tilted
straight to the left.

I flattened the nested if blocks and amended the unit test to check that special
case, too.

> 
> i.e.
> 
> if (pointer.tiltY < 0 && pointer.tiltX < 0)
>   ...
> else if(pointer.tiltY < 0 && pointer.tiltX > 0)
>   ...
> else if(pointer.tiltY > 0 && pointer.tiltX < 0)
>   ...
> else
>   ...

Done.

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/motion_event_web.cc:154: // always seem to
be set to zero) unchanged.
On 2015/11/02 14:44:07, tdresser wrote:
> While you're editing this, can you change:
> "always seem" -> "always seems"?

Done.

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
File content/browser/renderer_host/input/web_input_event_util_unittest.cc
(right):

https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
content/browser/renderer_host/input/web_input_event_util_unittest.cc:28:
namespace content {
On 2015/11/02 14:44:07, tdresser wrote:
> Leave space after namespace.

Done.

[Ted C, 2015-11-02 19:21:10]

On 2015/11/02 16:27:17, e_hakkinen wrote:
>
https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
> File content/browser/renderer_host/input/motion_event_web.cc (right):
> 
>
https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_web.cc:141: if (pointer.tiltY
<
> 0) {
> On 2015/11/02 14:44:07, tdresser wrote:
> > I think this would be easier to read if you explicitly check all four
> quadrants.
> 
> Yes, especially because the grouping was not actually entirely correct.
> The first condition should be
>   if (pointer.tiltY <= 0 && pointer.tiltX < 0)
> instead of
>   if (pointer.tiltY < 0 && pointer.tiltX < 0)
> thus this code did not handle correctly the case when a stylus was tilted
> straight to the left.
> 
> I flattened the nested if blocks and amended the unit test to check that
special
> case, too.
> 
> > 
> > i.e.
> > 
> > if (pointer.tiltY < 0 && pointer.tiltX < 0)
> >   ...
> > else if(pointer.tiltY < 0 && pointer.tiltX > 0)
> >   ...
> > else if(pointer.tiltY > 0 && pointer.tiltX < 0)
> >   ...
> > else
> >   ...
> 
> Done.
> 
>
https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
> content/browser/renderer_host/input/motion_event_web.cc:154: // always seem to
> be set to zero) unchanged.
> On 2015/11/02 14:44:07, tdresser wrote:
> > While you're editing this, can you change:
> > "always seem" -> "always seems"?
> 
> Done.
> 
>
https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
> File content/browser/renderer_host/input/web_input_event_util_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/1417803002/diff/70001/content/browser/rendere...
> content/browser/renderer_host/input/web_input_event_util_unittest.cc:28:
> namespace content {
> On 2015/11/02 14:44:07, tdresser wrote:
> > Leave space after namespace.
> 
> Done.

content/{browser,public}/android/ - lgtm

[e_hakkinen, 2015-11-02 22:48:41]

The CQ bit was checked by eero.hakkinen@intel.com

[e_hakkinen, 2015-11-02 22:48:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
aelias@chromium.org, sadrul@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1417803002/#ps90001
(title: "Flattened nested ifs, fixed a special case")

[commit-bot: I haz the power, 2015-11-02 22:49:24]

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

[commit-bot: I haz the power, 2015-11-02 23:50:18]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-11-02 23:51:10]

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