Event handling support for ozone.

Event handling support for ozone.

Basic framework for handling events in ozone. Touch events are
recognized and dispatched. Some button events are supported.

BUG=178543
R=sadrul@chromium.org, sky@chromium.org

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

[rjkroege, 2013-06-05 19:22:06]

please take a look.

[sadrul, 2013-06-06 16:54:30]

mostly nits. LGTM

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_converter...
File ui/base/ozone/event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_converter...
ui/base/ozone/event_converter_ozone.h:13: class EventConverterOzone : public
base::MessagePumpLibevent::Watcher {
Doc

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
File ui/base/ozone/event_factory_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
ui/base/ozone/event_factory_ozone.cc:24: }
Looks like FileDescriptorWatcher dtor does this. So this may not be necessary.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
ui/base/ozone/event_factory_ozone.cc:42: fprintf(stderr, "watching dev input 
%s\n", path);
DLOG(INFO), or remove

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/events_ozone.cc
File ui/base/ozone/events_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/events_ozone.cc...
ui/base/ozone/events_ozone.cc:61: // return event->offset();
Remove

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
File ui/base/ozone/touch_event_converter_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.cc:45: if (close(fd_) < 0)
Looks like the KeyEventConverter is not closing the fd_. Perhaps this could move
into EventConverterOzone at some point (in a separate patch)

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.cc:80: sscanf(display, "%dx%d",
&screen_width, &screen_height);
Check this returns 2?

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
File ui/base/ozone/touch_event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.h:70: InProgressEvents
events_[MAX_FINGERS];
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/16466003/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/16466003/diff/1/ui/ui.gyp#newcode848
ui/ui.gyp:848: 'base/touch/touch_device_linux.cc',
This file doesn't seem to exist?

https://codereview.chromium.org/16466003/diff/1/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/16466003/diff/1/ui/ui_unittests.gypi#newcode221
ui/ui_unittests.gypi:221: ],
There's a rule in filename_rules.gypi that excludes x11_ files when use_x11 is
0. So this may not be necessary.

[rjkroege, 2013-06-06 23:05:05]

please take a look.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_converter...
File ui/base/ozone/event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_converter...
ui/base/ozone/event_converter_ozone.h:13: class EventConverterOzone : public
base::MessagePumpLibevent::Watcher {
On 2013/06/06 16:54:30, sadrul wrote:
> Doc

Done.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
File ui/base/ozone/event_factory_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
ui/base/ozone/event_factory_ozone.cc:24: }
On 2013/06/06 16:54:30, sadrul wrote:
> Looks like FileDescriptorWatcher dtor does this. So this may not be necessary.

true. I didn't want to trust to chance that the Watcher instances would get
deleted after the FileDescriptorWatcher instances and that an event would arrive
during the shutdown.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/event_factory_o...
ui/base/ozone/event_factory_ozone.cc:42: fprintf(stderr, "watching dev input 
%s\n", path);
On 2013/06/06 16:54:30, sadrul wrote:
> DLOG(INFO), or remove

Done.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/events_ozone.cc
File ui/base/ozone/events_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/events_ozone.cc...
ui/base/ozone/events_ozone.cc:61: // return event->offset();
On 2013/06/06 16:54:30, sadrul wrote:
> Remove

Done.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
File ui/base/ozone/touch_event_converter_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.cc:45: if (close(fd_) < 0)
On 2013/06/06 16:54:30, sadrul wrote:
> Looks like the KeyEventConverter is not closing the fd_. Perhaps this could
move
> into EventConverterOzone at some point (in a separate patch)

TODO added.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.cc:80: sscanf(display, "%dx%d",
&screen_width, &screen_height);
On 2013/06/06 16:54:30, sadrul wrote:
> Check this returns 2?

much improved. thanks.

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
File ui/base/ozone/touch_event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/1/ui/base/ozone/touch_event_con...
ui/base/ozone/touch_event_converter_ozone.h:70: InProgressEvents
events_[MAX_FINGERS];
On 2013/06/06 16:54:30, sadrul wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/16466003/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

https://codereview.chromium.org/16466003/diff/1/ui/ui.gyp#newcode848
ui/ui.gyp:848: 'base/touch/touch_device_linux.cc',
On 2013/06/06 16:54:30, sadrul wrote:
> This file doesn't seem to exist?

Done.

https://codereview.chromium.org/16466003/diff/1/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/16466003/diff/1/ui/ui_unittests.gypi#newcode221
ui/ui_unittests.gypi:221: ],
On 2013/06/06 16:54:30, sadrul wrote:
> There's a rule in filename_rules.gypi that excludes x11_ files when use_x11 is
> 0. So this may not be necessary.

