evdev: Support device hotplug with udev

evdev: Support device hotplug with udev

This watches for new input devices and opens any that appear for event
dispatching. This is only supported when use_udev==1.

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

[sadrul, 2014-01-30 22:10:35]

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.cc:248: converters_[path] =
converter.release();
Can we store scoped_ptr<>s in converters_?

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.h (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.h:63: scoped_ptr<UdevWatcher> watcher_;
Can there a be single scoped_ptr<UdevController> that we use in this file, with
the rest of the udev-dependent code (including UdevController, which could be
friend to this class) moved into some _udev file that we exclude from gyp when
udev==0? That way, there will be very little USE_UDEV checks in the code, and it
will be a bit clearer.

[rjkroege, 2014-01-30 22:18:09]

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_device_info.cc (left):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_device_info.cc:17:
base::ThreadRestrictions::AssertIOAllowed();
for ozone, it would be really nice to have a "only io for events is allowed on
the ui thread" instead of "all I/O is allowed in here"  what do you think?

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.h (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.h:63: scoped_ptr<UdevWatcher> watcher_;
On 2014/01/30 22:10:36, sadrul wrote:
> Can there a be single scoped_ptr<UdevController> that we use in this file,
with
> the rest of the udev-dependent code (including UdevController, which could be
> friend to this class) moved into some _udev file that we exclude from gyp when
> udev==0? That way, there will be very little USE_UDEV checks in the code, and
it
> will be a bit clearer.

+1

[spang, 2014-01-31 17:32:59]

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_device_info.cc (left):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_device_info.cc:17:
base::ThreadRestrictions::AssertIOAllowed();
On 2014/01/30 22:18:09, rjkroege wrote:
> for ozone, it would be really nice to have a "only io for events is allowed on
> the ui thread" instead of "all I/O is allowed in here"  what do you think?

Should that go in base::ThreadRestrictions?

I think that sounds like a separate change ;)

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.cc:248: converters_[path] =
converter.release();
On 2014/01/30 22:10:36, sadrul wrote:
> Can we store scoped_ptr<>s in converters_?

Don't think so. scoped_ptr<> is not copyable and so can't be used in STL
containers.

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.h (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.h:63: scoped_ptr<UdevWatcher> watcher_;
On 2014/01/30 22:18:09, rjkroege wrote:
> On 2014/01/30 22:10:36, sadrul wrote:
> > Can there a be single scoped_ptr<UdevController> that we use in this file,
> with
> > the rest of the udev-dependent code (including UdevController, which could
be
> > friend to this class) moved into some _udev file that we exclude from gyp
when
> > udev==0? That way, there will be very little USE_UDEV checks in the code,
and
> it
> > will be a bit clearer.
> 
> +1

Done.

[sadrul, 2014-01-31 21:00:58]

Mostly nits. LGTM. Rob might have additional input

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.cc:248: converters_[path] =
converter.release();
On 2014/01/31 17:33:00, spang wrote:
> On 2014/01/30 22:10:36, sadrul wrote:
> > Can we store scoped_ptr<>s in converters_?
> 
> Don't think so. scoped_ptr<> is not copyable and so can't be used in STL
> containers.

It looks like we have one std::map that stores scoped_ptr<>s (in
win8/metro_driver/chrome_app_view.h). If we can use something like that here,
that would be great, because it looks like an easy to thing to forget to
explicitly delete

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
File ui/events/ozone/evdev/device_manager_udev.cc (right):

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:121: ~DeviceManagerUdev() {}
virtual?

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:124: void
ScanAndStartMonitoring(const EvdevDeviceCallback& device_added,
virtual

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:140: void
OnFileCanReadWithoutBlocking(int fd) OVERRIDE {
virtual

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:159: void
OnFileCanWriteWithoutBlocking(int fd) OVERRIDE { NOTREACHED(); }
virtual

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:193:
base::MessagePumpLibevent::FileDescriptorWatcher controller_;

+ DISALLOW_COPY_AND_ASSIGN...

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory.cc:40: ~DeviceManagerManual() {}
virtual here too, I think?

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory.cc:43: void ScanAndStartMonitoring(const
EvdevDeviceCallback& device_added,
virtual

[spang, 2014-01-31 22:02:07]

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/20001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory.cc:248: converters_[path] =
converter.release();
On 2014/01/31 21:00:59, sadrul wrote:
> On 2014/01/31 17:33:00, spang wrote:
> > On 2014/01/30 22:10:36, sadrul wrote:
> > > Can we store scoped_ptr<>s in converters_?
> > 
> > Don't think so. scoped_ptr<> is not copyable and so can't be used in STL
> > containers.
> 
> It looks like we have one std::map that stores scoped_ptr<>s (in
> win8/metro_driver/chrome_app_view.h). If we can use something like that here,
> that would be great, because it looks like an easy to thing to forget to
> explicitly delete

It is easy to forget, and I'm not super happy with this either. This seems to be
the usual style, though - there's 600 call sites for STLDelete[...] in the tree.

That win8 example seems suspect. scoped_ptr<> does not meet the preconditions
for std::map and trying does not even compile with gcc.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
File ui/events/ozone/evdev/device_manager_udev.cc (right):

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:121: ~DeviceManagerUdev() {}
On 2014/01/31 21:00:59, sadrul wrote:
> virtual?

This does not have any effect but I guess it's clearer. Added.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:124: void
ScanAndStartMonitoring(const EvdevDeviceCallback& device_added,
On 2014/01/31 21:00:59, sadrul wrote:
> virtual

Done.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:140: void
OnFileCanReadWithoutBlocking(int fd) OVERRIDE {
On 2014/01/31 21:00:59, sadrul wrote:
> virtual

Done.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:159: void
OnFileCanWriteWithoutBlocking(int fd) OVERRIDE { NOTREACHED(); }
On 2014/01/31 21:00:59, sadrul wrote:
> virtual

Done.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:193:
base::MessagePumpLibevent::FileDescriptorWatcher controller_;
On 2014/01/31 21:00:59, sadrul wrote:
> 
> + DISALLOW_COPY_AND_ASSIGN...

Done.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
File ui/events/ozone/evdev/event_factory.cc (right):

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory.cc:40: ~DeviceManagerManual() {}
On 2014/01/31 21:00:59, sadrul wrote:
> virtual here too, I think?

Done.

https://codereview.chromium.org/150633005/diff/230001/ui/events/ozone/evdev/e...
ui/events/ozone/evdev/event_factory.cc:43: void ScanAndStartMonitoring(const
EvdevDeviceCallback& device_added,
On 2014/01/31 21:00:59, sadrul wrote:
> virtual

Done.

[rjkroege, 2014-02-03 15:36:55]

lgtm with concerns about threading choices and seeking tests.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
File ui/events/ozone/evdev/device_manager_udev.cc (right):

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:49: else  // SYS_LOG_DEBUG
nit: i think that it might be more chrome-y to return early and discard the
else. Or maybe I'm channeling webkit style.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:93: udev_list_entry_foreach(entry,
devices) {
this is a macro?

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:115: // It also creates & monitors
a udev netlink socket and issues callbacks for
what thread does this class run on? hopefully not the UI thread.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:117: class DeviceManagerUdev :
public DeviceManagerEvdev,
this class looks sufficiently rich as to require unit tests...  You can add them
in a different CL if you want. But I think they're a really good idea.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:178: return
base::MessagePumpOzone::Current()->WatchFileDescriptor(
this is on the UI thread. I think putting it on an I/O thread would be
preferable? Or can you convince me that udev is sufficiently bounded in latency
in permit execution on ui?

[spang, 2014-02-03 16:43:31]

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
File ui/events/ozone/evdev/device_manager_udev.cc (right):

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:49: else  // SYS_LOG_DEBUG
On 2014/02/03 15:36:55, rjkroege wrote:
> nit: i think that it might be more chrome-y to return early and discard the
> else. Or maybe I'm channeling webkit style. 

If you have a less vague consistency argument I will change it. I like this more
symmetrical way.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:93: udev_list_entry_foreach(entry,
devices) {
On 2014/02/03 15:36:55, rjkroege wrote:
> this is a macro?

Yes.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:115: // It also creates & monitors
a udev netlink socket and issues callbacks for
On 2014/02/03 15:36:55, rjkroege wrote:
> what thread does this class run on? hopefully not the UI thread. 

UI thread. Would like to land the single-threaded version first.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:117: class DeviceManagerUdev :
public DeviceManagerEvdev,
On 2014/02/03 15:36:55, rjkroege wrote:
> this class looks sufficiently rich as to require unit tests...  You can add
them
> in a different CL if you want. But I think they're a really good idea.

I will add a TODO for now and figure out how best to test this.

https://codereview.chromium.org/150633005/diff/370001/ui/events/ozone/evdev/d...
ui/events/ozone/evdev/device_manager_udev.cc:178: return
base::MessagePumpOzone::Current()->WatchFileDescriptor(
On 2014/02/03 15:36:55, rjkroege wrote:
> this is on the UI thread. I think putting it on an I/O thread would be
> preferable? Or can you convince me that udev is sufficiently bounded in
latency
> in permit execution on ui?

TODO.

[spang, 2014-02-03 17:30:04]

The CQ bit was checked by spang@chromium.org

[commit-bot: I haz the power, 2014-02-03 17:30:36]

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

[commit-bot: I haz the power, 2014-02-03 20:06:50]

Change committed as 248560

[commit-bot: I haz the power, 2014-02-03 20:06:58]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 20:06:59]

CQ bit was unchecked on CL. Ignoring.