Make initial reset period configurable for audio input no data timer.

Make initial reset period configurable for audio input no data timer.

iOS needs extra initial delay in the reset timer to avoid getting
killed because there is a background music fading out.

BUG=b/6754065


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

[Milan Broum, 2012-09-04 14:56:52]

Part of iOS upstreaming: a more flexible reset timer is needed.

Stuart for the initial review, Henrik for the final LGTM.

[henrika (OOO until Aug 14), 2012-09-04 15:14:41]

Milan,

could you please add allanwoj who initially added the timer parts in this class
(see http://codereview.chromium.org/7129057 for details). I guess it is
essential to confirm that the original case is still covered after your patch.

From my part (WebRTC), do run the WebRTC unit tests in content_unittests to
verify that they are OK as well with your patch. If they are, I am basically
fine.

Finally, I am not an owner and can't give the final OK.

[Milan Broum, 2012-09-04 15:32:03]

On 2012/09/04 15:14:41, henrika wrote:
> Milan,
> 
> could you please add allanwoj who initially added the timer parts in this
class
> (see http://codereview.chromium.org/7129057 for details). I guess it is
> essential to confirm that the original case is still covered after your patch.
> 
> From my part (WebRTC), do run the WebRTC unit tests in content_unittests to
> verify that they are OK as well with your patch. If they are, I am basically
> fine.
> 
> Finally, I am not an owner and can't give the final OK.

Thanks for the pointer to the owner, I checked the latest file history only.

The functionality should be exactly the same so there should be no harm for
WebRTC tests. They are run as part of trybot, right? I cannot verify for all
platforms.

[Milan Broum, 2012-09-04 15:33:22]

Adding Allan as the owner.

[Milan Broum, 2012-09-04 16:28:20]

Adding media/OWNERS

[allanwoj, 2012-09-04 16:40:27]

On 2012/09/04 15:33:22, Milan Broum wrote:
> Adding Allan as the owner.

Hi Milan,

I'm not an owner of this directory so can't give a final ok. The changes don't
seem to affect coverage for the old bug but I can't test it as I don't have a
Windows station. scherkus@chromium.org is probably your best bet.

Cheers,
Allan

[henrika (OOO until Aug 14), 2012-09-04 17:18:38]

Milan,

the WebRTC test requires hardware to run fully. They are more or less
excluded on bots without hardware. Instead we have our own internal audio
bots as a complement. Running content_unittests locally on your own machine
is a good start though.

I can give your CL a spin on my Windows machine tomorrow and get back with
some test results.


On Tue, Sep 4, 2012 at 6:40 PM, <allanwoj@chromium.org> wrote:

> On 2012/09/04 15:33:22, Milan Broum wrote:
>
>> Adding Allan as the owner.
>>
>
> Hi Milan,
>
> I'm not an owner of this directory so can't give a final ok. The changes
> don't
> seem to affect coverage for the old bug but I can't test it as I don't
> have a
> Windows station. scherkus@chromium.org is probably your best bet.
>
> Cheers,
> Allan
>
>
http://codereview.chromium.**org/10911067/<http://codereview.chromium.org/109...
>

[Milan Broum, 2012-09-04 22:09:06]

On 2012/09/04 16:40:27, allanwoj wrote:
> On 2012/09/04 15:33:22, Milan Broum wrote:
> > Adding Allan as the owner.
> 
> Hi Milan,
> 
> I'm not an owner of this directory so can't give a final ok. The changes don't
> seem to affect coverage for the old bug but I can't test it as I don't have a
> Windows station. mailto:scherkus@chromium.org is probably your best bet.
> 
> Cheers,
> Allan

Thanks for suggestion. I'll change the main reviewer.

[Milan Broum, 2012-09-04 22:13:27]

On 2012/09/04 17:18:38, henrika wrote:
> Milan,
> 
> the WebRTC test requires hardware to run fully. They are more or less
> excluded on bots without hardware. Instead we have our own internal audio
> bots as a complement. Running content_unittests locally on your own machine
> is a good start though.
> 
> I can give your CL a spin on my Windows machine tomorrow and get back with
> some test results.
> 
> 
> On Tue, Sep 4, 2012 at 6:40 PM, <mailto:allanwoj@chromium.org> wrote:
> 
> > On 2012/09/04 15:33:22, Milan Broum wrote:
> >
> >> Adding Allan as the owner.
> >>
> >
> > Hi Milan,
> >
> > I'm not an owner of this directory so can't give a final ok. The changes
> > don't
> > seem to affect coverage for the old bug but I can't test it as I don't
> > have a
> > Windows station. mailto:scherkus@chromium.org is probably your best bet.
> >
> > Cheers,
> > Allan
> >
> >
>
http://codereview.chromium.**org/10911067/%3Chttp://codereview.chromium.org/1...>
> >

Henrik,

If you could do it for me and test it on windows HW that would be great. I do
not have windows machine available. I can try mac/linux tests on top of ios
tests if that would be helpful. I see what I can do tomorrow.

Cheers,
 Milan

[Milan Broum, 2012-09-04 22:14:38]

-allanwoj -fischman
+scherkus

I hope I got it right, now.

[henrika (OOO until Aug 14), 2012-09-05 10:57:14]

Tried content_unittests.exe --gtest_filter=WebRTC* with this patch for Rev.
154929 locally on my Win7 machine and all tests passed.

Also tried media_unittests. OK there as well.

LGTM from my side.

[scherkus (not reviewing), 2012-09-05 15:46:43]

LGTM w/ nits

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:152:
base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
hrmmm the indentation style was incorrect prior to your change -- mind fixing it
up?

no_data_timer_.reset(new base::Timer(
    FROM_HERE, base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
    &AudioInputController::DoCheckForNoData, base::Unretained(this)), false));

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:255: no_data_timer_->Start(FROM_HERE,
ditto

[Milan Broum, 2012-09-05 17:12:03]

thanks

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:152:
base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
On 2012/09/05 15:46:43, scherkus wrote:
> hrmmm the indentation style was incorrect prior to your change -- mind fixing
it
> up?
> 
> no_data_timer_.reset(new base::Timer(
>     FROM_HERE, base::TimeDelta::FromSeconds(kTimerInitialIntervalSeconds),
>     &AudioInputController::DoCheckForNoData, base::Unretained(this)), false));

Done. Your suggestion exceeded 80 chars, split it to three lines instead.

https://chromiumcodereview.appspot.com/10911067/diff/1/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:255: no_data_timer_->Start(FROM_HERE,
On 2012/09/05 15:46:43, scherkus wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-09-05 17:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/milanb@chromium.org/10911067/10001

[commit-bot: I haz the power, 2012-09-06 01:07:38]

Change committed as 155082