Framework to display disabled mute button when audio fails....

Framework to display disabled mute button when audio fails.
BUG=18970
TEST=play a video with no audio hardware, verify that UI changes

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

[kylep, 2009-08-14 20:31:29]

I'm trying to test this but my chrome refuses to build. The only issue might be
it not repainting the button if audio does not fail right away.

[awong, 2009-08-14 20:33:10]

LGTM

One comment.

Thanks for doing this so fast!

http://codereview.chromium.org/169010/diff/1003/9
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/169010/diff/1003/9#newcode692
Line 692: // that audio is not and will not be working.
File a bug about this and put the bug number into the comment.  Place on
MStone:4.

[scherkus (not reviewing), 2009-08-14 20:34:44]

LGTM assuming it compiles ;)

http://codereview.chromium.org/169010/diff/1003/9
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/169010/diff/1003/9#newcode694
Line 694: rendered_mime_types_.erase(mime_type::kMajorTypeAudio);
is this valid?

I think erase takes an iterator, but I could be wrong... I always forget which
operations take iterator vs. value

[kylep, 2009-08-14 20:48:02]

We have 1 trybot success so far. My build problem has to do with gclient
misbehaving, I think.

http://codereview.chromium.org/169010/diff/1003/9
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/169010/diff/1003/9#newcode692
Line 692: // that audio is not and will not be working.
On 2009/08/14 20:33:10, awong wrote:
> File a bug about this and put the bug number into the comment.  Place on
> MStone:4.

Bug filed: http://crbug.com/19384

http://codereview.chromium.org/169010/diff/1003/9#newcode694
Line 694: rendered_mime_types_.erase(mime_type::kMajorTypeAudio);
On 2009/08/14 20:34:44, scherkus wrote:
> is this valid?
> 
> I think erase takes an iterator, but I could be wrong... I always forget which
> operations take iterator vs. value

Yeah, I looked it up. It takes either an iterator or the element you want to
erase.

[scherkus (not reviewing), 2009-08-14 22:28:17]

http://codereview.chromium.org/169010/diff/1003/9
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/169010/diff/1003/9#newcode694
Line 694: rendered_mime_types_.erase(mime_type::kMajorTypeAudio);
On 2009/08/14 20:48:02, kylep wrote:
> On 2009/08/14 20:34:44, scherkus wrote:
> > is this valid?
> > 
> > I think erase takes an iterator, but I could be wrong... I always forget
which
> > operations take iterator vs. value
> 
> Yeah, I looked it up. It takes either an iterator or the element you want to
> erase.

What if the element doesn't exist?  I have this feeling STL will explode.  Maybe
write a unit test ;)

[kylep, 2009-08-14 22:52:32]

On Fri, Aug 14, 2009 at 3:28 PM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.org/169010/diff/1003/9
> File media/base/pipeline_impl.cc (right):
>
> http://codereview.chromium.org/169010/diff/1003/9#newcode694
> Line 694: rendered_mime_types_.erase(mime_type::kMajorTypeAudio);
> On 2009/08/14 20:48:02, kylep wrote:
>
>> On 2009/08/14 20:34:44, scherkus wrote:
>> > is this valid?
>> >
>> > I think erase takes an iterator, but I could be wrong... I always
>>
> forget which
>
>> > operations take iterator vs. value
>>
>
>  Yeah, I looked it up. It takes either an iterator or the element you
>>
> want to
>
>> erase.
>>
>
> What if the element doesn't exist?  I have this feeling STL will
> explode.  Maybe write a unit test ;)
>
>
> http://codereview.chromium.org/169010
>

Nah, it just returns the number of elements it deleted. So if it cant find
it, it returns 0. If it can, it erases it and returns 1.

[scherkus (not reviewing), 2009-08-14 23:01:29]

awesome lgtm!