Extract touchscreen device management into a generic manager

Extract touchscreen device management into a generic manager

DeviceDataManager is currently X11 specific, so CrOS code that is responsible for touchscreen management would only work under X11. This CL starts extracting device state as generic state that X11 and Ozone implementations can share.

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

[dnicoara, 2014-05-21 20:54:09]

PTAL

[sadrul, 2014-05-27 15:50:20]

I have one suggestion about GetInstance() implementation. The rest of the
changes look good.

https://codereview.chromium.org/289283015/diff/20001/ui/events/device_data_ma...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/289283015/diff/20001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:22: DeviceDataManager*
DeviceDataManager::GetInstance() {
Can we change this to be more like how PlatformEventSource::GetInstance() works?

You will probably need to explicitly initialize DeviceDataManager somewhere
(looks like we don't do that at the moment, unfortunately).

[dnicoara, 2014-05-28 18:10:34]

Please take another look.

There probably are other unittests that may need to be updated, but I'd like to
know if I'm heading in the right direction.

[sadrul, 2014-05-30 09:44:30]

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:19:
base::AtExitManager::RegisterTask(base::Bind(&DeviceDataManager::OnExit,
base::DeletePointer<>?

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:31: //static
I am not a fan of the ifdef here. I think there are two ways we could proceed:

. Have something (X11EventSource?) explicitly create a DDMX11, which sets the
DDM instance (like we do for PlatformEventSource). For platforms that don't
create their own DDM implementation object, we create an instance of the default
DDM.

or

. Have a platform-independent non-overridable DDM object, and have platform
specific DDProvider implementations. The providers feed data to the DDM. In this
approach, there will be a transition period when some parts of the code reaches
directly into DDProviderX11 to get the necessary information, while some other
code gets the info from DDM. But ultimately, we would store all the relevant
data in DDM, and the sole purpose of DDProviderX11 would be to feed the data to
the DDM.

https://codereview.chromium.org/289283015/diff/40001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_event_source.h (right):

https://codereview.chromium.org/289283015/diff/40001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_event_source.h:20: class DeviceDataManager;
Don't need this?

[dnicoara, 2014-06-10 20:25:03]

Sorry for the delay, I look a further look at how we could further centralize
device management in DDM & was trying to make sure that this patch will align
with what we'd want.

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
File ui/events/device_data_manager.cc (right):

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:19:
base::AtExitManager::RegisterTask(base::Bind(&DeviceDataManager::OnExit,
On 2014/05/30 09:44:31, sadrul wrote:
> base::DeletePointer<>?

Nice! Thanks.

Done

https://codereview.chromium.org/289283015/diff/40001/ui/events/device_data_ma...
ui/events/device_data_manager.cc:31: //static
On 2014/05/30 09:44:31, sadrul wrote:
> I am not a fan of the ifdef here. I think there are two ways we could proceed:
> 
> . Have something (X11EventSource?) explicitly create a DDMX11, which sets the
> DDM instance (like we do for PlatformEventSource). For platforms that don't
> create their own DDM implementation object, we create an instance of the
default
> DDM.
> 
> or
> 
> . Have a platform-independent non-overridable DDM object, and have platform
> specific DDProvider implementations. The providers feed data to the DDM. In
this
> approach, there will be a transition period when some parts of the code
reaches
> directly into DDProviderX11 to get the necessary information, while some other
> code gets the info from DDM. But ultimately, we would store all the relevant
> data in DDM, and the sole purpose of DDProviderX11 would be to feed the data
to
> the DDM.

I went with the first option since it seemed more flexible for the intermediate
steps where X11 will likely need to get/set state.

Also went with the requirement of explicit creation. I'm not sure if you were
thinking that we could lazily set the default DDM if none is set.

https://codereview.chromium.org/289283015/diff/40001/ui/events/platform/x11/x...
File ui/events/platform/x11/x11_event_source.h (right):

https://codereview.chromium.org/289283015/diff/40001/ui/events/platform/x11/x...
ui/events/platform/x11/x11_event_source.h:20: class DeviceDataManager;
On 2014/05/30 09:44:31, sadrul wrote:
> Don't need this?

No, thank you!

Done

[sadrul, 2014-06-16 14:48:38]

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:106:
base::ShadowingAtExitManager at_exit_;
Requiring all tests that use the DDMX11 to have a ShadowingAtExitManager seems
like an extra burden. Can we remove this?

https://codereview.chromium.org/289283015/diff/200001/ui/aura/test/aura_test_...
File ui/aura/test/aura_test_base.cc (right):

https://codereview.chromium.org/289283015/diff/200001/ui/aura/test/aura_test_...
ui/aura/test/aura_test_base.cc:35: // Object registers itself with AtExitManager
?

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
What does this do? Should this be a separate change?

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:103: virtual ~DeviceDataManagerX11();
Make these private (because otherwise any number of DDMX11 can be created). Have
a DDMX11::CreateInstance() for creating it.

[sadrul, 2014-06-16 14:50:24]

Also, please add some details in the CL description.

[dnicoara, 2014-06-16 15:13:57]

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:106:
base::ShadowingAtExitManager at_exit_;
On 2014/06/16 14:48:37, sadrul wrote:
> Requiring all tests that use the DDMX11 to have a ShadowingAtExitManager seems
> like an extra burden. Can we remove this?

Only if DeviceDataManager allows calling CreateInstance() (soon to be
introduced) multiple times, rather than performing the CHECK in
DeviceDataManager's constructor. I was under the impression you didn't like this
approach.

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:103: virtual ~DeviceDataManagerX11();
On 2014/06/16 14:48:38, sadrul wrote:
> Make these private (because otherwise any number of DDMX11 can be created).
Have
> a DDMX11::CreateInstance() for creating it.

The CHECK in DDM's constructor would prevent that.

[dnicoara, 2014-06-16 15:16:00]

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
On 2014/06/16 14:48:38, sadrul wrote:
> What does this do? Should this be a separate change?

It's the GYP equivalent for export_dependent_settings. I was trying to have the
GN trybot happy as well.

[sadrul, 2014-06-17 20:37:08]

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:106:
base::ShadowingAtExitManager at_exit_;
On 2014/06/16 15:13:56, dnicoara wrote:
> On 2014/06/16 14:48:37, sadrul wrote:
> > Requiring all tests that use the DDMX11 to have a ShadowingAtExitManager
seems
> > like an extra burden. Can we remove this?
> 
> Only if DeviceDataManager allows calling CreateInstance() (soon to be
> introduced) multiple times,

We should avoid calling CreateInstance() from too many places. But calling them
multiple times doesn't sound bad. (of course, given that only the first call
actually creates a new instance, and subsequent calls are no-ops)

> rather than performing the CHECK in
> DeviceDataManager's constructor. I was under the impression you didn't like
this
> approach.

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
On 2014/06/16 15:16:00, dnicoara wrote:
> On 2014/06/16 14:48:38, sadrul wrote:
> > What does this do? Should this be a separate change?
> 
> It's the GYP equivalent for export_dependent_settings. I was trying to have
the
> GN trybot happy as well.

Can this be a separate CL? What change in this CL requires this?

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:103: virtual ~DeviceDataManagerX11();
On 2014/06/16 15:13:56, dnicoara wrote:
> On 2014/06/16 14:48:38, sadrul wrote:
> > Make these private (because otherwise any number of DDMX11 can be created).
> Have
> > a DDMX11::CreateInstance() for creating it.
> 
> The CHECK in DDM's constructor would prevent that.

Making them private would make this easier to understand when reading the code.

[dnicoara, 2014-06-18 14:04:29]

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/200001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:106:
base::ShadowingAtExitManager at_exit_;
On 2014/06/17 20:37:08, sadrul wrote:
> On 2014/06/16 15:13:56, dnicoara wrote:
> > On 2014/06/16 14:48:37, sadrul wrote:
> > > Requiring all tests that use the DDMX11 to have a ShadowingAtExitManager
> seems
> > > like an extra burden. Can we remove this?
> > 
> > Only if DeviceDataManager allows calling CreateInstance() (soon to be
> > introduced) multiple times,
> 
> We should avoid calling CreateInstance() from too many places. But calling
them
> multiple times doesn't sound bad. (of course, given that only the first call
> actually creates a new instance, and subsequent calls are no-ops)
> 
> > rather than performing the CHECK in
> > DeviceDataManager's constructor. I was under the impression you didn't like
> this
> > approach.
> 

Done.

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
On 2014/06/17 20:37:08, sadrul wrote:
> On 2014/06/16 15:16:00, dnicoara wrote:
> > On 2014/06/16 14:48:38, sadrul wrote:
> > > What does this do? Should this be a separate change?
> > 
> > It's the GYP equivalent for export_dependent_settings. I was trying to have
> the
> > GN trybot happy as well.
> 
> Can this be a separate CL? What change in this CL requires this?

Removed it from this CL.

The forward dependency was always needed, though, depending on the compile order
some of the targets, gfx would have already been included. 

device_data_manager.h uses gfx::Transform, thus targets depending on it will
need to link against gfx as well.

I feel pretty bad for breaking the GN build. IS GN a tree closer?

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
File ui/events/x/device_data_manager_x11.h (right):

https://codereview.chromium.org/289283015/diff/200001/ui/events/x/device_data...
ui/events/x/device_data_manager_x11.h:103: virtual ~DeviceDataManagerX11();
On 2014/06/17 20:37:08, sadrul wrote:
> On 2014/06/16 15:13:56, dnicoara wrote:
> > On 2014/06/16 14:48:38, sadrul wrote:
> > > Make these private (because otherwise any number of DDMX11 can be
created).
> > Have
> > > a DDMX11::CreateInstance() for creating it.
> > 
> > The CHECK in DDM's constructor would prevent that.
> 
> Making them private would make this easier to understand when reading the
code.

Done

[sadrul, 2014-06-18 16:18:14]

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
On 2014/06/18 14:04:29, dnicoara wrote:
> On 2014/06/17 20:37:08, sadrul wrote:
> > On 2014/06/16 15:16:00, dnicoara wrote:
> > > On 2014/06/16 14:48:38, sadrul wrote:
> > > > What does this do? Should this be a separate change?
> > > 
> > > It's the GYP equivalent for export_dependent_settings. I was trying to
have
> > the
> > > GN trybot happy as well.
> > 
> > Can this be a separate CL? What change in this CL requires this?
> 
> Removed it from this CL.
> 
> The forward dependency was always needed, though, depending on the compile
order
> some of the targets, gfx would have already been included. 
> 
> device_data_manager.h uses gfx::Transform, thus targets depending on it will
> need to link against gfx as well.
> 
> I feel pretty bad for breaking the GN build. IS GN a tree closer?

The GN builder is tree closer, yes. You should add back the changes in
|sources|.

The current x11 device-manager uses gfx::Transform, but it doesn't need this
change (i.e. the change in |forward_dependent_configs_from| (and
|export_dependent_settings| in gyp)). So I am curious to know what changed in
this CL to require this.

https://codereview.chromium.org/289283015/diff/260001/ui/ozone/ozone_platform.cc
File ui/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/289283015/diff/260001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.cc:40: DeviceDataManager::CreateInstance();
Do this after instance_->InitializeUI(), so that the platform-implementation has
a chance to create its own version of the DDM if it wants to.

[dnicoara, 2014-06-18 17:28:02]

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

https://codereview.chromium.org/289283015/diff/200001/ui/events/BUILD.gn#newc...
ui/events/BUILD.gn:69: forward_dependent_configs_from = [ "//ui/gfx" ]
On 2014/06/18 16:18:14, sadrul wrote:
> On 2014/06/18 14:04:29, dnicoara wrote:
> > On 2014/06/17 20:37:08, sadrul wrote:
> > > On 2014/06/16 15:16:00, dnicoara wrote:
> > > > On 2014/06/16 14:48:38, sadrul wrote:
> > > > > What does this do? Should this be a separate change?
> > > > 
> > > > It's the GYP equivalent for export_dependent_settings. I was trying to
> have
> > > the
> > > > GN trybot happy as well.
> > > 
> > > Can this be a separate CL? What change in this CL requires this?
> > 
> > Removed it from this CL.
> > 
> > The forward dependency was always needed, though, depending on the compile
> order
> > some of the targets, gfx would have already been included. 
> > 
> > device_data_manager.h uses gfx::Transform, thus targets depending on it will
> > need to link against gfx as well.
> > 
> > I feel pretty bad for breaking the GN build. IS GN a tree closer?
> 
> The GN builder is tree closer, yes. You should add back the changes in
> |sources|.
> 
> The current x11 device-manager uses gfx::Transform, but it doesn't need this
> change (i.e. the change in |forward_dependent_configs_from| (and
> |export_dependent_settings| in gyp)). So I am curious to know what changed in
> this CL to require this.

ui/events/platform/x11 didn't use to create the DDM so it didn't need to depend
on events_base. ui/events/platform/x11 doesn't directly depend on gfx (unlike
most other targets) so it complains about missing definitions.

https://codereview.chromium.org/289283015/diff/260001/ui/ozone/ozone_platform.cc
File ui/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/289283015/diff/260001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.cc:40: DeviceDataManager::CreateInstance();
On 2014/06/18 16:18:14, sadrul wrote:
> Do this after instance_->InitializeUI(), so that the platform-implementation
has
> a chance to create its own version of the DDM if it wants to.

Done.

[sadrul, 2014-06-20 13:37:52]

lgtm

https://codereview.chromium.org/289283015/diff/280001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/280001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:58:
ui::DeviceDataManagerX11::CreateInstance();
Is this necessary?

https://codereview.chromium.org/289283015/diff/280001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/289283015/diff/280001/ui/events/device_data_m...
ui/events/device_data_manager.h:42: int64_t
touch_device_to_display_map_[kMaxDeviceNum];
These should be privates. Have a protected static ::set_instance() (or just set
instance_ from the ctor).

I think kMaxDeviceNum can remain protected.

https://codereview.chromium.org/289283015/diff/280001/ui/ozone/ozone_platform.cc
File ui/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/289283015/diff/280001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.cc:42: DeviceDataManager::CreateInstance();
Add a comment here that this is deliberately being created after InitializeUI()
so that the platform gets a chance to creates its own version of DDM

[dnicoara, 2014-06-20 14:48:00]

https://codereview.chromium.org/289283015/diff/280001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/289283015/diff/280001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider_unittest.cc:58:
ui::DeviceDataManagerX11::CreateInstance();
On 2014/06/20 13:37:52, sadrul wrote:
> Is this necessary?

Unfortunately it is. I double-checked.

https://codereview.chromium.org/289283015/diff/280001/ui/events/device_data_m...
File ui/events/device_data_manager.h (right):

https://codereview.chromium.org/289283015/diff/280001/ui/events/device_data_m...
ui/events/device_data_manager.h:42: int64_t
touch_device_to_display_map_[kMaxDeviceNum];
On 2014/06/20 13:37:52, sadrul wrote:
> These should be privates. Have a protected static ::set_instance() (or just
set
> instance_ from the ctor).
> 
> I think kMaxDeviceNum can remain protected.

Done.

https://codereview.chromium.org/289283015/diff/280001/ui/ozone/ozone_platform.cc
File ui/ozone/ozone_platform.cc (right):

https://codereview.chromium.org/289283015/diff/280001/ui/ozone/ozone_platform...
ui/ozone/ozone_platform.cc:42: DeviceDataManager::CreateInstance();
On 2014/06/20 13:37:52, sadrul wrote:
> Add a comment here that this is deliberately being created after
InitializeUI()
> so that the platform gets a chance to creates its own version of DDM

Done.

[dnicoara, 2014-06-20 16:18:27]

+sky@ for ash/ chrome/ ui/gfx/

[sky, 2014-06-20 19:57:15]

Said files LGTM

[dnicoara, 2014-06-23 13:19:45]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-06-23 13:20:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/289283015/320001

[commit-bot: I haz the power, 2014-06-23 13:23:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-06-23 13:25:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-23 13:25:26]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[dnicoara, 2014-06-23 14:04:49]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-06-23 14:05:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/289283015/340001

[commit-bot: I haz the power, 2014-06-23 17:11:22]

Change committed as 279126