Issue 616633002: [Ozone-Evdev] Propagate touchscreen device information to DeviceDataManager

Issue 616633002: [Ozone-Evdev] Propagate touchscreen device information to DeviceDataManager (Closed)

Created:
6 years, 2 months ago by dnicoara

Modified:
6 years, 2 months ago

Reviewers:
spang

CC:
chromium-reviews, kalyank, tdresser+watch_chromium.org, ozone-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

[Ozone-Evdev] Propagate touchscreen device information to DeviceDataManager Propagate touchscreen information so that Ash can map touchscreens to displays. BUG=none NOTRY=true Committed: https://crrev.com/6a0ddb2270dc59311d1789cae155685c046c0c29 Cr-Commit-Position: refs/heads/master@{#297823}

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Total comments: 16

Patch Set 4 : . #

Patch Set 5 : . #

Patch Set 6 : . #

Total comments: 7

Patch Set 7 : . #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+109 lines, -25 lines)			Patch
M	ui/events/ozone/evdev/event_converter_evdev.h	View	1 2 3 4	3 chunks	+14 lines, -1 line	0 comments	Download
M	ui/events/ozone/evdev/event_converter_evdev.cc	View	1 2 3 4 5	2 chunks	+13 lines, -2 lines	0 comments	Download
M	ui/events/ozone/evdev/event_factory_evdev.h	View	1 2 3	2 chunks	+7 lines, -0 lines	0 comments	Download
M	ui/events/ozone/evdev/event_factory_evdev.cc	View	1 2 3 4 5 6	11 chunks	+41 lines, -8 lines	0 comments	Download
M	ui/events/ozone/evdev/key_event_converter_evdev.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/events/ozone/evdev/key_event_converter_evdev.cc	View	1 2 3 4	1 chunk	+2 lines, -1 line	0 comments	Download
M	ui/events/ozone/evdev/key_event_converter_evdev_unittest.cc	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.h	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc	View	1 2 3 4	1 chunk	+2 lines, -1 line	0 comments	Download
M	ui/events/ozone/evdev/touch_event_converter_evdev.h	View	1 2 3 4	2 chunks	+8 lines, -0 lines	0 comments	Download
M	ui/events/ozone/evdev/touch_event_converter_evdev.cc	View	1 2 3 4 5 6	3 chunks	+18 lines, -12 lines	0 comments	Download
M	ui/events/ozone/evdev/touch_event_converter_evdev_unittest.cc	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 17 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

spang

can you upload the followup? https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc File ui/events/ozone/evdev/event_converter_evdev.cc (right): https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc#newcode17 ui/events/ozone/evdev/event_converter_evdev.cc:17: scoped_ptr<EventDeviceInfo> device_info) Do we ...

6 years, 2 months ago (2014-09-29 23:54:36 UTC) #3

dnicoara

https://codereview.chromium.org/611423002 is a WIP. As per Sadrul's comment we can move the above WIP into ...

6 years, 2 months ago (2014-09-30 18:23:28 UTC) #4

spang

https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc File ui/events/ozone/evdev/event_converter_evdev.cc (right): https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc#newcode17 ui/events/ozone/evdev/event_converter_evdev.cc:17: scoped_ptr<EventDeviceInfo> device_info) On 2014/09/30 18:23:27, dnicoara wrote: > On ...

6 years, 2 months ago (2014-09-30 18:59:24 UTC) #5

dnicoara

6 years, 2 months ago (2014-09-30 19:09:32 UTC) #6

spang

On 2014/09/30 19:09:32, dnicoara wrote: > https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc > File ui/events/ozone/evdev/event_converter_evdev.cc (right): > > https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc#newcode17 > ...

6 years, 2 months ago (2014-09-30 19:18:22 UTC) #7

dnicoara

On 2014/09/30 19:18:22, spang wrote: > On 2014/09/30 19:09:32, dnicoara wrote: > > > https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/event_converter_evdev.cc ...

6 years, 2 months ago (2014-09-30 20:20:48 UTC) #8

spang

On 2014/09/30 20:20:48, dnicoara wrote: > On 2014/09/30 19:18:22, spang wrote: > > On 2014/09/30 ...

6 years, 2 months ago (2014-09-30 20:43:20 UTC) #9

On 2014/09/30 20:20:48, dnicoara wrote:
> On 2014/09/30 19:18:22, spang wrote:
> > On 2014/09/30 19:09:32, dnicoara wrote:
> > >
> >
>
https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/ev...
> > > File ui/events/ozone/evdev/event_converter_evdev.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/ev...
> > > ui/events/ozone/evdev/event_converter_evdev.cc:17:
> scoped_ptr<EventDeviceInfo>
> > > device_info)
> > > On 2014/09/30 18:59:23, spang wrote:
> > > > On 2014/09/30 18:23:27, dnicoara wrote:
> > > > > On 2014/09/29 23:54:35, spang wrote:
> > > > > > Do we actually need to store EventDeviceInfo? You really only need
to
> > keep
> > > > the
> > > > > > size..
> > > > > 
> > > > > Size + properties to figure out what type of device it is. This seems
> like
> > > the
> > > > > cleanest way.
> > > > 
> > > > Just add two functions to EventConvertEvdev:
> > > >   HasTouchscreen()
> > > >   GetTouchscreenSize()
> > > > 
> > > > Let's please use the ABS_MT ioctl info in one place only.
> > > 
> > > The getters never call the ioctls. One would have to run
> > > EventDeviceInfo::Initialize to perform the ioctls. Sorry, am I missing
> > > something?
> > 
> > "use the ioctl info" meaning read the kernel provided values out of
> > EventDeviceInfo. It's already used in TouchEventConverterEvdev::Init, just
do
> > 
> > // ui/events/ozone/evdev/touch_event_converter_evdev.cc:110
> > native_size_ = gfx::Size(x_num_tuxels_, y_num_tuxels_)
> > 
> > and add
> > 
> > gfx::Size GetNativeTouchscreenSize() { return native_size_; }
> > 
> > Then you can drop the change to hold onto the ioctl data.
> 
> Done.
> 
> Why do you prefer to restrict using EventDeviceInfo?

