Silence detection test for the Windows audio capturer.

Silence detection test for the Windows audio capturer.


BUG=


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

[kxing, 2012-08-21 20:22:55]

Could someone PTAL?

[Sergey Ulanov, 2012-08-21 20:52:15]

mostly style nits

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:280: // static
usually that comments goes directly in front of the function, e.g.:
// <method docs>
// static
bool Foo::Bar() {
...
}

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win_unittest.cc (right):

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
move this include after basictypes.h

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:12: class AudioCapturerWinTest :
public testing::Test {
Don't really need this class. It's necessary only when you share code between
multiple tests. Remove it and use TEST() instead of TEST_F()

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:16: int16 samples1[] = {0, 0, 0, 0,
0, 0, 0, 0, 0, 0};
Maybe add a test for an empty buffer?

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:20:
ASSERT_TRUE(AudioCapturerWin::IsPacketOfSilence(samples1, 10));
here and below: s/10/arraysize(samples1)/

[kxing, 2012-08-21 21:24:27]

Could someone PTAL again?

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win.cc:280: // static
On 2012/08/21 20:52:15, sergeyu wrote:
> usually that comments goes directly in front of the function, e.g.:
> // <method docs>
> // static
> bool Foo::Bar() {
> ...
> }

Done.

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
File remoting/host/audio_capturer_win_unittest.cc (right):

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
On 2012/08/21 20:52:15, sergeyu wrote:
> move this include after basictypes.h

Done.

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:12: class AudioCapturerWinTest :
public testing::Test {
On 2012/08/21 20:52:15, sergeyu wrote:
> Don't really need this class. It's necessary only when you share code between
> multiple tests. Remove it and use TEST() instead of TEST_F()

Done.

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:16: int16 samples1[] = {0, 0, 0, 0,
0, 0, 0, 0, 0, 0};
On 2012/08/21 20:52:15, sergeyu wrote:
> Maybe add a test for an empty buffer?

Done.

http://codereview.chromium.org/10823440/diff/1/remoting/host/audio_capturer_w...
remoting/host/audio_capturer_win_unittest.cc:20:
ASSERT_TRUE(AudioCapturerWin::IsPacketOfSilence(samples1, 10));
On 2012/08/21 20:52:15, sergeyu wrote:
> here and below: s/10/arraysize(samples1)/

Done.

[Sergey Ulanov, 2012-08-21 21:34:39]

lgtm, but please see my nits.

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:8: #include <audioclient.h>
Don't need to include files that are already included in audio_capturer_win.h.

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.h (right):

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.h:10: #include <mmsystem.h>
Do we need all of these includes in the header?

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.h:14: #include "base/message_loop.h"
Don't need this include here.

[kxing, 2012-08-21 21:55:53]

I'll commit this soon, unless someone objects.

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.cc:8: #include <audioclient.h>
On 2012/08/21 21:34:39, sergeyu wrote:
> Don't need to include files that are already included in audio_capturer_win.h.

Done.

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
File remoting/host/audio_capturer_win.h (right):

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.h:10: #include <mmsystem.h>
On 2012/08/21 21:34:39, sergeyu wrote:
> Do we need all of these includes in the header?

Done.

http://codereview.chromium.org/10823440/diff/5001/remoting/host/audio_capture...
remoting/host/audio_capturer_win.h:14: #include "base/message_loop.h"
On 2012/08/21 21:34:39, sergeyu wrote:
> Don't need this include here.

Done.

[Sergey Ulanov, 2012-08-21 22:05:46]

http://codereview.chromium.org/10823440/diff/11002/remoting/host/audio_captur...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/11002/remoting/host/audio_captur...
remoting/host/audio_capturer_win.cc:16: #include "base/message_loop.h"
This is not used anywhere.

[kxing, 2012-08-21 22:17:08]

I'll commit this soon.

http://codereview.chromium.org/10823440/diff/11002/remoting/host/audio_captur...
File remoting/host/audio_capturer_win.cc (right):

http://codereview.chromium.org/10823440/diff/11002/remoting/host/audio_captur...
remoting/host/audio_capturer_win.cc:16: #include "base/message_loop.h"
On 2012/08/21 22:05:46, sergeyu wrote:
> This is not used anywhere.

Done.

[commit-bot: I haz the power, 2012-08-21 22:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10823440/8002

[commit-bot: I haz the power, 2012-08-22 00:56:57]

Change committed as 152704