Changed DeviceDataManager to be able to create multiple instances for the purposes of test.

Changed DeviceDataManager to be able to create multiple instances for the purposes of test.

The DeviceDataManager was implemented as a singleton and it was impossible
to create two different instances in tests. This change makes it possible
for future test framework to create multiple instances.

BUG=499476
Committed: https://crrev.com/51ba4dd85063d575866a5003c2502c7fe88a10f8
Cr-Commit-Position: refs/heads/master@{#336558}

[bruthig, 2015-06-23 22:23:02]

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

[bruthig, 2015-06-23 22:23:03]

sadrul@, can you please have a look?

This change is going to be used by this CL here:
https://codereview.chromium.org/1182303005/

[sadrul, 2015-06-23 23:36:50]

On 2015/06/23 22:23:03, bruthig wrote:
> sadrul@, can you please have a look?
> 
> This change is going to be used by this CL here:
> https://codereview.chromium.org/1182303005/

Why would a test need to create multiple DDMs? (instead of, say, a TestApi that
can clear the states of the DDM)

[bruthig, 2015-06-24 14:53:42]

On 2015/06/23 23:36:50, sadrul wrote:
> On 2015/06/23 22:23:03, bruthig wrote:
> > sadrul@, can you please have a look?
> > 
> > This change is going to be used by this CL here:
> > https://codereview.chromium.org/1182303005/
> 
> Why would a test need to create multiple DDMs? (instead of, say, a TestApi
that
> can clear the states of the DDM)

(Sorry for the long winded response but I'm taking advantage of this concrete
example of why static singletons are a nightmare from a test-ability
perspective.)

There wouldn't necessarily be two instances of DDMs within the same test but
there should most definitely be a 'clean state' DDM at the start of every test.
One option to get a clean state DDM would be, like you said, to have a TestApi
that would reset the state.  However as the state complexity of the DDM
increases so will the 'reset' functionality of the TestApi. If this complexity
grows so much it might warrant it's own set of tests to ensure the reset
behaviour is working. And that starts to sound like a recursive and/or brittle
problem. One alternative to that approach is the one in this CL.  It removes the
responsibility of lifetime management from the DDM's constructor and moves it
solely in to the 'static *Instance' functions.  This will allow a TestApi to
call the private/protected constructor of DDM and the test's can manage the
lifetime of it.  Since the tests would be constructing the DDM each time it must
have a clean state, and this will always be the case no matter how complex the
internal state of the DDM becomes.

As it stands now, the internal state of the DDM is essentially global data. All
the member variables may as well be tracked as static variables.  And static
variables in a test environment encourage pollution between tests and are VERY
difficult to work around.

To play devil's advocate, what concerns do you have about separating lifetime
management responsibilities from construction ones?

[bruthig, 2015-06-24 18:13:24]

FYI I've updated https://codereview.chromium.org/1182303005/ to work with this
CL.

[sadrul, 2015-06-24 19:50:13]

I should have actually looked at the CL before commenting, sorry :)

The approach looks fine. We have the same pattern for other singleton objects
(e.g. aura::Env, ash::Shell etc.). But I think DeviceDataManagerX11 will not be
destroyed anymore?

https://codereview.chromium.org/1202383002/diff/1/ui/events/devices/device_da...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1202383002/diff/1/ui/events/devices/device_da...
ui/events/devices/device_data_manager.cc:46: CHECK(!instance_) << "Can not
create multiple instances of DeviceDataManager";
This check should be in set_instance above, right?

[bruthig, 2015-06-24 21:52:59]

sadrul@, can you have another look?

https://codereview.chromium.org/1202383002/diff/1/ui/events/devices/device_da...
File ui/events/devices/device_data_manager.cc (right):

https://codereview.chromium.org/1202383002/diff/1/ui/events/devices/device_da...
ui/events/devices/device_data_manager.cc:46: CHECK(!instance_) << "Can not
create multiple instances of DeviceDataManager";
On 2015/06/24 19:50:13, sadrul wrote:
> This check should be in set_instance above, right?

Done.

[sadrul, 2015-06-27 07:09:06]

This lgtm. But please consider the suggestion below (for both DDM and DDMX11.
Feel free to address that in a follow-up CL).

https://codereview.chromium.org/1202383002/diff/20001/ui/events/devices/x11/d...
File ui/events/devices/x11/device_data_manager_x11.cc (right):

https://codereview.chromium.org/1202383002/diff/20001/ui/events/devices/x11/d...
ui/events/devices/x11/device_data_manager_x11.cc:137:
base::Bind(&base::DeletePointer<DeviceDataManager>, device_data_manager));
Can we avoid setting a callback here, and explicitly call DDM::DeleteInstance()
instead? Looks like it would need to be called only in a few places?

[bruthig, 2015-06-29 14:01:28]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2015-06-29 14:01:51]

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

[commit-bot: I haz the power, 2015-06-29 14:52:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-29 14:53:30]

Patchset 2 (id:??) landed as
https://crrev.com/51ba4dd85063d575866a5003c2502c7fe88a10f8
Cr-Commit-Position: refs/heads/master@{#336558}