evdev: Factor common code out of key & touch converters

evdev: Factor common code out of key & touch converters

This removes some dupication and also creates a common base class.

[rjkroege, 2014-01-24 22:25:36]

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"
this is unfortunate. Can it be 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.
why can't the instance variables still be private? I read the code reasonably
closely and making these non-private is unfortunate.

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_;
this can be 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_;
this too?

[spang, 2014-01-24 22:56:35]

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_;
+}

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.

[rjkroege, 2014-01-27 19:26:01]

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?

> 
>
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.

[spang, 2014-01-28 19:14:57]

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.