Setting valid force value for touch events on Ozone on X.

Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE

Committed: https://crrev.com/85fa838eb0ac79e83e4e3eaa7c1396129e6b4565
Cr-Commit-Position: refs/heads/master@{#403210}

[mfomitchev, 2016-06-28 15:15:09]

mfomitchev@chromium.org changed reviewers:
+ kylechar@chromium.org, nzolghadr@chromium.org, sadrul@chromium.org

[mfomitchev, 2016-06-28 15:15:10]

Sadrul, Kyle, Navid - WDYT?
I could instead(also?) set the force in TranslateXI2EventToEvent (via
GetTouchForceFromXEvent), but it seems we should have a valid force value set in
PointerDetails for touch events anyhow, since this is what Blink expects?

[kylechar, 2016-06-28 15:37:31]

On 2016/06/28 15:15:10, mfomitchev wrote:
> Sadrul, Kyle, Navid - WDYT?
> I could instead(also?) set the force in TranslateXI2EventToEvent (via
> GetTouchForceFromXEvent), but it seems we should have a valid force value set
in
> PointerDetails for touch events anyhow, since this is what Blink expects?

Setting the force in TranslateXI2EventToEvent via GetTouchForceFromXEvent sounds
like a good idea (possibly in addition to this change?).

I don't have much context on if the change to PointerDetails is appropriate. My
totally unqualified opinion is if NaN is a valid touch force value then blink
should accept it?

[mfomitchev, 2016-06-28 16:14:29]

On 2016/06/28 15:37:31, kylechar wrote:
> On 2016/06/28 15:15:10, mfomitchev wrote:
> > Sadrul, Kyle, Navid - WDYT?
> > I could instead(also?) set the force in TranslateXI2EventToEvent (via
> > GetTouchForceFromXEvent), but it seems we should have a valid force value
set
> in
> > PointerDetails for touch events anyhow, since this is what Blink expects?
> 
> Setting the force in TranslateXI2EventToEvent via GetTouchForceFromXEvent
sounds
> like a good idea (possibly in addition to this change?).
> 
> I don't have much context on if the change to PointerDetails is appropriate.
My
> totally unqualified opinion is if NaN is a valid touch force value then blink
> should accept it?

Ok, I updated TranslateXI2EventToEvent - I think it would be good to have valid
force values on the actual device. We can get rid of the change in event.h now,
although it still seems to me like it makes sense. Thoughts?

[sadrul, 2016-06-30 14:08:51]

On 2016/06/28 16:14:29, mfomitchev wrote:
> On 2016/06/28 15:37:31, kylechar wrote:
> > On 2016/06/28 15:15:10, mfomitchev wrote:
> > > Sadrul, Kyle, Navid - WDYT?
> > > I could instead(also?) set the force in TranslateXI2EventToEvent (via
> > > GetTouchForceFromXEvent), but it seems we should have a valid force value
> set
> > in
> > > PointerDetails for touch events anyhow, since this is what Blink expects?
> > 
> > Setting the force in TranslateXI2EventToEvent via GetTouchForceFromXEvent
> sounds
> > like a good idea (possibly in addition to this change?).
> > 
> > I don't have much context on if the change to PointerDetails is appropriate.
> My
> > totally unqualified opinion is if NaN is a valid touch force value then
blink
> > should accept it?
> 
> Ok, I updated TranslateXI2EventToEvent - I think it would be good to have
valid
> force values on the actual device. We can get rid of the change in event.h
now,
> although it still seems to me like it makes sense. Thoughts?

Hrm. Going back to https://codereview.chromium.org/1617863002, where we changed
the default value from 0 to nan, it looks like the value of nan is expected to
imply that the particular device does not support pressure. And blink has code
in place to set the pressure as per the web-spec when it receives nan as the
pressure
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...).
Is there a reason that code isn't kicking in?

[Navid Zolghadr, 2016-06-30 14:38:39]

On 2016/06/30 14:08:51, sadrul wrote:
> On 2016/06/28 16:14:29, mfomitchev wrote:
> > On 2016/06/28 15:37:31, kylechar wrote:
> > > On 2016/06/28 15:15:10, mfomitchev wrote:
> > > > Sadrul, Kyle, Navid - WDYT?
> > > > I could instead(also?) set the force in TranslateXI2EventToEvent (via
> > > > GetTouchForceFromXEvent), but it seems we should have a valid force
value
> > set
> > > in
> > > > PointerDetails for touch events anyhow, since this is what Blink
expects?
> > > 
> > > Setting the force in TranslateXI2EventToEvent via GetTouchForceFromXEvent
> > sounds
> > > like a good idea (possibly in addition to this change?).
> > > 
> > > I don't have much context on if the change to PointerDetails is
appropriate.
> > My
> > > totally unqualified opinion is if NaN is a valid touch force value then
> blink
> > > should accept it?
> > 
> > Ok, I updated TranslateXI2EventToEvent - I think it would be good to have
> valid
> > force values on the actual device. We can get rid of the change in event.h
> now,
> > although it still seems to me like it makes sense. Thoughts?
> 
> Hrm. Going back to https://codereview.chromium.org/1617863002, where we
changed
> the default value from 0 to nan, it looks like the value of nan is expected to
> imply that the particular device does not support pressure. And blink has code
> in place to set the pressure as per the web-spec when it receives nan as the
> pressure
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...).
> Is there a reason that code isn't kicking in?

Yup. As far as I remember there was no scenario that we were not setting the
pressure for touches. It seems that here is a scenario though. We are changing
the value when we see a NAN only when we are creating the pointerevents and not
the touch events themselves here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...

