Small refactor to media architecture in order to allow end-to-end tests.

Small refactor to media architecture in order to allow end-to-end tests.

- Introduced the class MockAudioManager that will be used by upcoming speech recognition tests.
- Added interface TestAudioInputController::Delegate, which allows to intercept Record/Close events.
- Added audio_manager_for_tests_ static field in SpeechRecognizer to inject the MockAudioManager during tests.

BUG=116954
TEST=none

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

[tommi (sloooow) - chröme, 2012-07-11 09:59:08]

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
File content/browser/speech/mock_audio_manager.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
content/browser/speech/mock_audio_manager.cc:103: };
remove semicolon

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
File content/browser/speech/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
content/browser/speech/mock_audio_manager.h:15: namespace speech {
why is the MockAudioManager in the speech namespace?
Actually, I think this file doesn't belong under content.  It should be under
media/audio and the namespace should be media.

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
content/browser/speech/mock_audio_manager.h:22: //    a dedicated audio thread,
easing their handling in the browser tests.
is there a chance of a deadlock in the destructor where ~AudioManagerBase stops
and waits for the audio thread (which is then the current thread, right?)?

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
content/browser/speech/mock_audio_manager.h:24: //
AudioManager::set_audio_manager_for_tests(new MockAudioManager()) before
set_audio_manager_for_tests?

https://chromiumcodereview.appspot.com/10704154/diff/2001/content/browser/spe...
content/browser/speech/mock_audio_manager.h:27: class MockAudioManager : public
media::AudioManagerBase {
What about inheriting from AudioManager instead of AudioManagerBase?  If you
really need AudioManagerBase, then please rename the mock class to
MockAudioManagerBase (and rename the file+fix #ifdefs).

https://chromiumcodereview.appspot.com/10704154/diff/2001/media/audio/audio_m...
File media/audio/audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2001/media/audio/audio_m...
media/audio/audio_manager.h:40: static void
set_audio_manager_for_tests(AudioManager* audio_manager);
Can you explain why you need this? I would like to avoid going down the route of
having global variables and static methods even if it's for tests only.
Can't you just inject your own AudioManager instance into the classes you want
to test?

[tommi (sloooow) - chröme, 2012-07-11 11:12:30]

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
content/browser/speech/speech_recognizer.cc:488:
recognition_engine_->AudioChunksEnded();
is this change intentional?

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
File content/browser/speech/speech_recognizer.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
content/browser/speech/speech_recognizer.h:60: static media::AudioManager*
audio_manager_for_tests_;
nit: keep this down where the other variables are.

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
File media/audio/mock_audio_manager.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.cc:69: return
content::BrowserThread::GetMessageLoopProxyForThread(thread_id_);
why don't we pass a pointer to a MessageLoopProxy to the constructor?  This
feels like unnecessarily bringing in BrowserThread dependency when we're really
only interested in a message loop.

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.cc:73: };
remove semicolon

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
File media/audio/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.h:41: const media::AudioParameters& params)
OVERRIDE;
fix indent here and below

https://chromiumcodereview.appspot.com/10704154/diff/2013/media/audio/test_au...
File media/audio/test_audio_input_controller_factory.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2013/media/audio/test_au...
media/audio/test_audio_input_controller_factory.h:57: AudioParameters
audio_parameters,
nit: const AudioParameters&

[Primiano Tucci (use gerrit), 2012-07-11 14:33:30]

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
File content/browser/speech/speech_recognizer.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
content/browser/speech/speech_recognizer.cc:488:
recognition_engine_->AudioChunksEnded();
On 2012/07/11 11:12:30, tommi wrote:
> is this change intentional?

Hmm it was intentional but not in this CL, reverted. Thanks for pointing out.

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
File content/browser/speech/speech_recognizer.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/content/browser/spe...
content/browser/speech/speech_recognizer.h:60: static media::AudioManager*
audio_manager_for_tests_;
On 2012/07/11 11:12:30, tommi wrote:
> nit: keep this down where the other variables are.

Done.

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
File media/audio/mock_audio_manager.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.cc:69: return
content::BrowserThread::GetMessageLoopProxyForThread(thread_id_);
On 2012/07/11 11:12:30, tommi wrote:
> why don't we pass a pointer to a MessageLoopProxy to the constructor?  This
> feels like unnecessarily bringing in BrowserThread dependency when we're
really
> only interested in a message loop.

I thought it was more loosely coupled since the thread_id_ is basically an
integer, while holding a (smart)pointer which keeps the MessageLoopProxy
refcounted could have some  side effects?

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.cc:73: };
On 2012/07/11 11:12:30, tommi wrote:
> remove semicolon

