exo: Implement v6 of touch protocol including shape and frame event

exo: Implement v6 of touch protocol including shape and frame event

This is a prerequisite for passing along pen information via touch
events.
Ideally we want to batch multiple touch updates of a single hardware
event together with a single frame event. However this will need
an extension of ui::TouchEvents. We still provide a valid implementation
of wl_touch by sending a frame for every updated touch separately.

BUG=665499
TEST=Updated unit tests

Committed: https://crrev.com/f4361e3208bca8ab2a5a8a2cc1af1b04602fdf2e
Cr-Commit-Position: refs/heads/master@{#438908}

[denniskempin, 2016-12-06 22:41:55]

denniskempin@google.com changed reviewers:
+ reveman@chromium.org

[denniskempin, 2016-12-06 22:41:56]

PTAL. To implement the zcr_stylus_v2 protocol we need to properly implement the
frame event. And while we are at it, should implement the touch shape as well to
have a full implementation of the latest wl_touch protocol.

[denniskempin, 2016-12-07 02:17:16]

denniskempin@google.com changed reviewers:
+ sadrul@chromium.org

[denniskempin, 2016-12-07 02:17:17]

Hi Sadrul! 

Writing some exo unittests I have made some useful extension to EventGenerator
to help testing touch events with pointer details. 

Could you have a look or let me know who could review this? You seem to be the
only direct owner of ui/events.

[reveman, 2016-12-07 03:16:18]

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc...
components/exo/touch.cc:20: static bool AlmostEqual(T x, T y) {
This would need some explaining and maybe a rename to TouchAxisAlmostEqual. But
first, do we really need this? Sending a Touch::OnTouchShape event for every
touch event would be so much easier and it doesn't seem like that would add any
significant overhead. Am I missing something here?

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch_de...
File components/exo/touch_delegate.h (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch_de...
components/exo/touch_delegate.h:47: // Called when a touch point has changed
it's shape.
s/it's/its/

[sadrul, 2016-12-07 18:00:19]

lgtm

https://codereview.chromium.org/2560633002/diff/40001/ui/events/test/event_ge...
File ui/events/test/event_generator.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/ui/events/test/event_ge...
ui/events/test/event_generator.cc:270: pen_pointer_mode_ = true;
Should these be updated?

[denniskempin, 2016-12-07 18:32:18]

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc...
components/exo/touch.cc:20: static bool AlmostEqual(T x, T y) {
On 2016/12/07 03:16:18, reveman wrote:
> This would need some explaining and maybe a rename to TouchAxisAlmostEqual.
But
> first, do we really need this? Sending a Touch::OnTouchShape event for every
> touch event would be so much easier and it doesn't seem like that would add
any
> significant overhead. Am I missing something here?

We don't really have to do this, but the protocol asks for those events to be
only sent when the shape is changing, much like mouse events are only sent when
the location is changing. 

We will have more fields here where we will apply the same pattern. Tilt and
force as well. So at that time we would be at up to 4 events (shape, tilt,
force, motion) for every update, so the overhead will be increasing. 

Either way, I think it would be fully functional with little overhead if we just
send those events for every update. I am just following the practice we were
using in the Pointer class. 

AlmostEqual is really just a generic function to compare to floating point
values for equality since you cannot compare them directly. So maybe
FloatAlmostEqual would be more fitting?

https://codereview.chromium.org/2560633002/diff/40001/ui/events/test/event_ge...
File ui/events/test/event_generator.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/ui/events/test/event_ge...
ui/events/test/event_generator.cc:270: pen_pointer_mode_ = true;
On 2016/12/07 18:00:19, sadrul wrote:
> Should these be updated?

These are used on mouse pen events only, so they are separate from the
touch_pointer_details.

[reveman, 2016-12-07 22:58:00]

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc...
components/exo/touch.cc:20: static bool AlmostEqual(T x, T y) {
On 2016/12/07 at 18:32:18, denniskempin wrote:
> On 2016/12/07 03:16:18, reveman wrote:
> > This would need some explaining and maybe a rename to TouchAxisAlmostEqual.
But
> > first, do we really need this? Sending a Touch::OnTouchShape event for every
> > touch event would be so much easier and it doesn't seem like that would add
any
> > significant overhead. Am I missing something here?
> 
> We don't really have to do this, but the protocol asks for those events to be
only sent when the shape is changing, much like mouse events are only sent when
the location is changing.

I don't think we'll break any clients by sending an event even if the shape
didn't change. If we really care to avoid this then I think we should add a new
touch shape event to chrome and track this at the evdev level instead of here. 
 
> 
> We will have more fields here where we will apply the same pattern. Tilt and
force as well. So at that time we would be at up to 4 events (shape, tilt,
force, motion) for every update, so the overhead will be increasing.

I suspect that this is still not going to add any significant overhead. We'll
send a few more bytes across the channel but we're not talking much data here.
Again, if we want to reduce processing overhead in regards to this then we
should probably do that at a lower level.
 
> 
> Either way, I think it would be fully functional with little overhead if we
just send those events for every update. I am just following the practice we
were using in the Pointer class.

Keeping things simple for now is best I think and we can always micro optimize
this later if it turns out to be a problem.

> 
> AlmostEqual is really just a generic function to compare to floating point
values for equality since you cannot compare them directly. So maybe
FloatAlmostEqual would be more fitting?

Why not simply "abs(a - b) < epsilon" ?

[denniskempin, 2016-12-07 23:40:54]

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch.cc...
components/exo/touch.cc:20: static bool AlmostEqual(T x, T y) {
On 2016/12/07 22:58:00, reveman wrote:
> On 2016/12/07 at 18:32:18, denniskempin wrote:
> > On 2016/12/07 03:16:18, reveman wrote:
> > > This would need some explaining and maybe a rename to
TouchAxisAlmostEqual.
> But
> > > first, do we really need this? Sending a Touch::OnTouchShape event for
every
> > > touch event would be so much easier and it doesn't seem like that would
add
> any
> > > significant overhead. Am I missing something here?
> > 
> > We don't really have to do this, but the protocol asks for those events to
be
> only sent when the shape is changing, much like mouse events are only sent
when
> the location is changing.
> 
> I don't think we'll break any clients by sending an event even if the shape
> didn't change. If we really care to avoid this then I think we should add a
new
> touch shape event to chrome and track this at the evdev level instead of here.

>  
> > 
> > We will have more fields here where we will apply the same pattern. Tilt and
> force as well. So at that time we would be at up to 4 events (shape, tilt,
> force, motion) for every update, so the overhead will be increasing.
> 
> I suspect that this is still not going to add any significant overhead. We'll
> send a few more bytes across the channel but we're not talking much data here.
> Again, if we want to reduce processing overhead in regards to this then we
> should probably do that at a lower level.
>  
> > 
> > Either way, I think it would be fully functional with little overhead if we
> just send those events for every update. I am just following the practice we
> were using in the Pointer class.
> 
> Keeping things simple for now is best I think and we can always micro optimize
> this later if it turns out to be a problem.
> 
> > 
> > AlmostEqual is really just a generic function to compare to floating point
> values for equality since you cannot compare them directly. So maybe
> FloatAlmostEqual would be more fitting?
> 
> Why not simply "abs(a - b) < epsilon" ?

sgtm. Let's go with always sending the update. It's significantly simpler.

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch_de...
File components/exo/touch_delegate.h (right):

https://codereview.chromium.org/2560633002/diff/40001/components/exo/touch_de...
components/exo/touch_delegate.h:47: // Called when a touch point has changed
it's shape.
On 2016/12/07 03:16:18, reveman wrote:
> s/it's/its/
I keep using the ESL excuse, but after 4 years in the US that doesn't cut it
anymore ;)

[reveman, 2016-12-08 01:40:45]

lgtm % nits

https://codereview.chromium.org/2560633002/diff/60001/components/exo/touch.cc
File components/exo/touch.cc (right):

https://codereview.chromium.org/2560633002/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:125: delegate_->OnTouchShape(event->touch_id(),
Can you move this up into the switch cases above instead so we only send it if
the touch id is valid? That way we also avoid branching twice.

https://codereview.chromium.org/2560633002/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:129: // todo(denniskempin): Extend ui::TouchEvent to
signal end of sequence of
s/todo/TODO/

https://codereview.chromium.org/2560633002/diff/60001/components/exo/touch.cc...
components/exo/touch.cc:131: delegate_->OnTouchFrame();
Move this up into the switch cases as well to avoid sending if touch_id is not
valid.

[denniskempin, 2016-12-08 19:37:13]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2016-12-08 19:37:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/2560633002/#ps80001
(title: "fixed nits")

[commit-bot: I haz the power, 2016-12-08 19:37:50]

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

[commit-bot: I haz the power, 2016-12-08 19:37:51]

This CL has an open dependency (Issue 2548603002 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[denniskempin, 2016-12-08 22:52:32]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-08 22:53:15]

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-12-08 23:08:45]

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

[commit-bot: I haz the power, 2016-12-08 23:08:46]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2016-12-08 23:14:33]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-08 23:14:57]

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-12-08 23:23:12]

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

[commit-bot: I haz the power, 2016-12-08 23:23:13]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[denniskempin, 2016-12-08 23:45:15]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-08 23:45:37]

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-12-08 23:59:14]

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

