Move touchscreen device caching to DeviceDataManager

Move touchscreen device caching to DeviceDataManager

Rather than poll for touchscreen devices in ui/display, centralize
X11 hotplug handling into ui/events and have TouchscreenDelegateImpl
reuse the cached device list.

In addition, DeviceDataManager will now cache all the touchscreen
devices and provides an observer pattern such that we can move input
event hotplugging out of ui/display entirely (this will be done in a
follow-up CL).

BUG=381326
Committed: https://crrev.com/1e77a6b0a26aecd03e8512e91cdd93befb8a66c0
Cr-Commit-Position: refs/heads/master@{#294388}

[dnicoara, 2014-08-27 22:39:31]

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

[dnicoara, 2014-08-27 22:39:31]

PTAL

[dnicoara, 2014-09-10 15:14:05]

dnicoara@chromium.org changed reviewers:
+ derat@chromium.org, spang@chromium.org

[dnicoara, 2014-09-10 15:14:06]

+ spang@ for ui/ozone
+ derat@ for ui/display

[Daniel Erat, 2014-09-10 16:51:25]

generally looks fine to me for ui/display and ui/events with some comments

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.h:17: explicit
TouchscreenDelegateImpl();
nit: 'explicit' is unneeded now

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.h:25: 
nit: delete blank line

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl_unittest.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:77:
DeviceHotplugEventObserver* device_delegate_;
nit: add a "// Not owned." comment here

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:77:
InputHotplugEventObserver(NativeDisplayDelegateX11::HelperDelegate* delegate)
nit: add 'explicit'

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:138:
ui::DeviceDataManager::GetInstance()->RemoveObserver(
wrap this in an "if (input_hotplug_observer_)" clause so you don't have an
unbalanced call to remove a NULL observer if an object is created and destroyed
without Initialize() getting called

https://codereview.chromium.org/508303002/diff/80001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/BUILD.gn#newco...
ui/events/BUILD.gn:29: "device_hotplug_event_observer.h",
fix alphabetization

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_data_ma...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_data_ma...
ui/events/device_data_manager.h:43: std::vector<TouchscreenDevice>
GetTouchscreenDevices() const;
can this instead be a simple inlined accessor that returns a const reference to
the member? (callers can assign to a local to do a copy if needed.)

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_hotplug...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_hotplug...
ui/events/device_hotplug_event_observer.h:19: virtual void OnTouchscreenDevices(
OnTouchscreenDevicesUpdated or OnTouchscreenDevicesChanged?

https://codereview.chromium.org/508303002/diff/80001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/events.gyp#new...
ui/events/events.gyp:53: 'device_hotplug_event_observer.h',
fix alphabetization

https://codereview.chromium.org/508303002/diff/80001/ui/events/input_device_e...
File ui/events/input_device_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/input_device_e...
ui/events/input_device_event_observer.h:10: class InputDeviceEventObserver {
please add a brief comment describing which class this observer is registered
with

https://codereview.chromium.org/508303002/diff/80001/ui/events/touchscreen_de...
File ui/events/touchscreen_device.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/touchscreen_de...
ui/events/touchscreen_device.cc:15: : id(id), size(size),
is_internal(is_internal) {
nit: in the past, the rule has been one-per-line if the list doesn't fit on the
same line as the signature. if clang-format still says that this is fine, feel
free to ignore me

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:78: if (event_node.empty() ||
!StartsWithASCII(event_node, "event", false)) {
nit: omit curly brackets

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.h:18:
HotplugEventHandlerX11(DeviceHotplugEventObserver* delegate);
add 'explicit'

[dnicoara, 2014-09-10 18:05:37]

Thanks for the quick review!

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.h:17: explicit
TouchscreenDelegateImpl();
On 2014/09/10 16:51:24, Daniel Erat wrote:
> nit: 'explicit' is unneeded now

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl.h:25: 
On 2014/09/10 16:51:24, Daniel Erat wrote:
> nit: delete blank line

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
File ui/display/chromeos/touchscreen_delegate_impl_unittest.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/touc...
ui/display/chromeos/touchscreen_delegate_impl_unittest.cc:77:
DeviceHotplugEventObserver* device_delegate_;
On 2014/09/10 16:51:24, Daniel Erat wrote:
> nit: add a "// Not owned." comment here

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:77:
InputHotplugEventObserver(NativeDisplayDelegateX11::HelperDelegate* delegate)
On 2014/09/10 16:51:24, Daniel Erat wrote:
> nit: add 'explicit'

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/display/chromeos/x11/...
ui/display/chromeos/x11/native_display_delegate_x11.cc:138:
ui::DeviceDataManager::GetInstance()->RemoveObserver(
On 2014/09/10 16:51:24, Daniel Erat wrote:
> wrap this in an "if (input_hotplug_observer_)" clause so you don't have an
> unbalanced call to remove a NULL observer if an object is created and
destroyed
> without Initialize() getting called

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/BUILD.gn
File ui/events/BUILD.gn (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/BUILD.gn#newco...
ui/events/BUILD.gn:29: "device_hotplug_event_observer.h",
On 2014/09/10 16:51:24, Daniel Erat wrote:
> fix alphabetization

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_data_ma...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_data_ma...
ui/events/device_data_manager.h:43: std::vector<TouchscreenDevice>
GetTouchscreenDevices() const;
On 2014/09/10 16:51:25, Daniel Erat wrote:
> can this instead be a simple inlined accessor that returns a const reference
to
> the member? (callers can assign to a local to do a copy if needed.)

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_hotplug...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/device_hotplug...
ui/events/device_hotplug_event_observer.h:19: virtual void OnTouchscreenDevices(
On 2014/09/10 16:51:25, Daniel Erat wrote:
> OnTouchscreenDevicesUpdated or OnTouchscreenDevicesChanged?

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/events.gyp
File ui/events/events.gyp (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/events.gyp#new...
ui/events/events.gyp:53: 'device_hotplug_event_observer.h',
On 2014/09/10 16:51:25, Daniel Erat wrote:
> fix alphabetization

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/input_device_e...
File ui/events/input_device_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/input_device_e...
ui/events/input_device_event_observer.h:10: class InputDeviceEventObserver {
On 2014/09/10 16:51:25, Daniel Erat wrote:
> please add a brief comment describing which class this observer is registered
> with

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/touchscreen_de...
File ui/events/touchscreen_device.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/touchscreen_de...
ui/events/touchscreen_device.cc:15: : id(id), size(size),
is_internal(is_internal) {
On 2014/09/10 16:51:25, Daniel Erat wrote:
> nit: in the past, the rule has been one-per-line if the list doesn't fit on
the
> same line as the signature. if clang-format still says that this is fine, feel
> free to ignore me

That's what clang format likes.

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.cc (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.cc:78: if (event_node.empty() ||
!StartsWithASCII(event_node, "event", false)) {
On 2014/09/10 16:51:25, Daniel Erat wrote:
> nit: omit curly brackets

Done.

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
File ui/events/x/hotplug_event_handler_x11.h (right):

https://codereview.chromium.org/508303002/diff/80001/ui/events/x/hotplug_even...
ui/events/x/hotplug_event_handler_x11.h:18:
HotplugEventHandlerX11(DeviceHotplugEventObserver* delegate);
On 2014/09/10 16:51:25, Daniel Erat wrote:
> add 'explicit'

Done.

[spang, 2014-09-10 18:47:15]

ui/ozone lgtm

[Daniel Erat, 2014-09-10 19:12:50]

lgtm

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/tou...
File ui/display/chromeos/touchscreen_delegate_impl.cc (right):

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/tou...
ui/display/chromeos/touchscreen_delegate_impl.cc:24:
std::vector<TouchscreenDevice> devices =
oh, and if you don't actually need to make a copy here, make the local be a
const std::vector<TouchscreenDevice>&; ditto for other callers

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/x11...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/x11...
ui/display/chromeos/x11/native_display_delegate_x11.cc:139: if
(input_hotplug_observer_)
nit: add curly brackets since the body of the if-statement is multiline

[dnicoara, 2014-09-10 19:24:25]

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/tou...
File ui/display/chromeos/touchscreen_delegate_impl.cc (right):

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/tou...
ui/display/chromeos/touchscreen_delegate_impl.cc:24:
std::vector<TouchscreenDevice> devices =
On 2014/09/10 19:12:50, Daniel Erat wrote:
> oh, and if you don't actually need to make a copy here, make the local be a
> const std::vector<TouchscreenDevice>&; ditto for other callers

Done.

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/x11...
File ui/display/chromeos/x11/native_display_delegate_x11.cc (right):

https://codereview.chromium.org/508303002/diff/120001/ui/display/chromeos/x11...
ui/display/chromeos/x11/native_display_delegate_x11.cc:139: if
(input_hotplug_observer_)
On 2014/09/10 19:12:50, Daniel Erat wrote:
> nit: add curly brackets since the body of the if-statement is multiline

Done.

[sadrul, 2014-09-10 19:31:12]

https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:21: };
Is the plan that this will grow to have other device updates (e.g.
OnExternalMouseAttached() etc.)?

[dnicoara, 2014-09-10 19:35:37]

https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
File ui/events/device_hotplug_event_observer.h (right):

https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
ui/events/device_hotplug_event_observer.h:21: };
On 2014/09/10 19:31:11, sadrul wrote:
> Is the plan that this will grow to have other device updates (e.g.
> OnExternalMouseAttached() etc.)?

That's what I was thinking. We already keep a separate list of other device
types (at least on X11). We could expose them through the observer in a similar
fashion.

[sadrul, 2014-09-10 19:44:36]

On 2014/09/10 19:35:37, dnicoara wrote:
>
https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
> File ui/events/device_hotplug_event_observer.h (right):
> 
>
https://codereview.chromium.org/508303002/diff/140001/ui/events/device_hotplu...
> ui/events/device_hotplug_event_observer.h:21: };
> On 2014/09/10 19:31:11, sadrul wrote:
> > Is the plan that this will grow to have other device updates (e.g.
> > OnExternalMouseAttached() etc.)?
> 
> That's what I was thinking. We already keep a separate list of other device
> types (at least on X11). We could expose them through the observer in a
similar
> fashion.

Cool. I think something like that will allow us to unify/eliminate things like
DeviceHierarchyObserver, PointerDeviceObserver,
XInputHierarchyChangedEventListener etc.

LGTM.

[dnicoara, 2014-09-11 13:48:50]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-09-11 13:49:51]

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

[commit-bot: I haz the power, 2014-09-11 15:09:33]

Committed patchset #8 (id:140001) as 16a99ba1c4da2f08fd61641e3e61e097be58dce1

[commit-bot: I haz the power, 2014-09-11 15:12:07]

Patchset 8 (id:??) landed as
https://crrev.com/1e77a6b0a26aecd03e8512e91cdd93befb8a66c0
Cr-Commit-Position: refs/heads/master@{#294388}