Added unittest to WebRtcAudioRenderer.

Added unittest to WebRtcAudioRenderer. This is to protect the use cases that WebRtcAudioRenderer will be shared by multiple audio tags.

NOTRY=true

BUG=351895, 264611, 351270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257388

[no longer working on chromium, 2014-03-13 13:53:02]

Tommi, please take a look.

Thanks,
SX

[tommi (sloooow) - chröme, 2014-03-13 14:25:49]

instead of adding "fortesting" code to WebRtcAudioRenderer, can you use the
|factory_| support in AudioDeviceFactory?

I really don't like ForTesting since it pollutes production code with test code
and has test specific checks that make tests run different code than production
does.

[no longer working on chromium, 2014-03-13 17:11:10]

Tommi, PTAL.

[tommi (sloooow) - chröme, 2014-03-13 17:33:44]

Can you run the try bots so that we can see the log output of the tests?

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_renderer_unittest.cc (right):

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:42: MockAudioOutputIPC*
ipc,
can we make this scoped_ptr<media::AudioOutputIPC> ipc instead?

Basically have this constructor exactly like the AudioOutputDevice constructor
and use .Pass() to pass ownership.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:61: virtual
media::AudioOutputDevice* CreateOutputDevice(
It would be good to also make this class a mock class and just mock these
methods:

MOCK_METHOD1(CreateOutputDevice, media::AudioOutputDevice*(int));

Then in the tests we're free to return whatever object we want and don't have to
pass it to the mock class.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:71:
scoped_refptr<media::AudioOutputDevice> output_device_;
then we also don't need member variables here

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:97:
EXPECT_CALL(*mock_output_device_, Start());
would it make sense to extract this part into a separate method that is then
called from the tests?  As is, it's not clear from looking only at the tests
that Start() has been called, so seeing just a Stop() method looks strange.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:100:
renderer_proxy_->Start();
Maybe it's enough to just remove this line and put it into the tests instead.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:103:
scoped_ptr<base::MessageLoopForIO> message_loop_;
add a comment here that explains that this variable must be at the top, since it
needs to be constructed before the others.

[no longer working on chromium, 2014-03-14 09:41:59]

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_renderer_unittest.cc (right):

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:42: MockAudioOutputIPC*
ipc,
On 2014/03/13 17:33:44, tommi wrote:
> can we make this scoped_ptr<media::AudioOutputIPC> ipc instead?
> 
> Basically have this constructor exactly like the AudioOutputDevice constructor
> and use .Pass() to pass ownership.

Done.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:61: virtual
media::AudioOutputDevice* CreateOutputDevice(
On 2014/03/13 17:33:44, tommi wrote:
> It would be good to also make this class a mock class and just mock these
> methods:
> 
> MOCK_METHOD1(CreateOutputDevice, media::AudioOutputDevice*(int));
> 
> Then in the tests we're free to return whatever object we want and don't have
to
> pass it to the mock class.

Done.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:71:
scoped_refptr<media::AudioOutputDevice> output_device_;
On 2014/03/13 17:33:44, tommi wrote:
> then we also don't need member variables here

Done.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:97:
EXPECT_CALL(*mock_output_device_, Start());
On 2014/03/13 17:33:44, tommi wrote:
> would it make sense to extract this part into a separate method that is then
> called from the tests?  As is, it's not clear from looking only at the tests
> that Start() has been called, so seeing just a Stop() method looks strange.

Done with moving renderer_proxy_->Start(); to the test cases.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:100:
renderer_proxy_->Start();
On 2014/03/13 17:33:44, tommi wrote:
> Maybe it's enough to just remove this line and put it into the tests instead.

Done.

https://codereview.chromium.org/199063002/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_audio_renderer_unittest.cc:103:
scoped_ptr<base::MessageLoopForIO> message_loop_;
On 2014/03/13 17:33:44, tommi wrote:
> add a comment here that explains that this variable must be at the top, since
it
> needs to be constructed before the others.

Done with a comment to explain they are used to construct mock_output_device_

[tommi (sloooow) - chröme, 2014-03-14 14:34:25]

lgtm.  Great to have tests for this class finally :)

[no longer working on chromium, 2014-03-16 11:14:47]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-16 11:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/199063002/120001

[commit-bot: I haz the power, 2014-03-16 11:48:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-16 11:48:51]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[no longer working on chromium, 2014-03-16 19:47:30]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-16 19:47:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/199063002/140001

[commit-bot: I haz the power, 2014-03-16 21:16:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-16 21:16:04]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[no longer working on chromium, 2014-03-16 22:59:15]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-16 22:59:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/199063002/140001

[commit-bot: I haz the power, 2014-03-17 06:47:09]

Change committed as 257388