[commit-bot: I haz the power, 2016-12-08 23:59:15]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[denniskempin, 2016-12-09 00:06:56]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-09 00:07:29]

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-12-09 00:16:20]

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

[commit-bot: I haz the power, 2016-12-09 00:16:21]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[denniskempin, 2016-12-09 00:29:58]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-09 00:30:22]

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-12-09 01:39:18]

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

[commit-bot: I haz the power, 2016-12-09 01:39:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2016-12-09 18:24:57]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-09 18:25:32]

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-12-09 20:26:49]

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

[commit-bot: I haz the power, 2016-12-09 20:26:50]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2016-12-09 21:11:14]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-09 21:11:51]

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-12-09 22:56:53]

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

[commit-bot: I haz the power, 2016-12-09 22:56:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2016-12-14 22:28:59]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2016-12-14 22:29:23]

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-12-14 23:52:42]

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

[commit-bot: I haz the power, 2016-12-14 23:52:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2016-12-15 19:11:51]

Jacob, FYI I changed event generator to allow for more generic pointer details.

[denniskempin, 2016-12-15 19:11:59]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2016-12-15 19:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2560633002/#ps100001
(title: "Fixed pen pointer mode conflict after rebase")

[commit-bot: I haz the power, 2016-12-15 19:12:30]

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-12-15 20:16:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481829119368090,
"parent_rev": "4ba958d22e008ef8298a8c90210bb81167d399b2", "commit_rev":
"d845e80d6d9617825b43af8425ac9c9a6c20e5d8"}

