Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150191:
Unit tests for AudioRendererHost... (Closed)
|
Created:
11 years, 5 months ago by Alpha Left Google Modified:
9 years, 6 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review), brettw, Ben Goodger (Google) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionUnit tests for AudioRendererHost
TEST=AudioRendererHostTest.CreateStream
AudioRendererHostTest.MockStreamDataConversation
BUG=16035
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20422
Patch Set 1 #Patch Set 2 : '' #
Total comments: 51
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #Patch Set 6 : '' #Messages
Total messages: 8 (0 generated)
The basic structure of the unit test, I'm going to add more test cases to cover other operations of ARH.
http://codereview.chromium.org/150191/diff/11/13 File chrome/browser/renderer_host/audio_renderer_host.cc (right): http://codereview.chromium.org/150191/diff/11/13#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. this should be 2009 http://codereview.chromium.org/150191/diff/11/12 File chrome/browser/renderer_host/audio_renderer_host.h (right): http://codereview.chromium.org/150191/diff/11/12#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. this should be 2009 http://codereview.chromium.org/150191/diff/11/12#newcode266 Line 266: // required properties. See IPCAudioSource::CreateIPCAudioSource for more add () http://codereview.chromium.org/150191/diff/11/12#newcode272 Line 272: // call to the corresponding IPCAudioSource::Start. add () http://codereview.chromium.org/150191/diff/11/12#newcode279 Line 279: // corresponding IPCAudioSource::Pause, ViewMsg_NotifyAudioStreamStateChanged add () http://codereview.chromium.org/150191/diff/11/12#newcode285 Line 285: // corresponding IPCAudioSource::Close, no returning IPC message to renderer add () http://codereview.chromium.org/150191/diff/11/12#newcode289 Line 289: // Set the volume for the stream specified. Delegates the SetVolume method add () http://codereview.chromium.org/150191/diff/11/12#newcode298 Line 298: // IPCAudioSource::GetVolume, see the method for more details. add () http://codereview.chromium.org/150191/diff/11/12#newcode305 Line 305: // IPCAudioSource::NotifyPacketReady, see the method for more details. add () http://codereview.chromium.org/150191/diff/11/14 File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): http://codereview.chromium.org/150191/diff/11/14#newcode13 Line 13: #include "testing/gtest/include/gtest/gtest_prod.h" I don't think you need gtest_prod.h http://codereview.chromium.org/150191/diff/11/14#newcode59 Line 59: ASSERT_TRUE(message); you might want to use CHECK instead ASSERT_TRUE will fail the test, but also calls return which makes the test keep running (normally calling return inside the real unit test will make it stop) http://codereview.chromium.org/150191/diff/11/14#newcode76 Line 76: private: add blank line before private http://codereview.chromium.org/150191/diff/11/14#newcode88 Line 88: ASSERT_TRUE(shared_memory_->Map(length)); consider CHECK instead http://codereview.chromium.org/150191/diff/11/14#newcode89 Line 89: ASSERT_TRUE(shared_memory_->memory()); consider CHECK instead http://codereview.chromium.org/150191/diff/11/14#newcode107 Line 107: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/150191/diff/11/14#newcode120 Line 120: ASSERT_TRUE(host_); consider CHECK instead http://codereview.chromium.org/150191/diff/11/14#newcode140 Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, _)); who calls this method after creating? http://codereview.chromium.org/150191/diff/11/14#newcode173 Line 173: }; private: DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/150191/diff/11/14#newcode176 Line 176: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); scoped_ptr http://codereview.chromium.org/150191/diff/11/14#newcode182 Line 182: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); scoped_ptr http://codereview.chromium.org/150191/diff/11/14#newcode186 Line 186: // then there will no more packet requests. this comment doesn't seem to match the expectations... to me it looks like you're expecting: 16kb, 32kb, 48kb, 49kb, 50kb, 51kb, 52kb... http://codereview.chromium.org/150191/diff/11/14#newcode191 Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { I'd consider using "size" instead of "i" http://codereview.chromium.org/150191/diff/11/14#newcode198 Line 198: for (int i = 0; i < kPacketSize; i += 1024) { same here for "size" versus "i"
I am on vacation. Please go ahead with Andrew's comments. On 2009/07/02 00:21:22, scherkus wrote: > http://codereview.chromium.org/150191/diff/11/13 > File chrome/browser/renderer_host/audio_renderer_host.cc (right): > > http://codereview.chromium.org/150191/diff/11/13#newcode1 > Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. > this should be 2009 > > http://codereview.chromium.org/150191/diff/11/12 > File chrome/browser/renderer_host/audio_renderer_host.h (right): > > http://codereview.chromium.org/150191/diff/11/12#newcode1 > Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. > this should be 2009 > > http://codereview.chromium.org/150191/diff/11/12#newcode266 > Line 266: // required properties. See IPCAudioSource::CreateIPCAudioSource for > more > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode272 > Line 272: // call to the corresponding IPCAudioSource::Start. > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode279 > Line 279: // corresponding IPCAudioSource::Pause, > ViewMsg_NotifyAudioStreamStateChanged > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode285 > Line 285: // corresponding IPCAudioSource::Close, no returning IPC message to > renderer > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode289 > Line 289: // Set the volume for the stream specified. Delegates the SetVolume > method > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode298 > Line 298: // IPCAudioSource::GetVolume, see the method for more details. > add () > > http://codereview.chromium.org/150191/diff/11/12#newcode305 > Line 305: // IPCAudioSource::NotifyPacketReady, see the method for more details. > add () > > http://codereview.chromium.org/150191/diff/11/14 > File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): > > http://codereview.chromium.org/150191/diff/11/14#newcode13 > Line 13: #include "testing/gtest/include/gtest/gtest_prod.h" > I don't think you need gtest_prod.h > > http://codereview.chromium.org/150191/diff/11/14#newcode59 > Line 59: ASSERT_TRUE(message); > you might want to use CHECK instead > > ASSERT_TRUE will fail the test, but also calls return which makes the test keep > running (normally calling return inside the real unit test will make it stop) > > http://codereview.chromium.org/150191/diff/11/14#newcode76 > Line 76: private: > add blank line before private > > http://codereview.chromium.org/150191/diff/11/14#newcode88 > Line 88: ASSERT_TRUE(shared_memory_->Map(length)); > consider CHECK instead > > http://codereview.chromium.org/150191/diff/11/14#newcode89 > Line 89: ASSERT_TRUE(shared_memory_->memory()); > consider CHECK instead > > http://codereview.chromium.org/150191/diff/11/14#newcode107 > Line 107: }; > DISALLOW_COPY_AND_ASSIGN > > http://codereview.chromium.org/150191/diff/11/14#newcode120 > Line 120: ASSERT_TRUE(host_); > consider CHECK instead > > http://codereview.chromium.org/150191/diff/11/14#newcode140 > Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, > _)); > who calls this method after creating? > > http://codereview.chromium.org/150191/diff/11/14#newcode173 > Line 173: }; > private: > DISALLOW_COPY_AND_ASSIGN > > http://codereview.chromium.org/150191/diff/11/14#newcode176 > Line 176: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); > scoped_ptr > > http://codereview.chromium.org/150191/diff/11/14#newcode182 > Line 182: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); > scoped_ptr > > http://codereview.chromium.org/150191/diff/11/14#newcode186 > Line 186: // then there will no more packet requests. > this comment doesn't seem to match the expectations... > > to me it looks like you're expecting: > 16kb, 32kb, 48kb, 49kb, 50kb, 51kb, 52kb... > > http://codereview.chromium.org/150191/diff/11/14#newcode191 > Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { > I'd consider using "size" instead of "i" > > http://codereview.chromium.org/150191/diff/11/14#newcode198 > Line 198: for (int i = 0; i < kPacketSize; i += 1024) { > same here for "size" versus "i"
BTW, please take care of the lint errors :) On 2009/07/02 08:06:04, cpu wrote: > I am on vacation. Please go ahead with Andrew's comments. > > On 2009/07/02 00:21:22, scherkus wrote: > > http://codereview.chromium.org/150191/diff/11/13 > > File chrome/browser/renderer_host/audio_renderer_host.cc (right): > > > > http://codereview.chromium.org/150191/diff/11/13#newcode1 > > Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. > > this should be 2009 > > > > http://codereview.chromium.org/150191/diff/11/12 > > File chrome/browser/renderer_host/audio_renderer_host.h (right): > > > > http://codereview.chromium.org/150191/diff/11/12#newcode1 > > Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. > > this should be 2009 > > > > http://codereview.chromium.org/150191/diff/11/12#newcode266 > > Line 266: // required properties. See IPCAudioSource::CreateIPCAudioSource for > > more > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode272 > > Line 272: // call to the corresponding IPCAudioSource::Start. > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode279 > > Line 279: // corresponding IPCAudioSource::Pause, > > ViewMsg_NotifyAudioStreamStateChanged > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode285 > > Line 285: // corresponding IPCAudioSource::Close, no returning IPC message to > > renderer > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode289 > > Line 289: // Set the volume for the stream specified. Delegates the SetVolume > > method > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode298 > > Line 298: // IPCAudioSource::GetVolume, see the method for more details. > > add () > > > > http://codereview.chromium.org/150191/diff/11/12#newcode305 > > Line 305: // IPCAudioSource::NotifyPacketReady, see the method for more > details. > > add () > > > > http://codereview.chromium.org/150191/diff/11/14 > > File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): > > > > http://codereview.chromium.org/150191/diff/11/14#newcode13 > > Line 13: #include "testing/gtest/include/gtest/gtest_prod.h" > > I don't think you need gtest_prod.h > > > > http://codereview.chromium.org/150191/diff/11/14#newcode59 > > Line 59: ASSERT_TRUE(message); > > you might want to use CHECK instead > > > > ASSERT_TRUE will fail the test, but also calls return which makes the test > keep > > running (normally calling return inside the real unit test will make it stop) > > > > http://codereview.chromium.org/150191/diff/11/14#newcode76 > > Line 76: private: > > add blank line before private > > > > http://codereview.chromium.org/150191/diff/11/14#newcode88 > > Line 88: ASSERT_TRUE(shared_memory_->Map(length)); > > consider CHECK instead > > > > http://codereview.chromium.org/150191/diff/11/14#newcode89 > > Line 89: ASSERT_TRUE(shared_memory_->memory()); > > consider CHECK instead > > > > http://codereview.chromium.org/150191/diff/11/14#newcode107 > > Line 107: }; > > DISALLOW_COPY_AND_ASSIGN > > > > http://codereview.chromium.org/150191/diff/11/14#newcode120 > > Line 120: ASSERT_TRUE(host_); > > consider CHECK instead > > > > http://codereview.chromium.org/150191/diff/11/14#newcode140 > > Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, > > _)); > > who calls this method after creating? > > > > http://codereview.chromium.org/150191/diff/11/14#newcode173 > > Line 173: }; > > private: > > DISALLOW_COPY_AND_ASSIGN > > > > http://codereview.chromium.org/150191/diff/11/14#newcode176 > > Line 176: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); > > scoped_ptr > > > > http://codereview.chromium.org/150191/diff/11/14#newcode182 > > Line 182: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); > > scoped_ptr > > > > http://codereview.chromium.org/150191/diff/11/14#newcode186 > > Line 186: // then there will no more packet requests. > > this comment doesn't seem to match the expectations... > > > > to me it looks like you're expecting: > > 16kb, 32kb, 48kb, 49kb, 50kb, 51kb, 52kb... > > > > http://codereview.chromium.org/150191/diff/11/14#newcode191 > > Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { > > I'd consider using "size" instead of "i" > > > > http://codereview.chromium.org/150191/diff/11/14#newcode198 > > Line 198: for (int i = 0; i < kPacketSize; i += 1024) { > > same here for "size" versus "i"
http://codereview.chromium.org/150191/diff/11/13 File chrome/browser/renderer_host/audio_renderer_host.cc (right): http://codereview.chromium.org/150191/diff/11/13#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. On 2009/07/02 00:21:22, scherkus wrote: > this should be 2009 Done. http://codereview.chromium.org/150191/diff/11/12 File chrome/browser/renderer_host/audio_renderer_host.h (right): http://codereview.chromium.org/150191/diff/11/12#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. On 2009/07/02 00:21:22, scherkus wrote: > this should be 2009 Done. http://codereview.chromium.org/150191/diff/11/12#newcode266 Line 266: // required properties. See IPCAudioSource::CreateIPCAudioSource for more On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/12#newcode272 Line 272: // call to the corresponding IPCAudioSource::Start. On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/12#newcode279 Line 279: // corresponding IPCAudioSource::Pause, ViewMsg_NotifyAudioStreamStateChanged On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/12#newcode285 Line 285: // corresponding IPCAudioSource::Close, no returning IPC message to renderer On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/12#newcode298 Line 298: // IPCAudioSource::GetVolume, see the method for more details. On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/12#newcode305 Line 305: // IPCAudioSource::NotifyPacketReady, see the method for more details. On 2009/07/02 00:21:22, scherkus wrote: > add () Done. http://codereview.chromium.org/150191/diff/11/14 File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): http://codereview.chromium.org/150191/diff/11/14#newcode13 Line 13: #include "testing/gtest/include/gtest/gtest_prod.h" On 2009/07/02 00:21:22, scherkus wrote: > I don't think you need gtest_prod.h Done. http://codereview.chromium.org/150191/diff/11/14#newcode59 Line 59: ASSERT_TRUE(message); On 2009/07/02 00:21:22, scherkus wrote: > you might want to use CHECK instead > > ASSERT_TRUE will fail the test, but also calls return which makes the test keep > running (normally calling return inside the real unit test will make it stop) Done. http://codereview.chromium.org/150191/diff/11/14#newcode76 Line 76: private: On 2009/07/02 00:21:22, scherkus wrote: > add blank line before private Done. http://codereview.chromium.org/150191/diff/11/14#newcode88 Line 88: ASSERT_TRUE(shared_memory_->Map(length)); On 2009/07/02 00:21:22, scherkus wrote: > consider CHECK instead Done. http://codereview.chromium.org/150191/diff/11/14#newcode89 Line 89: ASSERT_TRUE(shared_memory_->memory()); On 2009/07/02 00:21:22, scherkus wrote: > consider CHECK instead Done. http://codereview.chromium.org/150191/diff/11/14#newcode107 Line 107: }; On 2009/07/02 00:21:22, scherkus wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/150191/diff/11/14#newcode120 Line 120: ASSERT_TRUE(host_); On 2009/07/02 00:21:22, scherkus wrote: > consider CHECK instead Done. http://codereview.chromium.org/150191/diff/11/14#newcode140 Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, _)); On 2009/07/02 00:21:22, scherkus wrote: > who calls this method after creating? IPCAudioSource::CreateIPCAudioSource kick start the first packet request. http://codereview.chromium.org/150191/diff/11/14#newcode173 Line 173: }; On 2009/07/02 00:21:22, scherkus wrote: > private: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/150191/diff/11/14#newcode176 Line 176: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); On 2009/07/02 00:21:22, scherkus wrote: > scoped_ptr Done. http://codereview.chromium.org/150191/diff/11/14#newcode182 Line 182: AudioRendererHost::IPCAudioSource* source = CreateMockStream(); On 2009/07/02 00:21:22, scherkus wrote: > scoped_ptr Done. http://codereview.chromium.org/150191/diff/11/14#newcode186 Line 186: // then there will no more packet requests. On 2009/07/02 00:21:22, scherkus wrote: > this comment doesn't seem to match the expectations... > > to me it looks like you're expecting: > 16kb, 32kb, 48kb, 49kb, 50kb, 51kb, 52kb... Done. http://codereview.chromium.org/150191/diff/11/14#newcode191 Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { On 2009/07/02 00:21:22, scherkus wrote: > I'd consider using "size" instead of "i" Done. http://codereview.chromium.org/150191/diff/11/14#newcode198 Line 198: for (int i = 0; i < kPacketSize; i += 1024) { On 2009/07/02 00:21:22, scherkus wrote: > same here for "size" versus "i" Done.
don't forget to add BUG=16035 http://codereview.chromium.org/150191/diff/11/14 File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): http://codereview.chromium.org/150191/diff/11/14#newcode140 Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, _)); On 2009/07/03 01:24:21, Alpha wrote: > On 2009/07/02 00:21:22, scherkus wrote: > > who calls this method after creating? > > IPCAudioSource::CreateIPCAudioSource kick start the first packet request. ok I'd add that to the comment http://codereview.chromium.org/150191/diff/11/14#newcode191 Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { On 2009/07/03 01:24:21, Alpha wrote: > On 2009/07/02 00:21:22, scherkus wrote: > > I'd consider using "size" instead of "i" > > Done. Sorry... I meant the variable name: "int size = 3" "i" is usually for iterators, but here you're actually calculating a size http://codereview.chromium.org/150191/diff/11/14#newcode198 Line 198: for (int i = 0; i < kPacketSize; i += 1024) { On 2009/07/03 01:24:21, Alpha wrote: > On 2009/07/02 00:21:22, scherkus wrote: > > same here for "size" versus "i" > > Done. same here http://codereview.chromium.org/150191/diff/2006/2009#newcode136 Line 136: EXPECT_CALL(*host_, OnStreamCreated -> OnStreamCreated()
http://codereview.chromium.org/150191/diff/11/14 File chrome/browser/renderer_host/audio_renderer_host_unittest.cc (right): http://codereview.chromium.org/150191/diff/11/14#newcode140 Line 140: EXPECT_CALL(*host_, OnRequestPacket(kRouteId, current_stream_id_, 0, _)); On 2009/07/06 20:02:57, scherkus wrote: > On 2009/07/03 01:24:21, Alpha wrote: > > On 2009/07/02 00:21:22, scherkus wrote: > > > who calls this method after creating? > > > > IPCAudioSource::CreateIPCAudioSource kick start the first packet request. > > ok I'd add that to the comment Done. http://codereview.chromium.org/150191/diff/11/14#newcode191 Line 191: for (int i = 3 * kPacketSize; i < kBufferCapacity; i += 1024) { On 2009/07/06 20:02:57, scherkus wrote: > On 2009/07/03 01:24:21, Alpha wrote: > > On 2009/07/02 00:21:22, scherkus wrote: > > > I'd consider using "size" instead of "i" > > > > Done. > > Sorry... I meant the variable name: "int size = 3" > > "i" is usually for iterators, but here you're actually calculating a size Done. http://codereview.chromium.org/150191/diff/11/14#newcode198 Line 198: for (int i = 0; i < kPacketSize; i += 1024) { On 2009/07/06 20:02:57, scherkus wrote: > On 2009/07/03 01:24:21, Alpha wrote: > > On 2009/07/02 00:21:22, scherkus wrote: > > > same here for "size" versus "i" > > > > Done. > > same here Done.
lgtm! |
