Separate motion event touch geometry orientation from stylus orientation.

Separate motion event touch geometry orientation from stylus orientation.

On Android, motion event orientation is overloaded thus it has different
meanings and ranges for touch screens and pads and for styluses. As that
overloading is harmful, do not carry it on but break it in
AndroidMotionEvent.

This change aligns touch geometry rotation angles with TouchEvent spec
for cases when stylus orientation does not fall in [-90, 90] range.

BUG=492239

[USE eero AT chromium.org, 2015-05-27 08:43:48]

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

[USE eero AT chromium.org, 2015-05-27 08:43:49]

PTAL, if this would be a better fix for the issue than
https://codereview.chromium.org/1152463004/

https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/motion_event_android.h (right):

https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/motion_event_android.h:93: float
GetOrientation(size_t pointer_index) const;
I keep this as a separate function as it is useful for getting a tilt
orientation.

[tdresser, 2015-05-27 11:56:01]

On 2015/05/27 08:43:49, e_hakkinen wrote:
> PTAL, if this would be a better fix for the issue than
> https://codereview.chromium.org/1152463004/
> 
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/input/motion_event_android.h (right):
> 
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/input/motion_event_android.h:93: float
> GetOrientation(size_t pointer_index) const;
> I keep this as a separate function as it is useful for getting a tilt
> orientation.

I'd prefer that we keep MotionEvent aligned as closely as possible with
Android's MotionEvent, unless we have a good reason not to. I'm in favor of the
other patch.

[tdresser, 2015-05-27 13:59:45]

On 2015/05/27 11:56:01, tdresser wrote:
> On 2015/05/27 08:43:49, e_hakkinen wrote:
> > PTAL, if this would be a better fix for the issue than
> > https://codereview.chromium.org/1152463004/
> > 
> >
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/input/motion_event_android.h (right):
> > 
> >
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/input/motion_event_android.h:93: float
> > GetOrientation(size_t pointer_index) const;
> > I keep this as a separate function as it is useful for getting a tilt
> > orientation.
> 
> I'd prefer that we keep MotionEvent aligned as closely as possible with
> Android's MotionEvent, unless we have a good reason not to. I'm in favor of
the
> other patch.

I talked to mustaq@ about this offline - I'm fine with this approach.
The current Android behavior is pretty confusing.

[mustaq, 2015-05-27 14:22:53]

On 2015/05/27 13:59:45, tdresser wrote:
> On 2015/05/27 11:56:01, tdresser wrote:
> > On 2015/05/27 08:43:49, e_hakkinen wrote:
> > > PTAL, if this would be a better fix for the issue than
> > > https://codereview.chromium.org/1152463004/
> > > 
> > >
> >
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> > > File content/browser/renderer_host/input/motion_event_android.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1147083005/diff/1/content/browser/renderer_ho...
> > > content/browser/renderer_host/input/motion_event_android.h:93: float
> > > GetOrientation(size_t pointer_index) const;
> > > I keep this as a separate function as it is useful for getting a tilt
> > > orientation.
> > 
> > I'd prefer that we keep MotionEvent aligned as closely as possible with
> > Android's MotionEvent, unless we have a good reason not to. I'm in favor of
> the
> > other patch.
> 
> I talked to mustaq@ about this offline - I'm fine with this approach.
> The current Android behavior is pretty confusing.

Thanks Tim. Yes, I think this is a better solution because a different field
name from the Android spec makes it clear that ui::MotionEvent breaks the
overloaded "meaning" of the field.

Two suggestions:
- I would prefer preserving the original name as a prefix, like
"getOrientationForTouch" (and "getOrientationForStylus" in future), to make the
complementary nature of these two fields a bit more obvious.
- Please make the comment for GetTouchOrientation a bit more explicit about
touch, like "Returns the orientation of the touch surface geometry. ...".

[Rick Byers, 2015-05-27 14:30:52]

rbyers@chromium.org changed reviewers:
+ jdduke@chromium.org
- nick@semenkovich.com, rbyers@chromium.org, sky@chromium.org

