Make PointerDeviceObserver work under Ozone

Make PointerDeviceObserver work under Ozone

This CL:
- Makes PointerDeviceObserver get notified when a mouse or touchpad is
  plugged in or removed by making PointerDeviceObserver an
  InputDeviceEventObserver on ozone only.
- Adds InputDeviceEventObserver::OnMouseDeviceConfigurationChanged() and
       InputDeviceEventObserver::OnTouchpadDeviceConfigurationChanged().
- Adds the plumbing from EventFactoryEvdev to notify
  InputDeviceEventObservers when a mouse or touchpad is plugged in or
  removed.

In a later CL I will make PointerDeviceObserver an InputDeviceEventObserver on
X11 and add the X11 plumbing

BUG=450899
TEST=Manual, see bug

Committed: https://crrev.com/b726eb10541ccd17e678202bc7e502feb0854742
Cr-Commit-Position: refs/heads/master@{#314400}

[pkotwicz, 2015-02-03 00:04:10]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-02-03 00:08:19]

pkotwicz@chromium.org changed reviewers:
+ spang@chromium.org

[pkotwicz, 2015-02-03 00:08:19]

spang@ PTAL

I tried to keep this CL as simple as possible so that it is possible to
backport. Do you have any ideas as to how to make it simpler?

[spang, 2015-02-03 00:27:13]

On 2015/02/03 00:08:19, pkotwicz wrote:
> spang@ PTAL
> 
> I tried to keep this CL as simple as possible so that it is possible to
> backport. Do you have any ideas as to how to make it simpler?

Hm not really. I was thinking to just apply the previously configured settings
on hotplug but I guess that won't bubble up and cause the JS in the settings
page to update (& show/hide mouse or touchpad settings).

lgtm

[pkotwicz, 2015-02-03 01:21:01]

pkotwicz@chromium.org changed reviewers:
+ oshima@chromium.org

[pkotwicz, 2015-02-03 01:21:01]

Oshima for ash/ and chrome/browser/chromeos OWNERS

[pkotwicz, 2015-02-03 01:22:37]

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

[pkotwicz, 2015-02-03 01:22:38]

sadrul@ for ui/events OWNERS

[sadrul, 2015-02-03 01:29:13]

Please update the CL description to include a brief description of the changes
this CL makes.

[pkotwicz, 2015-02-03 14:46:56]

Sadrul, can you please take another look? Added a brief description.

[oshima, 2015-02-03 14:54:32]

ash, c/b/chromeos lgtm

[sadrul, 2015-02-03 15:29:04]

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/pointer_device_observer.cc (right):

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/pointer_device_observer.cc:32:
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
There's no reason to add this ifdef. We should just remove
XInputHierarchyChangedEventListener and DeviceHierarchyObserver, and use the
same pattern (i.e. use InputDeviceEventObserver) on X11. (you can reference
issue 360377)

https://codereview.chromium.org/889403003/diff/20001/ui/events/devices/device...
File ui/events/devices/device_data_manager.h (right):

https://codereview.chromium.org/889403003/diff/20001/ui/events/devices/device...
ui/events/devices/device_data_manager.h:68: const std::vector<InputDevice>&
deviecs) override;
*devices

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:344: // lists to
DeviceHotplugEventObserver.
Would we need to requery the list here? Presumably the list we get
(|device_list|) would include mouse and touchpad devices?

[pkotwicz, 2015-02-03 16:43:23]

Patchset #2 (id:40001) has been deleted

[pkotwicz, 2015-02-03 16:50:14]

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/pointer_device_observer.cc (right):

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/pointer_device_observer.cc:32:
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
I completely agree. I am not doing this as part of this CL because the bug is
marked as M41 release-block stable. I think that it will be easier to convince
dharani@ that the CL can be merged back if I do not modify the X11 code path. I
can file a bug and reference the bug in the code.

If we merge back this CL, we will need to merge back at least some of
https://codereview.chromium.org/868043003

dharani@ seems keen to have this bug fixed in M41. Do you have any suggestions
as to how to make this CL simpler?

Should I investigate in what the consequences would be if the all of the users
of PointerDeviceObserver thought that the mouse and touchpad always existed on
Ozone in M41?

[sadrul, 2015-02-03 17:33:04]

If we want to merge this back for 41, then this makes sense for now. But we
should clean up before 42.

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/system/pointer_device_observer.cc (right):

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/pointer_device_observer.cc:32:
ui::DeviceDataManager::GetInstance()->RemoveObserver(this);
On 2015/02/03 16:50:14, pkotwicz wrote:
> I completely agree. I am not doing this as part of this CL because the bug is
> marked as M41 release-block stable. I think that it will be easier to convince
> dharani@ that the CL can be merged back if I do not modify the X11 code path.
I
> can file a bug and reference the bug in the code.
> 
> If we merge back this CL, we will need to merge back at least some of
> https://codereview.chromium.org/868043003
> 
> dharani@ seems keen to have this bug fixed in M41. Do you have any suggestions
> as to how to make this CL simpler?

Unfortunately no. Providing default no-op impls on the new observer methods
(which I think we should always do; it doesn't make sense to force observers to
override everything) would make the line count smaller. But I don't know how
much that would help in terms of making this simple.

> 
> Should I investigate in what the consequences would be if the all of the users
> of PointerDeviceObserver thought that the mouse and touchpad always existed on
> Ozone in M41?

I am unsure. You can get confirmation from dharani@

https://codereview.chromium.org/889403003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/system/pointer_device_observer.cc:73:
weak_factory_.GetWeakPtr()));
side rant: eh, why is there an ozone implementation of InputDeviceSettings? Why
didn't we go through DDM?

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:344: // lists to
DeviceHotplugEventObserver.
On 2015/02/03 15:29:04, sadrul wrote:
> Would we need to requery the list here? Presumably the list we get
> (|device_list|) would include mouse and touchpad devices?

ping

[pkotwicz, 2015-02-03 18:09:33]

Sadrul, can you please take another look?

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_hotplug_event_handler.cc (right):

https://codereview.chromium.org/889403003/diff/20001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_hotplug_event_handler.cc:344: // lists to
DeviceHotplugEventObserver.
I have updated the comment so that it is clearer.

[pkotwicz, 2015-02-03 18:09:34]

Sadrul, can you please take another look?

[sadrul, 2015-02-03 18:10:46]

lgtm

[pkotwicz, 2015-02-03 19:33:04]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-02-03 19:34:14]

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

[commit-bot: I haz the power, 2015-02-03 20:33:35]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2015-02-03 20:35:27]

Patchset 3 (id:??) landed as
https://crrev.com/b726eb10541ccd17e678202bc7e502feb0854742
Cr-Commit-Position: refs/heads/master@{#314400}

[spang, 2015-02-03 20:43:23]

https://codereview.chromium.org/889403003/diff/80001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory_evdev.cc (left):

https://codereview.chromium.org/889403003/diff/80001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory_evdev.cc:255: // There's no list of mice in
DeviceDataManager.
Just noticed these comments got left behind. Something for the followup :)

[pkotwicz, 2015-02-04 03:33:42]

https://codereview.chromium.org/889403003/diff/80001/ui/events/ozone/evdev/ev...
File ui/events/ozone/evdev/event_factory_evdev.cc (left):

https://codereview.chromium.org/889403003/diff/80001/ui/events/ozone/evdev/ev...
ui/events/ozone/evdev/event_factory_evdev.cc:255: // There's no list of mice in
DeviceDataManager.
There still isn't a list of mice in DeviceDataManager. However, it probably
should have a list and we should check whether it has changed in
DeviceDataManager::OnMouseDevicesUpdated() for consistency