Moving input device hotplug event processing outside of ui/display

Moving input device hotplug event processing outside of ui/display

The change decouples input device hotplug event processing from output device
hotplug processing. In addition, for input device event processing, it takes
advantage of the X11 caches in ui/events and simplifies how hotplug events are
propagated to the system.

Depends on https://codereview.chromium.org/508303002/

BUG=381326
Committed: https://crrev.com/67548025123b035f4beacfea3d948e1a1b8eba5b
Cr-Commit-Position: refs/heads/master@{#294624}

[dnicoara, 2014-06-13 18:47:58]

There are a few different changes that aren't present in this CL before it is
done, but I'd like a bit of early feedback on the change and if you'd like the
observers to be wired in different parts.

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:266: DisplayInfo display =
display_manager->GetDisplayInfo(displays[i].id());
Bleh, I hate that we need to get the gfx::Displays and use them to get the
DisplayInfos. The reason behind this is because we're currently not removing old
displays from the mapping in DisplayManager.

This should be fixed otherwise a long standing session with lots of different
monitor connections/disconnections may cause an increase in memory usage.

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
File ash/display/display_change_observer_chromeos.h (right):

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.h:22: public
ui::InputDeviceEventObserver,
I originally wanted to put this in DisplayManager, but that proved to be harder
than expected due to how the state in updated within DisplayManager.

[Daniel Erat, 2014-06-17 21:59:19]

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:56: if (modes[i].native) {
nit: omit curly brackets on this one

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:72: gfx::Size native_size =
GetNativeModeSize(*state);;
nit: const

https://codereview.chromium.org/336863002/diff/1/ash/touch/touch_transformer_...
File ash/touch/touch_transformer_controller.cc (right):

https://codereview.chromium.org/336863002/diff/1/ash/touch/touch_transformer_...
ash/touch/touch_transformer_controller.cc:28: if (modes[i].native) {
nit: remove these curly brackets

https://codereview.chromium.org/336863002/diff/1/ui/events/device_data_manager.h
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/336863002/diff/1/ui/events/device_data_manage...
ui/events/device_data_manager.h:40: virtual std::vector<TouchscreenDevice>
GetTouchscreenDevices();
i think that there's still a general push to avoid non-interface inheritance in
chromium. how hard/messy would it be to restructure this class to e.g. be a pure
interface or call out to a passed-in object to get the touchscreen devices?

[dnicoara, 2014-06-20 17:01:05]

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:56: if (modes[i].native) {
On 2014/06/17 21:59:18, Daniel Erat wrote:
> nit: omit curly brackets on this one

Done.

https://codereview.chromium.org/336863002/diff/1/ash/display/display_change_o...
ash/display/display_change_observer_chromeos.cc:72: gfx::Size native_size =
GetNativeModeSize(*state);;
On 2014/06/17 21:59:18, Daniel Erat wrote:
> nit: const

Done.

https://codereview.chromium.org/336863002/diff/1/ash/touch/touch_transformer_...
File ash/touch/touch_transformer_controller.cc (right):

https://codereview.chromium.org/336863002/diff/1/ash/touch/touch_transformer_...
ash/touch/touch_transformer_controller.cc:28: if (modes[i].native) {
On 2014/06/17 21:59:18, Daniel Erat wrote:
> nit: remove these curly brackets

Done.

https://codereview.chromium.org/336863002/diff/1/ui/events/device_data_manager.h
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/336863002/diff/1/ui/events/device_data_manage...
ui/events/device_data_manager.h:40: virtual std::vector<TouchscreenDevice>
GetTouchscreenDevices();
On 2014/06/17 21:59:18, Daniel Erat wrote:
> i think that there's still a general push to avoid non-interface inheritance
in
> chromium. how hard/messy would it be to restructure this class to e.g. be a
pure
> interface or call out to a passed-in object to get the touchscreen devices?

I've been talking to sadrul@ and rjkroege@ a bit about DDM. The plan is to mold
DDM into a nicer interface to allow Ozone to hook into it. Rob is going to add
better Ozone support, so I think I'll wait until he makes more progress before
getting this CL into its final form. (I'll work on updating the unittests in the
meantime)

[dnicoara, 2014-09-11 15:11:35]

dnicoara@chromium.org changed reviewers:
- sadrul@chromium.org

[dnicoara, 2014-09-11 15:11:35]

-sadrul@ since the ui/events changes went in a different CL
(https://codereview.chromium.org/508303002/)

Daniel could you please take another look? I've updated this CL and with the
touchscreen management CL going in, this should be ready as well.

[Daniel Erat, 2014-09-11 20:34:01]

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
File ash/touch/touchscreen_util.cc (right):

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
ash/touch/touchscreen_util.cc:16: std::vector<ui::TouchscreenDevice> devices =
this function would probably be easier to test if you also passed in the
touchscreen devices, wouldn't it?

(but feel free to ignore this comment if you're intentionally trying to keep the
DeviceDataManager from leaking out into the callers.)

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
File ash/touch/touchscreen_util.h (right):

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
ash/touch/touchscreen_util.h:15: // Given a list of displays, it queries for the
active touchscreens and
nit: s/it //

[dnicoara, 2014-09-11 21:08:55]

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
File ash/touch/touchscreen_util.cc (right):

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
ash/touch/touchscreen_util.cc:16: std::vector<ui::TouchscreenDevice> devices =
On 2014/09/11 20:34:00, Daniel Erat wrote:
> this function would probably be easier to test if you also passed in the
> touchscreen devices, wouldn't it?
> 
> (but feel free to ignore this comment if you're intentionally trying to keep
the
> DeviceDataManager from leaking out into the callers.)

Done.

You're right, DisplayChangeObserverChromeos already has to deal with
DeviceDataManager, so it doesn't matter if it is called to get the touchscreens
as well.

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
File ash/touch/touchscreen_util.h (right):

https://codereview.chromium.org/336863002/diff/90001/ash/touch/touchscreen_ut...
ash/touch/touchscreen_util.h:15: // Given a list of displays, it queries for the
active touchscreens and
On 2014/09/11 20:34:00, Daniel Erat wrote:
> nit: s/it //

Done.

[Daniel Erat, 2014-09-11 21:51:35]

lgtm

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:246:
ui::DeviceDataManager::GetInstance()->touchscreen_devices();
nit: move this inline in the AssociateTouchscreens() call below?

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:288:
ui::DeviceDataManager::GetInstance()->touchscreen_devices();
nit: same here

[dnicoara, 2014-09-12 14:30:01]

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:246:
ui::DeviceDataManager::GetInstance()->touchscreen_devices();
On 2014/09/11 21:51:35, Daniel Erat wrote:
> nit: move this inline in the AssociateTouchscreens() call below?

Done.

https://codereview.chromium.org/336863002/diff/110001/ash/display/display_cha...
ash/display/display_change_observer_chromeos.cc:288:
ui::DeviceDataManager::GetInstance()->touchscreen_devices();
On 2014/09/11 21:51:35, Daniel Erat wrote:
> nit: same here

Done.

[dnicoara, 2014-09-12 16:46:36]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-09-12 16:49:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/336863002/150001

[commit-bot: I haz the power, 2014-09-12 19:08:23]

Committed patchset #9 (id:150001) as a05ee6952e240cd97710479606ed573fc35c87e5

[commit-bot: I haz the power, 2014-09-12 19:13:04]

Patchset 9 (id:??) landed as
https://crrev.com/67548025123b035f4beacfea3d948e1a1b8eba5b
Cr-Commit-Position: refs/heads/master@{#294624}