Done.

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
File media/audio/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/6003/media/audio/mock_au...
media/audio/mock_audio_manager.h:41: const media::AudioParameters& params)
OVERRIDE;
On 2012/07/11 11:12:30, tommi wrote:
> fix indent here and below

Done.

https://chromiumcodereview.appspot.com/10704154/diff/2013/media/audio/test_au...
File media/audio/test_audio_input_controller_factory.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2013/media/audio/test_au...
media/audio/test_audio_input_controller_factory.h:57: AudioParameters
audio_parameters,
On 2012/07/11 11:12:30, tommi wrote:
> nit: const AudioParameters&

Done.

[Primiano Tucci (use gerrit), 2012-07-11 16:42:05]

Changed MockAudioManager ctor to take a MessageLoopProxy instead of ThreadID

[tommi (sloooow) - chröme, 2012-07-12 08:32:05]

lgtm with one request below

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
File media/audio/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
media/audio/mock_audio_manager.h:25: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy);
should be:
explicit MockAudioManager(base::MessageLoopProxy* message_loop_proxy);

[Satish, 2012-07-12 09:38:13]

lgtm

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
File media/audio/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
media/audio/mock_audio_manager.h:12: // This class is a simple mock around
AudioManager, used exclusively for tests,
a newline above to make it easier for reading

[commit-bot: I haz the power, 2012-07-12 09:42:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/primiano@chromium.org/10704154/8003

[Primiano Tucci (use gerrit), 2012-07-12 09:42:03]

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
File media/audio/mock_audio_manager.h (right):

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
media/audio/mock_audio_manager.h:12: // This class is a simple mock around
AudioManager, used exclusively for tests,
On 2012/07/12 09:38:13, Satish wrote:
> a newline above to make it easier for reading

Done.

https://chromiumcodereview.appspot.com/10704154/diff/2021/media/audio/mock_au...
media/audio/mock_audio_manager.h:25: scoped_refptr<base::MessageLoopProxy>
message_loop_proxy);
On 2012/07/12 08:32:05, tommi wrote:
> should be:
> explicit MockAudioManager(base::MessageLoopProxy* message_loop_proxy);

Done.

[commit-bot: I haz the power, 2012-07-12 11:19:35]

Change committed as 146316

[Ami GONE FROM CHROMIUM, 2012-08-06 23:12:09]

https://chromiumcodereview.appspot.com/10704154/diff/8003/media/audio/test_au...
File media/audio/test_audio_input_controller_factory.cc (right):

https://chromiumcodereview.appspot.com/10704154/diff/8003/media/audio/test_au...
media/audio/test_audio_input_controller_factory.cc:58: void
TestAudioInputControllerFactory::SetDelegateForTests(
This is a dead method (nobody calls it).  Are there plans to use it sometime
soon or should it be deleted?
(if the latter, should more of this CL be reverted?)

[Primiano Tucci (use gerrit), 2012-08-07 07:12:31]

It was designed to be used by http://codereview.chromium.org/10703141/
I added hans@ in CC who tookover my work.

On 2012/08/06 23:12:09, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/10704154/diff/8003/media/audio/test_au...
> File media/audio/test_audio_input_controller_factory.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10704154/diff/8003/media/audio/test_au...
> media/audio/test_audio_input_controller_factory.cc:58: void
> TestAudioInputControllerFactory::SetDelegateForTests(
> This is a dead method (nobody calls it).  Are there plans to use it sometime
> soon or should it be deleted?
> (if the latter, should more of this CL be reverted?)

[hans, 2012-08-07 09:29:44]

On 2012/08/07 07:12:31, Primiano Tucci wrote:
> It was designed to be used by http://codereview.chromium.org/10703141/
> I added hans@ in CC who tookover my work.
> 
>
https://chromiumcodereview.appspot.com/10704154/diff/8003/media/audio/test_au...
> > media/audio/test_audio_input_controller_factory.cc:58: void
> > TestAudioInputControllerFactory::SetDelegateForTests(
> > This is a dead method (nobody calls it).  Are there plans to use it sometime
> > soon or should it be deleted?

Yes, as Primiano pointed out, there are plans to use it soon.