Adds method to provide custom AudioManager at runtime.

Adds method to provide custom AudioManager at runtime.

Adds a static SetFactory method to media::AudioManager which allows a
caller to pass an AudioManagerFactory to the AudioMananger. The static
instance of AudioManager will be obtained from this factory.

Also adds an implementation for Cast, to be used on third party
platforms, with associated tests.

BUG=none

Committed: https://crrev.com/8e273264c96562ee35f60b03a1ef4e32ef8f7fcf
Cr-Commit-Position: refs/heads/master@{#326604}

[slan, 2015-04-23 06:24:28]

slan@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[slan, 2015-04-23 06:24:29]

This CL allows an implementation of AudioManager to be provided at runtime,
particularly so third-party code can avoid statically linking in a
platform-specific implementation.

Thank you for taking a look.

[DaleCurtis, 2015-04-23 16:18:26]

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager.h:33: // Used to override the defualt implementation.
After this is called, all
default.

Add some more details about why this is being added: I.e. dynamically linked 3rd
party clients.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager.h:56: static void ResetFactory();
Name it ResetFactoryForTesting, so presubmit catches bad usage.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager_factory.h (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
no (c) anymore.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory.h:19: AudioManagerFactory() {}
No constructor on pure virtual interfaces.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager_factory_unittest.cc (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory_unittest.cc:48: AudioManager::SetFactory(new
FakeAudioManagerFactory);
()

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory_unittest.cc:56: // Reset the AudioManager and
AudioManagerFactory to prevent persisting state
I don't think this is an issue. Or we'd have tripped the g_last_created CHECK()
in the existing tests, no?

[slan, 2015-04-23 18:11:33]

Hey Dale, thanks for the timely and thorough review.

I made some improvements to the test factory. Please take a look.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager.h (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager.h:33: // Used to override the defualt implementation.
After this is called, all
On 2015/04/23 16:18:25, DaleCurtis wrote:
> default.
> 
> Add some more details about why this is being added: I.e. dynamically linked
3rd
> party clients.

Done.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager.h:56: static void ResetFactory();
On 2015/04/23 16:18:25, DaleCurtis wrote:
> Name it ResetFactoryForTesting, so presubmit catches bad usage.

Done.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager_factory.h (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/04/23 16:18:25, DaleCurtis wrote:
> no (c) anymore.

Done.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory.h:19: AudioManagerFactory() {}
On 2015/04/23 16:18:25, DaleCurtis wrote:
> No constructor on pure virtual interfaces.

Done.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
File media/audio/audio_manager_factory_unittest.cc (right):

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory_unittest.cc:48: AudioManager::SetFactory(new
FakeAudioManagerFactory);
On 2015/04/23 16:18:26, DaleCurtis wrote:
> ()

Done. Here and above.

https://codereview.chromium.org/1104583002/diff/20001/media/audio/audio_manag...
media/audio/audio_manager_factory_unittest.cc:56: // Reset the AudioManager and
AudioManagerFactory to prevent persisting state
On 2015/04/23 16:18:25, DaleCurtis wrote:
> I don't think this is an issue. Or we'd have tripped the g_last_created
CHECK()
> in the existing tests, no?

Any existing unittest using AudioManager wraps the created instance in a
scoped_ptr, such that it is destroyed when the scope of the test exits. That is
what we are doing here (I have removed the redundant call to reset() and added a
comment for clarity :) ). I believe that gtest guarantees serial execution of
all test cases within a process, so that should be sufficient to avoid the CHECK
you mentioned.

The call to AudioManager::ResetFactory() is also important, but for a different
reason. If it is not called, the static instance of the factory will continue to
serve whatever AudioManager it wants (in this case |g_fake_audio_manager|, which
is deleted when this scope exits.... yikes!) when Create is called, for the life
of the process. This puts all tests run after this one on the same process at
the mercy of this factory, which is not good.

I'm glad you pointed this out though, because returning |g_fake_audio_manager|
violates the assumption that applies everywhere else... that the instance can be
deleted in testing without issue. I have fixed that, so the assumption applies. 

But in any case, the factory still needs to be reset when all the tests in the
file are through.

[DaleCurtis, 2015-04-23 18:14:43]

lgtm

[slan, 2015-04-23 18:17:03]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-04-23 18:19:25]

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

[commit-bot: I haz the power, 2015-04-23 19:26:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-23 19:27:12]

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