evdev: Fix infinite loop when devices are unplugged

evdev: Fix infinite loop when devices are unplugged

File descriptors that have become disconnected are always readable and
cause the UI thread to spin. Stop watching once we've encountered an
error.

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

[spang, 2014-01-24 17:55:52]

On top of https://codereview.chromium.org/137273009/

[rjkroege, 2014-01-24 22:35:58]

Wouldn't it be simpler to add stop/start support to ECOz and make less of the
other changes? I must be missing something.

[spang, 2014-01-24 23:06:09]

On 2014/01/24 22:35:58, rjkroege wrote:
> Wouldn't it be simpler to add stop/start support to ECOz and make less of the
> other changes? I must be missing something.

I don't think the assumptions in the previous interface are broadly applicable.
The "event converter is a single device and has 1 file descriptor" is just too
simplistic to be fit all of our potential platforms. So that's why I'd like to
the file descriptor management from EventFactoryOzone into EventFactoryEvdev.

Neither wayland nor x11 actually fit into the current template.

[spang, 2014-01-24 23:13:25]

On 2014/01/24 23:06:09, spang wrote:
> On 2014/01/24 22:35:58, rjkroege wrote:
> > Wouldn't it be simpler to add stop/start support to ECOz and make less of
the
> > other changes? I must be missing something.
> 
> I don't think the assumptions in the previous interface are broadly
applicable.
> The "event converter is a single device and has 1 file descriptor" is just too
> simplistic to be fit all of our potential platforms. So that's why I'd like to
> the file descriptor management from EventFactoryOzone into EventFactoryEvdev.
> 
> Neither wayland nor x11 actually fit into the current template.

I've probably confused things by divided my changes up in a strange way. There's
more in the following CL:
https://codereview.chromium.org/133233008/

After that, EventConverterOzone is just something that calls Dispatch() on the
UI thread sometimes. No other policy is provided.

[kalyank, 2014-01-25 03:11:50]

On 2014/01/24 23:06:09, spang wrote:
> On 2014/01/24 22:35:58, rjkroege wrote:
> > Wouldn't it be simpler to add stop/start support to ECOz and make less of
the
> > other changes? I must be missing something.
> 
> I don't think the assumptions in the previous interface are broadly
applicable.
> The "event converter is a single device and has 1 file descriptor" is just too
> simplistic to be fit all of our potential platforms. So that's why I'd like to
> the file descriptor management from EventFactoryOzone into EventFactoryEvdev.
> 
> Neither wayland nor x11 actually fit into the current template.

Ya.

[rjkroege, 2014-01-27 19:03:16]

I think it's reasonable to extract all fd assumptions for EventConverterOzone
but perhaps that requires some other API updates?

https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_conver...
File ui/events/ozone/event_converter_ozone.h (right):

https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_conver...
ui/events/ozone/event_converter_ozone.h:23: // Start processing events.
Is it re-startable? I.e.: would one ever call Stop() and then later call
Start()?

Should Start and Stop be implicit on the constructor and destructor?

https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_factor...
File ui/events/ozone/event_factory_ozone.h (right):

https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_factor...
ui/events/ozone/event_factory_ozone.h:17: // Creates and dispatches
|ui.Event|'s. Ozone assumes that events arrive on file
You have convinced me that altering the assumption of 1-to-1 mapping between
fd's and EventCovnerter instances is reasonable. 

However, given that, there are additional parts of this class that need to
evolve right? Like the above text. And the AddEventConverter  -- the fd argument
is not necessarily appropriate? And the maps?

[spang, 2014-01-28 21:57:58]

I've dropped EventConverterEvdev and rebased this on top of
https://codereview.chromium.org/133233008/ .

On 2014/01/27 19:03:16, rjkroege wrote:
> I think it's reasonable to extract all fd assumptions for EventConverterOzone
> but perhaps that requires some other API updates?
> 
>
https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_conver...
> File ui/events/ozone/event_converter_ozone.h (right):
> 
>
https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_conver...
> ui/events/ozone/event_converter_ozone.h:23: // Start processing events.
> Is it re-startable? I.e.: would one ever call Stop() and then later call
> Start()?
> 
> Should Start and Stop be implicit on the constructor and destructor?

Without the base class, so I guess it has to be that way.

I don't want to add Start/Stop to EventConverterOzone, either, since I don't
want to re-add anything to that class that contains assumptions about the device
model for the platform. Starting and stopping individual converters when devices
come and go is an implementation detail.

> 
>
https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_factor...
> File ui/events/ozone/event_factory_ozone.h (right):
> 
>
https://codereview.chromium.org/144233006/diff/1/ui/events/ozone/event_factor...
> ui/events/ozone/event_factory_ozone.h:17: // Creates and dispatches
> |ui.Event|'s. Ozone assumes that events arrive on file
> You have convinced me that altering the assumption of 1-to-1 mapping between
> fd's and EventCovnerter instances is reasonable. 
> 
> However, given that, there are additional parts of this class that need to
> evolve right? Like the above text. And the AddEventConverter  -- the fd
argument
> is not necessarily appropriate? And the maps?

That's correct, see the other CL https://codereview.chromium.org/133233008/.

[rjkroege, 2014-01-28 23:30:25]

lgtm

[commit-bot: I haz the power, 2014-01-29 12:34:30]

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

[commit-bot: I haz the power, 2014-01-29 12:35:33]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-29 12:39:29]

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

[commit-bot: I haz the power, 2014-01-30 02:04:02]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2014-01-30 14:18:39]

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

[commit-bot: I haz the power, 2014-01-30 17:51:47]

Change committed as 247985

[commit-bot: I haz the power, 2014-01-30 17:51:48]

CQ bit was unchecked on CL. Ignoring.