[Rick Byers, 2015-05-27 14:32:54]

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

[Rick Byers, 2015-05-27 14:32:54]

This approach seems good to me, I like the explicitness for avoiding confusion
here and I think it justifies the minor deviation from MotionEvent (if it didn't
have app compat impact, then I'd say the Android API should change too - but I
doubt it's worth the hassle given the old API must stay around).

+jdduke

[jdduke (slow), 2015-05-27 15:52:03]

On 2015/05/27 14:32:54, Rick Byers wrote:
> This approach seems good to me, I like the explicitness for avoiding confusion
> here and I think it justifies the minor deviation from MotionEvent (if it
didn't
> have app compat impact, then I'd say the Android API should change too - but I
> doubt it's worth the hassle given the old API must stay around).
> 
> +jdduke

Why can't we just relax the range restriction for GetOrientation()? The only
difference really between stylus/touch here is that stylus has an extra bit that
allows us to distinguish between "mirrored" orientations, i.e., we can
distinguish between the forward/backward directions of the pointer. Tilt is a
separate property altogether. I have a slight preference for losing the bit when
converting to WebTouchEvent than losing it in the orientation getter.

[mustaq, 2015-05-27 17:03:48]

On 2015/05/27 15:52:03, jdduke wrote:
> On 2015/05/27 14:32:54, Rick Byers wrote:
> > This approach seems good to me, I like the explicitness for avoiding
confusion
> > here and I think it justifies the minor deviation from MotionEvent (if it
> didn't
> > have app compat impact, then I'd say the Android API should change too - but
I
> > doubt it's worth the hassle given the old API must stay around).
> > 
> > +jdduke
> 
> Why can't we just relax the range restriction for GetOrientation()? The only
> difference really between stylus/touch here is that stylus has an extra bit
that
> allows us to distinguish between "mirrored" orientations, i.e., we can
> distinguish between the forward/backward directions of the pointer. Tilt is a
> separate property altogether. I have a slight preference for losing the bit
when
> converting to WebTouchEvent than losing it in the orientation getter.

Did you mean relaxing the range restriction for GetOrientation()
unconditionally, or just for stylus? If you meant unconditionally, then we are
going off the spec for touch which we should clearly avoid (in addition to
developers' hassles resulting from possible non-canonical representation of a
touch geometry). If you meant only for stylus, we need a way to distinguish this
case from the touch case. We can store an extra bit in ui::MotionEvent to mark
the distingsion (e.g. 'isStylus') but separating the two "meanings" into
separate fields is much cleaner IMO.

Most importantly, the difference between touch vs stylus geometry is more than
just a bit, it's one extra dimension to be precise. My interpretation of the
Android spec for stylus is as follows: the orientation captures the direction of
the projection of the stylus on the 2D screen surface, and the tilt adds back
the z-dimension information using the acute angle the stylus makes with the
surface normal. These two angles are mutually orthogonal, and a "loss" in one
cannot be recovered from the other. The spec is a bit vague about tilt, but I
think this is the only unambiguous interpretation. Right?

[jdduke (slow), 2015-05-27 17:07:28]

On 2015/05/27 17:03:48, mustaq wrote:
> On 2015/05/27 15:52:03, jdduke wrote:
> > On 2015/05/27 14:32:54, Rick Byers wrote:
> > > This approach seems good to me, I like the explicitness for avoiding
> confusion
> > > here and I think it justifies the minor deviation from MotionEvent (if it
> > didn't
> > > have app compat impact, then I'd say the Android API should change too -
but
> I
> > > doubt it's worth the hassle given the old API must stay around).
> > > 
> > > +jdduke
> > 
> > Why can't we just relax the range restriction for GetOrientation()? The only
> > difference really between stylus/touch here is that stylus has an extra bit
> that
> > allows us to distinguish between "mirrored" orientations, i.e., we can
> > distinguish between the forward/backward directions of the pointer. Tilt is
a
> > separate property altogether. I have a slight preference for losing the bit
> when
> > converting to WebTouchEvent than losing it in the orientation getter.
> 
> Did you mean relaxing the range restriction for GetOrientation()
> unconditionally, or just for stylus? If you meant unconditionally, then we are
> going off the spec for touch which we should clearly avoid (in addition to
> developers' hassles resulting from possible non-canonical representation of a
> touch geometry). If you meant only for stylus, we need a way to distinguish
this
> case from the touch case. We can store an extra bit in ui::MotionEvent to mark
> the distingsion (e.g. 'isStylus') but separating the two "meanings" into
> separate fields is much cleaner IMO.
> 

The spec documentation looks incomplete, which is why we have this problem. The
result of MotionEventAndroid::GetOrientation() *is* effectively the Android
spec. 

> Most importantly, the difference between touch vs stylus geometry is more than
> just a bit, it's one extra dimension to be precise. My interpretation of the
> Android spec for stylus is as follows: the orientation captures the direction
of
> the projection of the stylus on the 2D screen surface, and the tilt adds back
> the z-dimension information using the acute angle the stylus makes with the
> surface normal. These two angles are mutually orthogonal, and a "loss" in one
> cannot be recovered from the other. The spec is a bit vague about tilt, but I
> think this is the only unambiguous interpretation. Right?

If you're thinking about pointer events, we don't currently expose the "tilt"
value from the MotionEvent, which would be required for you to reconstruct the
tilt values for pointer events. This issues is solely about orientation in the
screen's (touch) plane.

[jdduke (slow), 2015-05-27 18:04:03]

I dunno, I was OK with the previous patch, but this is also fine and I don't
feel strongly enough to put up a fight. Would "GetRestrictedOrientation" be any
better than "GetTouchOrientation", as it will be applicable/callable for
non-touch MotionEvents?

[tdresser, 2015-05-28 12:20:00]

On 2015/05/27 18:04:03, jdduke wrote:
> I dunno, I was OK with the previous patch, but this is also fine and I don't
> feel strongly enough to put up a fight. Would "GetRestrictedOrientation" be
any
> better than "GetTouchOrientation", as it will be applicable/callable for
> non-touch MotionEvents?

When would we want to get the restricted orientation for non-touch motion
events?
I was thinking GetTouchOrientation would only be callable for touch events (via
a DCHECK or similar).

It does feel a bit like we're letting the WebTouchEvent spec influence our
decision making in MotionEvent, which is somewhat backwards.

[mustaq, 2015-05-28 14:19:02]

On 2015/05/28 12:20:00, tdresser wrote:
> On 2015/05/27 18:04:03, jdduke wrote:
> > I dunno, I was OK with the previous patch, but this is also fine and I don't
> > feel strongly enough to put up a fight. Would "GetRestrictedOrientation" be
> any
> > better than "GetTouchOrientation", as it will be applicable/callable for
> > non-touch MotionEvents?
> 
> When would we want to get the restricted orientation for non-touch motion
> events?
> I was thinking GetTouchOrientation would only be callable for touch events
(via
> a DCHECK or similar).
> 
> It does feel a bit like we're letting the WebTouchEvent spec influence our
> decision making in MotionEvent, which is somewhat backwards.

Or we can simply use reasonable defaults for touch-specific methods in non-touch
MotionEvents: I think we are already doing this for getTouchMajor() in non-touch
MotionEvents (e.g. for mouse/joystick). From that perspective, I am a bit more
biased towards GetTouchOrientation/GetOrientationForTouch than
GetRestrictedOrientation because the 'restriction' is clear from the name.

We can also add runtime checks on top of it as Tim suggested, perhaps by
MotionEvent::ToolType?

My suggestion is more from a lower level (device level) perspective than from a
higher level one like WebTouchEvent---sorry for not making this clear---we need
to support 3D orientation geometry of stylus, and carrying on the Android API is
not the best solution IMO.

[jdduke (slow), 2015-05-28 15:03:18]

OK, we decided it might be best to go with your previous solution for now (sorry
for the noise!). We'll probably have to change it at some point when we
need/want to support 3D orientations (tilt), but for now let's just fix the
immediate issue. Thanks.