Done.

[sky, 2013-06-07 04:57:24]

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
File ui/base/ozone/event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:22: EventConverterOzone() {}
The style guide says no to inlining of constructor/destructor.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:29: void DispatchEvent(ui::Event* event);
scoped_ptr<ui::Event> event to make ownership clear?

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:31:
DISALLOW_COPY_AND_ASSIGN(EventConverterOzone);
private:

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
File ui/base/ozone/event_factory_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
ui/base/ozone/event_factory_ozone.cc:28: for (int id = 0; true; id++) {
This is subtle enough that it's worth a comment.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
ui/base/ozone/event_factory_ozone.cc:29: char path[32];
Can we use one of the stringprintf functions that use a string so I don't have
to worry about overflow here?

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/events_ozon...
File ui/base/ozone/events_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/events_ozon...
ui/base/ozone/events_ozone.cc:30: ui::LocatedEvent* e =
static_cast<ui::LocatedEvent*>(native_event);
Can this be const ui::LocatedEvent* ?

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/events_ozon...
ui/base/ozone/events_ozone.cc:54: ui::Event* e =
static_cast<ui::Event*>(native_event);
Same thing about const ui::Event* here (casting away const is always bad).

[rjkroege, 2013-06-07 21:10:42]

ptal

note: I also had to make some additional minor changes to make us build after
merging.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
File ui/base/ozone/event_converter_ozone.h (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:22: EventConverterOzone() {}
On 2013/06/07 04:57:25, sky wrote:
> The style guide says no to inlining of constructor/destructor.

Done.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:29: void DispatchEvent(ui::Event* event);
On 2013/06/07 04:57:25, sky wrote:
> scoped_ptr<ui::Event> event to make ownership clear?

done.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_conve...
ui/base/ozone/event_converter_ozone.h:31:
DISALLOW_COPY_AND_ASSIGN(EventConverterOzone);
On 2013/06/07 04:57:25, sky wrote:
> private:

Done.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
File ui/base/ozone/event_factory_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
ui/base/ozone/event_factory_ozone.cc:28: for (int id = 0; true; id++) {
On 2013/06/07 04:57:25, sky wrote:
> This is subtle enough that it's worth a comment.

Done.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/event_facto...
ui/base/ozone/event_factory_ozone.cc:29: char path[32];
On 2013/06/07 04:57:25, sky wrote:
> Can we use one of the stringprintf functions that use a string so I don't have
> to worry about overflow here?

Done.

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/events_ozon...
File ui/base/ozone/events_ozone.cc (right):

https://codereview.chromium.org/16466003/diff/15001/ui/base/ozone/events_ozon...
ui/base/ozone/events_ozone.cc:30: ui::LocatedEvent* e =
static_cast<ui::LocatedEvent*>(native_event);
On 2013/06/07 04:57:25, sky wrote:
> Can this be const ui::LocatedEvent* ?

Done here and elsewhere.

[sky, 2013-06-10 15:01:08]

LGTM

[commit-bot: I haz the power, 2013-06-10 15:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/16466003/20001

[commit-bot: I haz the power, 2013-06-10 17:26:07]

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

[commit-bot: I haz the power, 2013-06-10 18:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rjkroege@chromium.org/16466003/61001

[rjkroege, 2013-06-10 22:11:58]

Committed patchset #6 manually as r205324 (presubmit successful).