I believe sticking to what we agreed on back then makes more sense. We should be
setting NAN for the cases that the device doesn't report the pressure so that
Blink can implement the spec. In the case of pointerevent spec it is mentioned
it should be 0.5 when no button is pressed and 1 if there was any button
pressed. This could possibly be different in the other specs.

Going back to this crash case, can you provide a deeper stack trace? I mean I
wonder what the full path is that causes the ASSERT to hit.

[mfomitchev, 2016-06-30 14:57:10]

Description was changed from

==========
Setting valid force value in PointerDetails for touch events.

When --touch-devices flag is used in Ozone builds, chrome crashes whenever
the mouse set as a touch device is used.
This is because of the ASSERT in blink::PlatformTouchPoint::force() in
combination with how TouchEvents are consructed in Ozone on X:

new TouchEvent(type, location, touch_id, time_stamp)
(called from TranslateXI2EventToEvent in x11_event_source_libevent.cc).

That TouchEvent constructor uses
PointerDetails(EventPointerType::POINTER_TYPE_TOUCH)
to initialize PointerDetails, which (before the CL) sets force to NaN, which
causes problems down the line.

Note that the analogous flow on X uses TouchEvent(native_event) constructor,
which uses ui::GetTouchPointerDetailsFromNative, which initializes force to 0 if
it is not already set in the NativeEvent.

BUG=NONE
==========

to

==========
Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses
GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE
==========

[mfomitchev, 2016-06-30 14:59:38]

On 2016/06/30 14:38:39, Navid Zolghadr wrote:
> On 2016/06/30 14:08:51, sadrul wrote:
> > On 2016/06/28 16:14:29, mfomitchev wrote:
> > > On 2016/06/28 15:37:31, kylechar wrote:
> > > > On 2016/06/28 15:15:10, mfomitchev wrote:
> > > > > Sadrul, Kyle, Navid - WDYT?
> > > > > I could instead(also?) set the force in TranslateXI2EventToEvent (via
> > > > > GetTouchForceFromXEvent), but it seems we should have a valid force
> value
> > > set
> > > > in
> > > > > PointerDetails for touch events anyhow, since this is what Blink
> expects?
> > > > 
> > > > Setting the force in TranslateXI2EventToEvent via
GetTouchForceFromXEvent
> > > sounds
> > > > like a good idea (possibly in addition to this change?).
> > > > 
> > > > I don't have much context on if the change to PointerDetails is
> appropriate.
> > > My
> > > > totally unqualified opinion is if NaN is a valid touch force value then
> > blink
> > > > should accept it?
> > > 
> > > Ok, I updated TranslateXI2EventToEvent - I think it would be good to have
> > valid
> > > force values on the actual device. We can get rid of the change in event.h
> > now,
> > > although it still seems to me like it makes sense. Thoughts?
> > 
> > Hrm. Going back to https://codereview.chromium.org/1617863002, where we
> changed
> > the default value from 0 to nan, it looks like the value of nan is expected
to
> > imply that the particular device does not support pressure. And blink has
code
> > in place to set the pressure as per the web-spec when it receives nan as the
> > pressure
> >
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...).
> > Is there a reason that code isn't kicking in?
> 
> Yup. As far as I remember there was no scenario that we were not setting the
> pressure for touches. It seems that here is a scenario though. We are changing
> the value when we see a NAN only when we are creating the pointerevents and
not
> the touch events themselves here:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...
> 
> I believe sticking to what we agreed on back then makes more sense. We should
be
> setting NAN for the cases that the device doesn't report the pressure so that
> Blink can implement the spec. In the case of pointerevent spec it is mentioned
> it should be 0.5 when no button is pressed and 1 if there was any button
> pressed. This could possibly be different in the other specs.
> 
> Going back to this crash case, can you provide a deeper stack trace? I mean I
> wonder what the full path is that causes the ASSERT to hit.

Ok, I have logged crbug.com/624804 to address the issue on the Blink sides, and
I've added the relevant details there.

This CL now just makes Ozone consistent with X. Note that this makes it so the
ASSERT no longer triggers on Ozone in chrome, but we should still fix
crbug.com/624804.

[Navid Zolghadr, 2016-06-30 17:32:24]

lgtm. I'll look into the bug you created for Blink.

[sadrul, 2016-06-30 17:33:13]

lgtm

[mfomitchev, 2016-06-30 17:34:30]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2016-06-30 17:35:03]

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-06-30 17:41:12]

Description was changed from

==========
Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses
GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE
==========

to

==========
Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses
GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE
==========

[commit-bot: I haz the power, 2016-06-30 17:41:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-30 17:41:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 17:45:24]

Description was changed from

==========
Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses
GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE
==========

to

==========
Setting valid force value for touch events on Ozone on X.

When --touch-devices flag is used on X, we use TouchEvent(native_event)
constructor, which uses ui::GetTouchForceFromXEvent() to initialize the force
value. On Ozone we don't do this, and force gets initialized to NaN.

This CL makes Ozone on X consistent with X in that it uses
GetTouchForceFromXEvent(xev)
to initialize the force value in the constructed TouchEvent.

BUG=NONE

Committed: https://crrev.com/85fa838eb0ac79e83e4e3eaa7c1396129e6b4565
Cr-Commit-Position: refs/heads/master@{#403210}
==========

[commit-bot: I haz the power, 2016-06-30 17:45:25]

Patchset 3 (id:??) landed as
https://crrev.com/85fa838eb0ac79e83e4e3eaa7c1396129e6b4565
Cr-Commit-Position: refs/heads/master@{#403210}