Expose a way for unit tests to terminate the AudioManager singleton to avoid conflicts between te...

Expose a way for unit tests to terminate the AudioManager singleton to avoid conflicts between tests.
This fixes occasional crashes when running audio tests manually (won't happen on the bots right now since they don't have the required hardware).

TEST=Run content tests with --gtest_filter=*WebRTC* --gtest_repeat=1000
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111586

[henrika (OOO until Aug 14), 2011-11-20 18:43:03]

Drive-by review. Some questions and comments.

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:118: // Utility class to
delete the AudioManager
AudioManager.

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:122: if
(DeleteAndResurrectIfExists()) {
I would call it DeleteAndRessurrect(). If not, it feels like the
delete-and-resurrect part can go wrong and it can't. You only return false for
!AudioManager::SingletonExists().

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:148:
TEST_F(WebRTCAudioDeviceTest, Construct) {
Why is this the only test were we don't add  if (IsRunningHeadless())?

[tommi (sloooow) - chröme, 2011-11-21 10:39:32]

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:118: // Utility class to
delete the AudioManager
On 2011/11/20 18:43:03, henrika1 wrote:
> AudioManager.

Done.

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:122: if
(DeleteAndResurrectIfExists()) {
On 2011/11/20 18:43:03, henrika1 wrote:
> I would call it DeleteAndRessurrect(). If not, it feels like the
> delete-and-resurrect part can go wrong and it can't. You only return false for
> !AudioManager::SingletonExists().

Done.  Although, FYI: The name says exactly what the method does. The case for
when it returns true and when it does not, is clearly documented. This is not a
public method, so it shouldn't confuse anyone using the class.

http://codereview.chromium.org/8558022/diff/1/content/renderer/media/webrtc_a...
content/renderer/media/webrtc_audio_device_unittest.cc:148:
TEST_F(WebRTCAudioDeviceTest, Construct) {
On 2011/11/20 18:43:03, henrika1 wrote:
> Why is this the only test were we don't add  if (IsRunningHeadless())?
This is the only test that can currently pass on the build bots.
Having 1 test that runs on the build bots is of course better than 0 and we've
already found several issues thanks to it.

[tommi (sloooow) - chröme, 2011-11-21 11:01:32]

Andrew, Henrik - thinking about this some more, should I move the utility
class to media/test?
Currently there are no cc/h files there, so I'm not sure about this but
since the AudioManager class
is fairly widely used, I can imagine that other tests than just the webrtc
tests might have to use the
utility class.

[Paweł Hajdan Jr., 2011-11-21 12:37:56]

Drive-by, did I comment on this singleton before? If so, please *listen* this
time.

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:122: if
(DeleteAndResurrect()) {
nit: Just EXPECT_FALSE(DeleteAndResurrect()) << "AudioManager ...";

But really, maybe just get rid of the Singleton. If it's causing you trouble,
don't paper over it, it'll only get worse. Try to fix the problem.

[tommi (sloooow) - chröme, 2011-11-21 15:10:10]

On Mon, Nov 21, 2011 at 1:37 PM, <phajdan.jr@chromium.org> wrote:

> Drive-by, did I comment on this singleton before? If so, please *listen*
> this
> time.
>

Some of the authors are cc-ed, so I guess your advice is directed at them.


> http://codereview.chromium.**org/8558022/diff/6001/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc<http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrtc_audio_device_unittest.cc>
> File content/renderer/media/webrtc_**audio_device_unittest.cc (right):
>
> http://codereview.chromium.**org/8558022/diff/6001/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc#newcode122<http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrtc_audio_device_unittest.cc#newcode122>
> content/renderer/media/webrtc_**audio_device_unittest.cc:122: if
> (DeleteAndResurrect()) {
> nit: Just EXPECT_FALSE(**DeleteAndResurrect()) << "AudioManager ...";
>
> But really, maybe just get rid of the Singleton. If it's causing you
> trouble, don't paper over it, it'll only get worse. Try to fix the
> problem.
>

Getting rid of the singleton would be great and of course something
we (Andrew and myself)
discussed and considered.  However, that is a major operation that we
should file a bug for getting fixed.
In the meantime, there's no need to keep the tests randomly crashing.


>
>
http://codereview.chromium.**org/8558022/<http://codereview.chromium.org/8558...
>

[scherkus (not reviewing), 2011-11-23 01:35:44]

LGTM w/ nits

agree w/ pawel -- let's get this in, reduce flakiness, then figure out a better
way to remove the singleton

file a bug under Feature-Media-Audio and cc' myself, vrk@, and enal@

thanks!

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:119: class
AutoAudioManagerCleanup {
can we file a bug w/ TODO for eliminating the singleton?

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:134: protected:
private?

[tommi (sloooow) - chröme, 2011-11-23 07:33:24]

Thanks.  Yes, we're all in agreement.  I already started working on it.

On Wed, Nov 23, 2011 at 2:35 AM, <scherkus@chromium.org> wrote:

> LGTM w/ nits
>
> agree w/ pawel -- let's get this in, reduce flakiness, then figure out a
> better
> way to remove the singleton
>
> file a bug under Feature-Media-Audio and cc' myself, vrk@, and enal@
>
> thanks!
>
>
>
> http://codereview.chromium.**org/8558022/diff/6001/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc<http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrtc_audio_device_unittest.cc>
> File content/renderer/media/webrtc_**audio_device_unittest.cc (right):
>
> http://codereview.chromium.**org/8558022/diff/6001/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc#newcode119<http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrtc_audio_device_unittest.cc#newcode119>
> content/renderer/media/webrtc_**audio_device_unittest.cc:119: class
> AutoAudioManagerCleanup {
> can we file a bug w/ TODO for eliminating the singleton?
>
> http://codereview.chromium.**org/8558022/diff/6001/content/**
>
renderer/media/webrtc_audio_**device_unittest.cc#newcode134<http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrtc_audio_device_unittest.cc#newcode134>
> content/renderer/media/webrtc_**audio_device_unittest.cc:134: protected:
> private?
>
>
http://codereview.chromium.**org/8558022/<http://codereview.chromium.org/8558...
>

[tommi (sloooow) - chröme, 2011-11-23 09:55:49]

All done.  New bug filed:
http://code.google.com/p/chromium/issues/detail?id=105249
Hope to get it done this week.

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:119: class
AutoAudioManagerCleanup {
On 2011/11/23 01:35:44, scherkus wrote:
> can we file a bug w/ TODO for eliminating the singleton?

Done.

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:122: if
(DeleteAndResurrect()) {
On 2011/11/21 12:37:57, Paweł Hajdan Jr. wrote:
> nit: Just EXPECT_FALSE(DeleteAndResurrect()) << "AudioManager ...";
> 
> But really, maybe just get rid of the Singleton. If it's causing you trouble,
> don't paper over it, it'll only get worse. Try to fix the problem.

Done.

http://codereview.chromium.org/8558022/diff/6001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:134: protected:
On 2011/11/23 01:35:44, scherkus wrote:
> private?

Done.

[commit-bot: I haz the power, 2011-11-23 09:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/8558022/15001

[tommi (sloooow) - chröme, 2011-11-23 13:05:38]

FYI - I'm not going to submit this just yet and possibly not at all.

The media tests aren't in a good state and apparently haven't been for some
time.  They've stayed green in the tree for weeks but the reason is simply that
the tests are not being run (the bots don't have the required hardware).  So,
changes have been checked in that relied only on bot placebo.  If you run the
tests locally however, you'll see them crash (from first glance it appears that
macs are currently "more broken" than other platforms).  The problems are
indirectly related to the AudioManager, so I don't think it's a good idea to
introduce the gun in the AutoAudioManagerCleanup constructor, as it is currently
aimed at my foot!

I've pinged some folks to fix this and in the meantime I'm working on issue
105249 (no AudioManager singleton).  It might be that I'll be done with those
changes when the media tests have been fixed.