[commit-bot: I haz the power, 2016-12-15 20:17:00]

Description was changed from

==========
exo: Implement v6 of touch protocol including shape and frame event

This is a prerequisite for passing along pen information via touch
events.
Ideally we want to batch multiple touch updates of a single hardware
event together with a single frame event. However this will need
an extension of ui::TouchEvents. We still provide a valid implementation
of wl_touch by sending a frame for every updated touch separately.

BUG=665499
TEST=Updated unit tests
==========

to

==========
exo: Implement v6 of touch protocol including shape and frame event

This is a prerequisite for passing along pen information via touch
events.
Ideally we want to batch multiple touch updates of a single hardware
event together with a single frame event. However this will need
an extension of ui::TouchEvents. We still provide a valid implementation
of wl_touch by sending a frame for every updated touch separately.

BUG=665499
TEST=Updated unit tests

Review-Url: https://codereview.chromium.org/2560633002
==========

[commit-bot: I haz the power, 2016-12-15 20:17:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-15 20:20:56]

Description was changed from

==========
exo: Implement v6 of touch protocol including shape and frame event

This is a prerequisite for passing along pen information via touch
events.
Ideally we want to batch multiple touch updates of a single hardware
event together with a single frame event. However this will need
an extension of ui::TouchEvents. We still provide a valid implementation
of wl_touch by sending a frame for every updated touch separately.

BUG=665499
TEST=Updated unit tests

Review-Url: https://codereview.chromium.org/2560633002
==========

to

==========
exo: Implement v6 of touch protocol including shape and frame event

This is a prerequisite for passing along pen information via touch
events.
Ideally we want to batch multiple touch updates of a single hardware
event together with a single frame event. However this will need
an extension of ui::TouchEvents. We still provide a valid implementation
of wl_touch by sending a frame for every updated touch separately.

BUG=665499
TEST=Updated unit tests

Committed: https://crrev.com/f4361e3208bca8ab2a5a8a2cc1af1b04602fdf2e
Cr-Commit-Position: refs/heads/master@{#438908}
==========

[commit-bot: I haz the power, 2016-12-15 20:20:57]

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