The stuff in absinfo isn't even constant, it contains the axis values which are
constantly changing. I'd rather not cache it wholesale if parts of it will soon
be invalid.

And it seems cleaner to snag the size in the code that is otherwise responsible
for interpreting the axis information, rather than duplicating this
responsibility in EventFactoryEvdev.

spang

https://codereview.chromium.org/616633002/diff/100001/ui/events/ozone/evdev/event_factory_evdev.cc File ui/events/ozone/evdev/event_factory_evdev.cc (right): https://codereview.chromium.org/616633002/diff/100001/ui/events/ozone/evdev/event_factory_evdev.cc#newcode261 ui/events/ozone/evdev/event_factory_evdev.cc:261: for (std::map<base::FilePath, EventConverterEvdev*>::iterator it = Should be able to ...

6 years, 2 months ago (2014-09-30 21:42:42 UTC) #10

dnicoara

On 2014/09/30 20:43:20, spang wrote: > On 2014/09/30 20:20:48, dnicoara wrote: > > On 2014/09/30 ...

6 years, 2 months ago (2014-10-01 19:49:04 UTC) #11

On 2014/09/30 20:43:20, spang wrote:
> On 2014/09/30 20:20:48, dnicoara wrote:
> > On 2014/09/30 19:18:22, spang wrote:
> > > On 2014/09/30 19:09:32, dnicoara wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/ev...
> > > > File ui/events/ozone/evdev/event_converter_evdev.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/616633002/diff/40001/ui/events/ozone/evdev/ev...
> > > > ui/events/ozone/evdev/event_converter_evdev.cc:17:
> > scoped_ptr<EventDeviceInfo>
> > > > device_info)
> > > > On 2014/09/30 18:59:23, spang wrote:
> > > > > On 2014/09/30 18:23:27, dnicoara wrote:
> > > > > > On 2014/09/29 23:54:35, spang wrote:
> > > > > > > Do we actually need to store EventDeviceInfo? You really only need
> to
> > > keep
> > > > > the
> > > > > > > size..
> > > > > > 
> > > > > > Size + properties to figure out what type of device it is. This
seems
> > like
> > > > the
> > > > > > cleanest way.
> > > > > 
> > > > > Just add two functions to EventConvertEvdev:
> > > > >   HasTouchscreen()
> > > > >   GetTouchscreenSize()
> > > > > 
> > > > > Let's please use the ABS_MT ioctl info in one place only.
> > > > 
> > > > The getters never call the ioctls. One would have to run
> > > > EventDeviceInfo::Initialize to perform the ioctls. Sorry, am I missing
> > > > something?
> > > 
> > > "use the ioctl info" meaning read the kernel provided values out of
> > > EventDeviceInfo. It's already used in TouchEventConverterEvdev::Init, just
> do
> > > 
> > > // ui/events/ozone/evdev/touch_event_converter_evdev.cc:110
> > > native_size_ = gfx::Size(x_num_tuxels_, y_num_tuxels_)
> > > 
> > > and add
> > > 
> > > gfx::Size GetNativeTouchscreenSize() { return native_size_; }
> > > 
> > > Then you can drop the change to hold onto the ioctl data.
> > 
> > Done.
> > 
> > Why do you prefer to restrict using EventDeviceInfo?
> 
> The stuff in absinfo isn't even constant, it contains the axis values which
are
> constantly changing. I'd rather not cache it wholesale if parts of it will
soon
> be invalid.
> 
> And it seems cleaner to snag the size in the code that is otherwise
responsible
> for interpreting the axis information, rather than duplicating this
> responsibility in EventFactoryEvdev.

Thanks, this makes more sense now. I didn't realize some of the information
could be stale.

dnicoara

6 years, 2 months ago (2014-10-01 19:49:14 UTC) #12

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/616633002/120001

6 years, 2 months ago (2014-10-02 13:34:38 UTC) #15

commit-bot: I haz the power

Committed patchset #7 (id:120001) as 29680a91f71190750d337b59c88cad3f438f5720

6 years, 2 months ago (2014-10-02 13:35:26 UTC) #16

commit-bot: I haz the power

6 years, 2 months ago (2014-10-02 13:36:01 UTC) #17

Message was sent while issue was closed.

Patchset 7 (id:??) landed as
https://crrev.com/6a0ddb2270dc59311d1789cae155685c046c0c29
Cr-Commit-Position: refs/heads/master@{#297823}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages