Some fixups for AudioController and unit tests

Some fixups for AudioController and unit tests

Some fixes to AudioController and more unit tests. AudioController is
now ready to be used by AudioRendererHost.

BUG=39885
TEST=media_unittests

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

[awong, 2010-06-18 18:42:14]

http://codereview.chromium.org/2861010/diff/1/2
File media/audio/audio_controller.cc (right):

http://codereview.chromium.org/2861010/diff/1/2#newcode181
media/audio/audio_controller.cc:181: if (!LowLatencyMode()) {
I don't exactly understand what this is doing, but I assume you you do and are
sure it won't block DoCreate or something?

http://codereview.chromium.org/2861010/diff/1/4
File media/audio/audio_controller_unittest.cc (right):

http://codereview.chromium.org/2861010/diff/1/4#newcode205
media/audio/audio_controller_unittest.cc:205: TEST(AudioControllerTest,
CloseTwice) {
Why is this a valid test?  Please add a comment explaning why we bother testing
this.

Is there a bug in our close logic?  Or do we really want the API contract to be
okay with us calling close twice? If so, this should be documented in the
function description.

[Alpha Left Google, 2010-06-18 18:48:50]

http://codereview.chromium.org/2861010/diff/1/2
File media/audio/audio_controller.cc (right):

http://codereview.chromium.org/2861010/diff/1/2#newcode181
media/audio/audio_controller.cc:181: if (!LowLatencyMode()) {
On 2010/06/18 18:42:14, awong wrote:
> I don't exactly understand what this is doing, but I assume you you do and are
> sure it won't block DoCreate or something?

This won't block DoCreate, SubmitOnMoreData_Locked() is a short method that
sends signal to |handler_|.

http://codereview.chromium.org/2861010/diff/1/4
File media/audio/audio_controller_unittest.cc (right):

http://codereview.chromium.org/2861010/diff/1/4#newcode205
media/audio/audio_controller_unittest.cc:205: TEST(AudioControllerTest,
CloseTwice) {
On 2010/06/18 18:42:14, awong wrote:
> Why is this a valid test?  Please add a comment explaning why we bother
testing
> this.
> 
> Is there a bug in our close logic?  Or do we really want the API contract to
be
> okay with us calling close twice? If so, this should be documented in the
> function description.

Not really. Just that the API allows we call Close() more than once.

[awong, 2010-06-18 18:56:24]

On Fri, Jun 18, 2010 at 11:48 AM, <hclam@chromium.org> wrote:

>
> http://codereview.chromium.org/2861010/diff/1/2
> File media/audio/audio_controller.cc (right):
>
> http://codereview.chromium.org/2861010/diff/1/2#newcode181
> media/audio/audio_controller.cc:181: if (!LowLatencyMode()) {
> On 2010/06/18 18:42:14, awong wrote:
>
>> I don't exactly understand what this is doing, but I assume you you do
>>
> and are
>
>> sure it won't block DoCreate or something?
>>
>
> This won't block DoCreate, SubmitOnMoreData_Locked() is a short method
> that sends signal to |handler_|.
>
>
> http://codereview.chromium.org/2861010/diff/1/4
> File media/audio/audio_controller_unittest.cc (right):
>
> http://codereview.chromium.org/2861010/diff/1/4#newcode205
> media/audio/audio_controller_unittest.cc:205: TEST(AudioControllerTest,
> CloseTwice) {
> On 2010/06/18 18:42:14, awong wrote:
>
>> Why is this a valid test?  Please add a comment explaning why we
>>
> bother testing
>
>> this.
>>
>
>  Is there a bug in our close logic?  Or do we really want the API
>>
> contract to be
>
>> okay with us calling close twice? If so, this should be documented in
>>
> the
>
>> function description.
>>
>
> Not really. Just that the API allows we call Close() more than once.


Document!


>
>
> http://codereview.chromium.org/2861010/show
>

[Alpha Left Google, 2010-06-18 18:58:47]

On 2010/06/18 18:56:24, awong wrote:
> On Fri, Jun 18, 2010 at 11:48 AM, <mailto:hclam@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/2861010/diff/1/2
> > File media/audio/audio_controller.cc (right):
> >
> > http://codereview.chromium.org/2861010/diff/1/2#newcode181
> > media/audio/audio_controller.cc:181: if (!LowLatencyMode()) {
> > On 2010/06/18 18:42:14, awong wrote:
> >
> >> I don't exactly understand what this is doing, but I assume you you do
> >>
> > and are
> >
> >> sure it won't block DoCreate or something?
> >>
> >
> > This won't block DoCreate, SubmitOnMoreData_Locked() is a short method
> > that sends signal to |handler_|.
> >
> >
> > http://codereview.chromium.org/2861010/diff/1/4
> > File media/audio/audio_controller_unittest.cc (right):
> >
> > http://codereview.chromium.org/2861010/diff/1/4#newcode205
> > media/audio/audio_controller_unittest.cc:205: TEST(AudioControllerTest,
> > CloseTwice) {
> > On 2010/06/18 18:42:14, awong wrote:
> >
> >> Why is this a valid test?  Please add a comment explaning why we
> >>
> > bother testing
> >
> >> this.
> >>
> >
> >  Is there a bug in our close logic?  Or do we really want the API
> >>
> > contract to be
> >
> >> okay with us calling close twice? If so, this should be documented in
> >>
> > the
> >
> >> function description.
> >>
> >
> > Not really. Just that the API allows we call Close() more than once.
> 
> 
> Document!
> 
> 
> >
> >
> > http://codereview.chromium.org/2861010/show
> >
> 

Already put in the .h file. Forgot to mention in last email.

[awong, 2010-06-18 19:00:39]

LGTM

[scherkus (not reviewing), 2010-06-21 18:05:48]

LGTM