Mocks for all libcros elements

Mocks for all libcros elements. Added library registry object that provides access to all *Libray classes that used to be singletons. This registry adds an indirection layer that will let is replace the actual objects with mocks during test runs. 

BUG=chromium-os:2026
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42405

[zel, 2010-03-20 00:05:39]

Added cros library registry object that provides access to all other elements
that used to be singletons. This object provides a layer of indirection so each
*Library class can be mocked and replaced during test runs.

[DaveMoore, 2010-03-21 23:12:35]

I get that we need some indirection to allow mocking of the cros access classes,
but why do we need a single registry class? Couldn't we do it in each of the
cros classes themselves?

You would remove all the static methods from each class and make them instance
methods on the singleton and then have a private / friend method that would
allow registry of a mock class.

What's the value of the central registry class?

[zel, 2010-03-22 04:25:11]

Yes, there isn't just one way to deal with singleton in tests. For the
purpose of code testability, I prefer not having them at all. The
registry approach is just a standard code refactoring pattern that I've
picked for that goal.


On Sun, Mar 21, 2010 at 4:12 PM, <davemoore@chromium.org> wrote:

> I get that we need some indirection to allow mocking of the cros access
> classes,
> but why do we need a single registry class? Couldn't we do it in each of
> the
> cros classes themselves?
>
> You would remove all the static methods from each class and make them
> instance
> methods on the singleton and then have a private / friend method that would
> allow registry of a mock class.
>
> What's the value of the central registry class?
>
>
> http://codereview.chromium.org/1142005
>

[zel, 2010-03-22 14:17:10]

One more thing that I've forgot to mention - this class was intended to be
merged into CrosLibrary. That way we will have it really control libcros.so
access and we could potentially get rid of those EnsureLoaded calls that are
all over our code.


On Sun, Mar 21, 2010 at 9:25 PM, Zelidrag Hornung <zelidrag@chromium.org>wrote:

> Yes, there isn't just one way to deal with singleton in tests. For the
> purpose of code testability, I prefer not having them at all. The
> registry approach is just a standard code refactoring pattern that I've
> picked for that goal.
>
>
> On Sun, Mar 21, 2010 at 4:12 PM, <davemoore@chromium.org> wrote:
>
>> I get that we need some indirection to allow mocking of the cros access
>> classes,
>> but why do we need a single registry class? Couldn't we do it in each of
>> the
>> cros classes themselves?
>>
>> You would remove all the static methods from each class and make them
>> instance
>> methods on the singleton and then have a private / friend method that
>> would
>> allow registry of a mock class.
>>
>> What's the value of the central registry class?
>>
>>
>> http://codereview.chromium.org/1142005
>>
>
>

[DaveMoore, 2010-03-22 16:03:36]

Resolved questions in IM. LGTM.

[zel, 2010-03-22 19:51:26]

based on sync up over IM, I have merged repository's functionality into
CrosLibrary class and created an additional .so loading redirection layer.

[zel, 2010-03-23 02:14:46]

I had to separate interfaces from the implementation in order to get all targets
to build properly.

[tfarina (gmail-do not use), 2010-03-23 03:27:39]

http://codereview.chromium.org/1142005/diff/16014/36046
File chrome/browser/chromeos/cros/cros_library.cc (right):

http://codereview.chromium.org/1142005/diff/16014/36046#newcode56
chrome/browser/chromeos/cros/cros_library.cc:56: CryptohomeLibrary*
CrosLibrary::GetCryptohomeLibrary() {
Is this comment and the others below really necessary? Seems a bit redundant,
since the function name is self explanation.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode62
chrome/browser/chromeos/cros/cros_library.cc:62: // // Getter for
LanguageLibrary
nit: extra // and period at end of the sentence.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode76
chrome/browser/chromeos/cros/cros_library.cc:76: // Getter for MountLibrary
nit: period.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode83
chrome/browser/chromeos/cros/cros_library.cc:83: // Getter for NetworkLibrary
nit: period.

[tfarina (gmail-do not use), 2010-03-23 03:38:53]

http://codereview.chromium.org/1142005/diff/16014/36046
File chrome/browser/chromeos/cros/cros_library.cc (right):

http://codereview.chromium.org/1142005/diff/16014/36046#newcode106
chrome/browser/chromeos/cros/cros_library.cc:106: SynapticsLibrary*
CrosLibrary::GetSynapticsLibrary() {
nit: this should match with the order of the header file.

http://codereview.chromium.org/1142005/diff/16014/36052
File chrome/browser/chromeos/cros/cros_library_loader.cc (right):

http://codereview.chromium.org/1142005/diff/16014/36052#newcode30
chrome/browser/chromeos/cros/cros_library_loader.cc:30: }
nit: // namespace chromeos

[zel, 2010-03-23 18:15:08]

http://codereview.chromium.org/1142005/diff/16014/36046
File chrome/browser/chromeos/cros/cros_library.cc (right):

http://codereview.chromium.org/1142005/diff/16014/36046#newcode56
chrome/browser/chromeos/cros/cros_library.cc:56: CryptohomeLibrary*
CrosLibrary::GetCryptohomeLibrary() {
On 2010/03/23 03:27:40, tfarina wrote:
> Is this comment and the others below really necessary? Seems a bit redundant,
> since the function name is self explanation.

Done.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode62
chrome/browser/chromeos/cros/cros_library.cc:62: // // Getter for
LanguageLibrary
On 2010/03/23 03:27:40, tfarina wrote:
> nit: extra // and period at end of the sentence.

Done.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode76
chrome/browser/chromeos/cros/cros_library.cc:76: // Getter for MountLibrary
On 2010/03/23 03:27:40, tfarina wrote:
> nit: period.

Done.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode83
chrome/browser/chromeos/cros/cros_library.cc:83: // Getter for NetworkLibrary
On 2010/03/23 03:27:40, tfarina wrote:
> nit: period.

Done.

http://codereview.chromium.org/1142005/diff/16014/36046#newcode106
chrome/browser/chromeos/cros/cros_library.cc:106: SynapticsLibrary*
CrosLibrary::GetSynapticsLibrary() {
On 2010/03/23 03:38:53, tfarina wrote:
> nit: this should match with the order of the header file.

Done.

http://codereview.chromium.org/1142005/diff/16014/36052
File chrome/browser/chromeos/cros/cros_library_loader.cc (right):

http://codereview.chromium.org/1142005/diff/16014/36052#newcode30
chrome/browser/chromeos/cros/cros_library_loader.cc:30: }
On 2010/03/23 03:38:53, tfarina wrote:
> nit: // namespace chromeos

Done.

[DaveMoore, 2010-03-23 18:25:32]

LGTM

http://codereview.chromium.org/1142005/diff/91001/64030
File chrome/browser/chromeos/cros/cros_library.h (right):

http://codereview.chromium.org/1142005/diff/91001/64030#newcode48
chrome/browser/chromeos/cros/cros_library.h:48: void
SetSynapticsLibrary(SynapticsLibrary* library);
Nit: nl

[zel, 2010-03-23 22:07:12]

http://codereview.chromium.org/1142005/diff/91001/64030
File chrome/browser/chromeos/cros/cros_library.h (right):

http://codereview.chromium.org/1142005/diff/91001/64030#newcode48
chrome/browser/chromeos/cros/cros_library.h:48: void
SetSynapticsLibrary(SynapticsLibrary* library);
On 2010/03/23 18:25:32, davemoore wrote:
> Nit: nl

Done.