ozone: evdev: Add libgestures bindings for touchpad & mouse support

ozone: evdev: Add libgestures bindings for touchpad & mouse support

The gestures library converts kernel input events into gestures. This
currently happens inside X11 in the xf86-input-cmt driver, but without
X11 we have to do use it from EventFactoryEvdev

BUG=351168
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264273

[rjkroege, 2014-03-11 12:58:47]

this (and your other CLs) probably needs some tests?

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/event_...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/event_...
ui/events/ozone/evdev/event_factory_evdev.cc:23: #if defined(USE_EVDEV_GESTURES)
do we ever not want to use evdev_gestures?

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
File ui/events/ozone/evdev/gestures/event_reader_libevdev_cros.h (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
ui/events/ozone/evdev/gestures/event_reader_libevdev_cros.h:15: 
add a comment to say what this is for?

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
File ui/events/ozone/evdev/gestures/gesture_interpreter_libevdev_cros.h (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
ui/events/ozone/evdev/gestures/gesture_interpreter_libevdev_cros.h:26: class
EVENTS_EXPORT GestureInterpreterLibevdevCros
class comment?

[sadrul, 2014-03-11 16:13:09]

Can we call this touchpad_gestures/ to avoid confusion with ui/events/gestures/?

[spang, 2014-03-11 16:31:30]

On 2014/03/11 16:13:09, sadrul wrote:
> Can we call this touchpad_gestures/ to avoid confusion with
ui/events/gestures/?

It will also be used for mice.

The fact that we have two gestureses is indeed confusing. Maybe they could
eventually be merged?

[spang, 2014-03-11 16:38:11]

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/event_...
File ui/events/ozone/evdev/event_factory_evdev.cc (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/event_...
ui/events/ozone/evdev/event_factory_evdev.cc:23: #if defined(USE_EVDEV_GESTURES)
On 2014/03/11 12:58:47, rjkroege wrote:
> do we ever not want to use evdev_gestures?

It is only present on chromeos.

My suspicion is that we should choose our battles with respect to moving code
around. Let's use the one from the system, and keep an ifdef. Only bona fide
ChromeOS builds have any use for this code.

If we want it everywhere, it obviously must go into third_party, but I am not
sure doing that is really worth the effort at this time.

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
File ui/events/ozone/evdev/gestures/event_reader_libevdev_cros.h (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
ui/events/ozone/evdev/gestures/event_reader_libevdev_cros.h:15: 
On 2014/03/11 12:58:47, rjkroege wrote:
> add a comment to say what this is for?

Done.

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
File ui/events/ozone/evdev/gestures/gesture_interpreter_libevdev_cros.h (right):

https://codereview.chromium.org/193813003/diff/1/ui/events/ozone/evdev/gestur...
ui/events/ozone/evdev/gestures/gesture_interpreter_libevdev_cros.h:26: class
EVENTS_EXPORT GestureInterpreterLibevdevCros
On 2014/03/11 12:58:47, rjkroege wrote:
> class comment?

Done.

[spang, 2014-03-11 16:42:48]

On 2014/03/11 16:31:30, spang wrote:
> On 2014/03/11 16:13:09, sadrul wrote:
> > Can we call this touchpad_gestures/ to avoid confusion with
> ui/events/gestures/?
> 
> It will also be used for mice.
> 
> The fact that we have two gestureses is indeed confusing. Maybe they could
> eventually be merged?

Calling it "cmt" might make sense, as it fills the same role of xf86-input-cmt.

Or maybe it's confusing to use the same name as the x11 driver.

The gestures processing itself is not really even here. libgestures_glue?

[sadrul, 2014-03-11 16:48:11]

> libgestures_glue

sg

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1', {
Do we really need to support this level of granularity for every feature? Can we
just require that if you are using evdev, you should always use libgestures as
well?

[spang, 2014-03-11 16:58:49]

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1', {
On 2014/03/11 16:48:12, sadrul wrote:
> Do we really need to support this level of granularity for every feature? Can
we
> just require that if you are using evdev, you should always use libgestures as
> well?

It's this way because the library does not exist on desktop, and so can't be in
in build-deps, and because it is not checked into third_party.

There's precedent for this. In particular CRAS works this way, because libcras
is a similar library that only exists in ChromeOS.

[sadrul, 2014-03-11 18:13:40]

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1', {
On 2014/03/11 16:58:50, spang wrote:
> On 2014/03/11 16:48:12, sadrul wrote:
> > Do we really need to support this level of granularity for every feature?
Can
> we
> > just require that if you are using evdev, you should always use libgestures
as
> > well?
> 
> It's this way because the library does not exist on desktop, and so can't be
in
> in build-deps, and because it is not checked into third_party.
> 
> There's precedent for this. In particular CRAS works this way, because libcras
> is a similar library that only exists in ChromeOS.

Fair enough. I suspect we would want some sort of test-coverage for this code at
some point though, and so we would need to pull this into third_party at that
time. We should make sure that this doesn't get lost (file a bug?)

[spang, 2014-03-11 19:24:21]

On 2014/03/11 18:13:40, sadrul wrote:
> https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> File ui/events/events.gyp (right):
> 
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1', {
> On 2014/03/11 16:58:50, spang wrote:
> > On 2014/03/11 16:48:12, sadrul wrote:
> > > Do we really need to support this level of granularity for every feature?
> Can
> > we
> > > just require that if you are using evdev, you should always use
libgestures
> as
> > > well?
> > 
> > It's this way because the library does not exist on desktop, and so can't be
> in
> > in build-deps, and because it is not checked into third_party.
> > 
> > There's precedent for this. In particular CRAS works this way, because
libcras
> > is a similar library that only exists in ChromeOS.
> 
> Fair enough. I suspect we would want some sort of test-coverage for this code
at
> some point though, and so we would need to pull this into third_party at that
> time. We should make sure that this doesn't get lost (file a bug?)

This is a good point. I was going to add some tests, but no builder will run
them. I am kind of surprised we don't have a builder that runs tests in a
ChromeOS chroot build.

While I'm not super worried about test coverage of this glue code (the
interesting bits are in the library), tests that don't run aren't too helpful.

I'm worried about pushback though with adding lots of stuff to the linux
checkout, though.

[spang, 2014-03-12 21:21:10]

On 2014/03/11 19:24:21, spang wrote:
> On 2014/03/11 18:13:40, sadrul wrote:
> > https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> > File ui/events/events.gyp (right):
> > 
> >
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> > ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1', {
> > On 2014/03/11 16:58:50, spang wrote:
> > > On 2014/03/11 16:48:12, sadrul wrote:
> > > > Do we really need to support this level of granularity for every
feature?
> > Can
> > > we
> > > > just require that if you are using evdev, you should always use
> libgestures
> > as
> > > > well?
> > > 
> > > It's this way because the library does not exist on desktop, and so can't
be
> > in
> > > in build-deps, and because it is not checked into third_party.
> > > 
> > > There's precedent for this. In particular CRAS works this way, because
> libcras
> > > is a similar library that only exists in ChromeOS.
> > 
> > Fair enough. I suspect we would want some sort of test-coverage for this
code
> at
> > some point though, and so we would need to pull this into third_party at
that
> > time. We should make sure that this doesn't get lost (file a bug?)
> 
> This is a good point. I was going to add some tests, but no builder will run
> them. I am kind of surprised we don't have a builder that runs tests in a
> ChromeOS chroot build.
> 
> While I'm not super worried about test coverage of this glue code (the
> interesting bits are in the library), tests that don't run aren't too helpful.
> 
> I'm worried about pushback though with adding lots of stuff to the linux
> checkout, though.

I've made a terrifying discovery that may force us to check it into third_party
after all.

gestures uses libbase from ChromeOS, which is a fork of chromium's base, and
cannot be safely loaded into chrome.

[rjkroege, 2014-03-13 21:17:26]

On 2014/03/12 21:21:10, spang wrote:
> On 2014/03/11 19:24:21, spang wrote:
> > On 2014/03/11 18:13:40, sadrul wrote:
> > > https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> > > File ui/events/events.gyp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> > > ui/events/events.gyp:170: ['use_ozone_evdev==1 and use_evdev_gestures==1',
{
> > > On 2014/03/11 16:58:50, spang wrote:
> > > > On 2014/03/11 16:48:12, sadrul wrote:
> > > > > Do we really need to support this level of granularity for every
> feature?
> > > Can
> > > > we
> > > > > just require that if you are using evdev, you should always use
> > libgestures
> > > as
> > > > > well?
> > > > 
> > > > It's this way because the library does not exist on desktop, and so
can't
> be
> > > in
> > > > in build-deps, and because it is not checked into third_party.
> > > > 
> > > > There's precedent for this. In particular CRAS works this way, because
> > libcras
> > > > is a similar library that only exists in ChromeOS.
> > > 
> > > Fair enough. I suspect we would want some sort of test-coverage for this
> code
> > at
> > > some point though, and so we would need to pull this into third_party at
> that
> > > time. We should make sure that this doesn't get lost (file a bug?)
> > 
> > This is a good point. I was going to add some tests, but no builder will run
> > them. I am kind of surprised we don't have a builder that runs tests in a
> > ChromeOS chroot build.
> > 
> > While I'm not super worried about test coverage of this glue code (the
> > interesting bits are in the library), tests that don't run aren't too
helpful.
> > 
> > I'm worried about pushback though with adding lots of stuff to the linux
> > checkout, though.
> 
> I've made a terrifying discovery that may force us to check it into
third_party
> after all.
> 
> gestures uses libbase from ChromeOS, which is a fork of chromium's base, and
> cannot be safely loaded into chrome.

gack. So: gestures for ozone needs to link against base in Chrome yes?

[spang, 2014-03-14 20:08:44]

On 2014/03/13 21:17:26, rjkroege wrote:
> On 2014/03/12 21:21:10, spang wrote:
> > On 2014/03/11 19:24:21, spang wrote:
> > > On 2014/03/11 18:13:40, sadrul wrote:
> > > >
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> > > > File ui/events/events.gyp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> > > > ui/events/events.gyp:170: ['use_ozone_evdev==1 and
use_evdev_gestures==1',
> {
> > > > On 2014/03/11 16:58:50, spang wrote:
> > > > > On 2014/03/11 16:48:12, sadrul wrote:
> > > > > > Do we really need to support this level of granularity for every
> > feature?
> > > > Can
> > > > > we
> > > > > > just require that if you are using evdev, you should always use
> > > libgestures
> > > > as
> > > > > > well?
> > > > > 
> > > > > It's this way because the library does not exist on desktop, and so
> can't
> > be
> > > > in
> > > > > in build-deps, and because it is not checked into third_party.
> > > > > 
> > > > > There's precedent for this. In particular CRAS works this way, because
> > > libcras
> > > > > is a similar library that only exists in ChromeOS.
> > > > 
> > > > Fair enough. I suspect we would want some sort of test-coverage for this
> > code
> > > at
> > > > some point though, and so we would need to pull this into third_party at
> > that
> > > > time. We should make sure that this doesn't get lost (file a bug?)
> > > 
> > > This is a good point. I was going to add some tests, but no builder will
run
> > > them. I am kind of surprised we don't have a builder that runs tests in a
> > > ChromeOS chroot build.
> > > 
> > > While I'm not super worried about test coverage of this glue code (the
> > > interesting bits are in the library), tests that don't run aren't too
> helpful.
> > > 
> > > I'm worried about pushback though with adding lots of stuff to the linux
> > > checkout, though.
> > 
> > I've made a terrifying discovery that may force us to check it into
> third_party
> > after all.
> > 
> > gestures uses libbase from ChromeOS, which is a fork of chromium's base, and
> > cannot be safely loaded into chrome.
> 
> gack. So: gestures for ozone needs to link against base in Chrome yes?

Probably. However, that does not compile currently, because the ChromeOS libbase
is old and things have been rearranged since it was last gardened.

I am not convinced it is OK to have a gardened library use base. Doing this
opens us up to breakage in response to rearrangements or changes to base.

We might want to check all of the gestures code into ToT. This would allow
updates to base and gestures to happen simultaneously.

[spang, 2014-03-14 20:21:09]

On 2014/03/14 20:08:44, spang wrote:
> On 2014/03/13 21:17:26, rjkroege wrote:
> > On 2014/03/12 21:21:10, spang wrote:
> > > On 2014/03/11 19:24:21, spang wrote:
> > > > On 2014/03/11 18:13:40, sadrul wrote:
> > > > >
> https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> > > > > File ui/events/events.gyp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> > > > > ui/events/events.gyp:170: ['use_ozone_evdev==1 and
> use_evdev_gestures==1',
> > {
> > > > > On 2014/03/11 16:58:50, spang wrote:
> > > > > > On 2014/03/11 16:48:12, sadrul wrote:
> > > > > > > Do we really need to support this level of granularity for every
> > > feature?
> > > > > Can
> > > > > > we
> > > > > > > just require that if you are using evdev, you should always use
> > > > libgestures
> > > > > as
> > > > > > > well?
> > > > > > 
> > > > > > It's this way because the library does not exist on desktop, and so
> > can't
> > > be
> > > > > in
> > > > > > in build-deps, and because it is not checked into third_party.
> > > > > > 
> > > > > > There's precedent for this. In particular CRAS works this way,
because
> > > > libcras
> > > > > > is a similar library that only exists in ChromeOS.
> > > > > 
> > > > > Fair enough. I suspect we would want some sort of test-coverage for
this
> > > code
> > > > at
> > > > > some point though, and so we would need to pull this into third_party
at
> > > that
> > > > > time. We should make sure that this doesn't get lost (file a bug?)
> > > > 
> > > > This is a good point. I was going to add some tests, but no builder will
> run
> > > > them. I am kind of surprised we don't have a builder that runs tests in
a
> > > > ChromeOS chroot build.
> > > > 
> > > > While I'm not super worried about test coverage of this glue code (the
> > > > interesting bits are in the library), tests that don't run aren't too
> > helpful.
> > > > 
> > > > I'm worried about pushback though with adding lots of stuff to the linux
> > > > checkout, though.
> > > 
> > > I've made a terrifying discovery that may force us to check it into
> > third_party
> > > after all.
> > > 
> > > gestures uses libbase from ChromeOS, which is a fork of chromium's base,
and
> > > cannot be safely loaded into chrome.
> > 
> > gack. So: gestures for ozone needs to link against base in Chrome yes?
> 
> Probably. However, that does not compile currently, because the ChromeOS
libbase
> is old and things have been rearranged since it was last gardened.
> 
> I am not convinced it is OK to have a gardened library use base. Doing this
> opens us up to breakage in response to rearrangements or changes to base.
> 
> We might want to check all of the gestures code into ToT. This would allow
> updates to base and gestures to happen simultaneously.

gestures also uses C++11

[spang, 2014-04-15 23:39:39]

On 2014/03/14 20:21:09, spang wrote:
> On 2014/03/14 20:08:44, spang wrote:
> > On 2014/03/13 21:17:26, rjkroege wrote:
> > > On 2014/03/12 21:21:10, spang wrote:
> > > > On 2014/03/11 19:24:21, spang wrote:
> > > > > On 2014/03/11 18:13:40, sadrul wrote:
> > > > > >
> > https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp
> > > > > > File ui/events/events.gyp (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/193813003/diff/20001/ui/events/events.gyp#new...
> > > > > > ui/events/events.gyp:170: ['use_ozone_evdev==1 and
> > use_evdev_gestures==1',
> > > {
> > > > > > On 2014/03/11 16:58:50, spang wrote:
> > > > > > > On 2014/03/11 16:48:12, sadrul wrote:
> > > > > > > > Do we really need to support this level of granularity for every
> > > > feature?
> > > > > > Can
> > > > > > > we
> > > > > > > > just require that if you are using evdev, you should always use
> > > > > libgestures
> > > > > > as
> > > > > > > > well?
> > > > > > > 
> > > > > > > It's this way because the library does not exist on desktop, and
so
> > > can't
> > > > be
> > > > > > in
> > > > > > > in build-deps, and because it is not checked into third_party.
> > > > > > > 
> > > > > > > There's precedent for this. In particular CRAS works this way,
> because
> > > > > libcras
> > > > > > > is a similar library that only exists in ChromeOS.
> > > > > > 
> > > > > > Fair enough. I suspect we would want some sort of test-coverage for
> this
> > > > code
> > > > > at
> > > > > > some point though, and so we would need to pull this into
third_party
> at
> > > > that
> > > > > > time. We should make sure that this doesn't get lost (file a bug?)
> > > > > 
> > > > > This is a good point. I was going to add some tests, but no builder
will
> > run
> > > > > them. I am kind of surprised we don't have a builder that runs tests
in
> a
> > > > > ChromeOS chroot build.
> > > > > 
> > > > > While I'm not super worried about test coverage of this glue code (the
> > > > > interesting bits are in the library), tests that don't run aren't too
> > > helpful.
> > > > > 
> > > > > I'm worried about pushback though with adding lots of stuff to the
linux
> > > > > checkout, though.
> > > > 
> > > > I've made a terrifying discovery that may force us to check it into
> > > third_party
> > > > after all.
> > > > 
> > > > gestures uses libbase from ChromeOS, which is a fork of chromium's base,
> and
> > > > cannot be safely loaded into chrome.
> > > 
> > > gack. So: gestures for ozone needs to link against base in Chrome yes?
> > 
> > Probably. However, that does not compile currently, because the ChromeOS
> libbase
> > is old and things have been rearranged since it was last gardened.
> > 
> > I am not convinced it is OK to have a gardened library use base. Doing this
> > opens us up to breakage in response to rearrangements or changes to base.
> > 
> > We might want to check all of the gestures code into ToT. This would allow
> > updates to base and gestures to happen simultaneously.
> 
> gestures also uses C++11

We can land this now.

PTAL

[rjkroege, 2014-04-16 14:46:06]

lgtm

https://codereview.chromium.org/193813003/diff/60001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
(right):

https://codereview.chromium.org/193813003/diff/60001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc:30:
scoped_ptr<Delegate> delegate)
is this line too long?

[spang, 2014-04-16 15:46:37]

https://codereview.chromium.org/193813003/diff/60001/ui/events/ozone/evdev/li...
File ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
(right):

https://codereview.chromium.org/193813003/diff/60001/ui/events/ozone/evdev/li...
ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc:30:
scoped_ptr<Delegate> delegate)
On 2014/04/16 14:46:06, rjkroege wrote:
> is this line too long?

I count 79 chars.

[spang, 2014-04-16 15:46:44]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-04-16 15:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/spang@chromium.org/193813003/80001

[commit-bot: I haz the power, 2014-04-16 19:04:32]

Change committed as 264273