Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(187)

Issue 137273009: evdev: Factor common code out of key & touch converters (Closed)

Created:
6 years, 11 months ago by spang
Modified:
6 years, 10 months ago
Reviewers:
rjkroege
CC:
chromium-reviews, kalyank, ozone-reviews_chromium.org
Visibility:
Public.

Description

evdev: Factor common code out of key & touch converters This removes some dupication and also creates a common base class.

Patch Set 1 #

Total comments: 8

Patch Set 2 : remove duplicate variables & unneeded #include #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -81 lines) Patch
M ui/events/events.gyp View 1 1 chunk +2 lines, -0 lines 0 comments Download
A + ui/events/ozone/evdev/event_converter.h View 1 3 chunks +14 lines, -15 lines 0 comments Download
A ui/events/ozone/evdev/event_converter.cc View 1 chunk +42 lines, -0 lines 0 comments Download
M ui/events/ozone/evdev/event_factory.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/events/ozone/evdev/key_event_converter.h View 2 chunks +4 lines, -15 lines 0 comments Download
M ui/events/ozone/evdev/key_event_converter.cc View 1 chunk +2 lines, -19 lines 0 comments Download
M ui/events/ozone/evdev/touch_event_converter.h View 1 2 chunks +9 lines, -12 lines 0 comments Download
M ui/events/ozone/evdev/touch_event_converter.cc View 2 chunks +10 lines, -19 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
spang
6 years, 11 months ago (2014-01-24 17:13:32 UTC) #1
rjkroege
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h File ui/events/ozone/evdev/event_converter.h (right): https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h#newcode9 ui/events/ozone/evdev/event_converter.h:9: #include "base/message_loop/message_pump_ozone.h" this is unfortunate. Can it be removed? ...
6 years, 11 months ago (2014-01-24 22:25:36 UTC) #2
spang
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h File ui/events/ozone/evdev/event_converter.h (right): https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h#newcode9 ui/events/ozone/evdev/event_converter.h:9: #include "base/message_loop/message_pump_ozone.h" On 2014/01/24 22:25:36, rjkroege wrote: > this ...
6 years, 11 months ago (2014-01-24 22:56:35 UTC) #3
rjkroege
On 2014/01/24 22:56:35, spang wrote: > https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h > File ui/events/ozone/evdev/event_converter.h (right): > > https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_converter.h#newcode9 > ...
6 years, 10 months ago (2014-01-27 19:26:01 UTC) #4
spang
6 years, 10 months ago (2014-01-28 19:14:57 UTC) #5
On 2014/01/27 19:26:01, rjkroege wrote:
> On 2014/01/24 22:56:35, spang wrote:
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_...
> > File ui/events/ozone/evdev/event_converter.h (right):
> > 
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_...
> > ui/events/ozone/evdev/event_converter.h:9: #include
> > "base/message_loop/message_pump_ozone.h"
> > On 2014/01/24 22:25:36, rjkroege wrote:
> > > this is unfortunate. Can it be removed?
> > 
> > Oops. Removed.
> > 
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/event_...
> > ui/events/ozone/evdev/event_converter.h:34: // File descriptor for the
> > /dev/input/event* instance.
> > On 2014/01/24 22:25:36, rjkroege wrote:
> > > why can't the instance variables still be private? I read the code
> reasonably
> > > closely and making these non-private is unfortunate.
> > 
> > They are used in the subclasses. Would you prefer something like this:
> > 
> > +int EventConverterEvdev::device_fd() const { return device_fd_; }
> > +
> > +const base::FilePath& EventConverterEvdev::path() const { return path_; }
> > +
> > +const EventModifiersEvdev& EventConverterEvdev::modifiers() const {
> > +  return *modifiers_;
> > +}
> > +
> > +EventModifiersEvdev* EventConverterEvdev::mutable_modifiers() {
> > +  return modifiers_;
> > +}
> 
> Actually, I think that I'd prefer that you'd left the fd_ etc. in the
> {key|touch}_event_converter. At least so far, the change factors out code that
> mostly consists of a read and some (necessary and desirable alterations to
errno
> checking) Wouldn't it be simpler to put the read in a utility function
somewhere
> and not bother introducing a new class?

I am actually not sure that splitting by device "type" here is the right way at
all. As far as the kernel interface is concerned, the types of events a device
can generate is a giant bitmask. You can have relative axes, absolute axes,
mouse buttons, and a full keyboard, all at the same time. So that was one reason
I felt it was natural to start in the direction of unification. The only thing
stopping me from immediately merging it all together is that I do not have any
examples of such an "innovative" device. Splitting by type does work in
practice.

I think the "correct" way might be to have a common path for reading the events
and a delegate for allowing that need postprocessing (the ABS axes in
particular, since they might be mapped to screen coordinates or converted to
pointer movement).

I will revert this out of my tree for now, and we can argue about how this
should work over the next while.

> 
> > 
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/touch_...
> > File ui/events/ozone/evdev/touch_event_converter.h (right):
> > 
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/touch_...
> > ui/events/ozone/evdev/touch_event_converter.h:57: int fd_;
> > On 2014/01/24 22:25:36, rjkroege wrote:
> > > this can be removed?
> > 
> > Oops. Removed.
> > 
> >
>
https://codereview.chromium.org/137273009/diff/1/ui/events/ozone/evdev/touch_...
> > ui/events/ozone/evdev/touch_event_converter.h:60: base::FilePath path_;
> > On 2014/01/24 22:25:36, rjkroege wrote:
> > > this too?
> > 
> > Done.

Powered by Google App Engine
This is Rietveld 408576698