Refactoring to share MockAudioOutputStream implementations across 3 platforms...

Refactoring to share MockAudioOutputStream implementations across 3 platforms


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

[Alpha Left Google, 2009-07-14 01:30:55]

I used what was in audio_output_win.cc to create the MockAudioOutputStream. I
left the part of creating a new thread and simulating the behavior of a real
audio output stream because such test is not necessary in the moment. I think
our other integration tests can cover those cases

[cpu_(ooo_6.6-7.5), 2009-07-14 01:46:25]

LGTM

I am glad we removed the number of buffer thing. Never liked it.

I did not review the /linux/ files.

One comment:

http://codereview.chromium.org/155471/diff/1047/80
File media/audio/simple_sources_unittest.cc (right):

http://codereview.chromium.org/155471/diff/1047/80#newcode69
Line 69: // TODO(hclam): move this test to SimpleSourcesTest once mock audio
stream is
remove line 69, 70

[awong, 2009-07-14 01:47:34]

I'm not sure I understand why we need the faked audio output stream for this
unittest...

http://codereview.chromium.org/155471/diff/1047/93
File media/audio/mock_audio_output_stream.h (right):

http://codereview.chromium.org/155471/diff/1047/93#newcode17
Line 17: class MockAudioOutputStream : public AudioOutputStream {
Call this a Fake.  It's not a Mock because it's simulating actual behavior.

http://codereview.chromium.org/155471/diff/1047/80
File media/audio/simple_sources_unittest.cc (right):

http://codereview.chromium.org/155471/diff/1047/80#newcode70
Line 70: // implemented on other platform.
platform -> platforms.

http://codereview.chromium.org/155471/diff/1047/80#newcode86
Line 86: EXPECT_TRUE(oas->Open(samples * bytes_per_sample));
Can't you just test the SineWaveAudioSource directly?  Why do you have to go
through the audio output stream?

[Alpha Left Google, 2009-07-14 02:30:45]

The idea is to have a simple fake audio output stream to get rid of flakiness of
the real stream when testing more complicated code, like in AudioRendererHost.

[Alpha Left Google, 2009-07-14 03:17:22]

I changed the class name to FakeAudioOutputStream now.

http://codereview.chromium.org/155471/diff/1047/80
File media/audio/simple_sources_unittest.cc (right):

http://codereview.chromium.org/155471/diff/1047/80#newcode69
Line 69: // TODO(hclam): move this test to SimpleSourcesTest once mock audio
stream is
On 2009/07/14 01:46:25, cpu wrote:
> remove line 69, 70

done.

http://codereview.chromium.org/155471/diff/1047/80#newcode70
Line 70: // implemented on other platform.
On 2009/07/14 01:47:34, awong wrote:
> platform -> platforms.

removed.

http://codereview.chromium.org/155471/diff/1047/80#newcode86
Line 86: EXPECT_TRUE(oas->Open(samples * bytes_per_sample));
On 2009/07/14 01:47:34, awong wrote:
> Can't you just test the SineWaveAudioSource directly?  Why do you have to go
> through the audio output stream?

I guess this test is to make sure data is passed to the output stream.

[awong, 2009-07-14 03:26:21]

LGTM