Bug 28310 – Chromium: Show a "Mute Disabled" button on audio error.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Kyle Prete]

Load and paint a mediaSoundDisabled image in RenderThemeChromiumSkia::paintMediaMuteButton() to display when the mediaElement cannot load audio.

[Kyle Prete]

Created attachment 34864 [details]
Fix

Add methods to ask the mediaplayer if it has audio (by default returning true) and modify chromium theme renderer to paint the disabled button.

[Eric Seidel (no email)]

Comment on attachment 34864 [details]
Fix

I'm not a media expert, but this patch looks wrong.  Mostly because the methods look out of place:

One of these does not look like the other:
     virtual bool isVideo() const { return false; }
     virtual bool hasVideo() const { return false; }
+    virtual bool hasAudio() const { return player() && player()->hasAudio(); }

And again:
     virtual bool hasVideo() const = 0;
+    virtual bool hasAudio() const { return true; }

It seems maybe you're not following the design of the rest of the media classes?

Please explain.

[Kyle Prete]

Sure. The issue is that hasAudio is being used differently than hasVideo is. hasVideo() returns true if the video has loaded. It could return false if the video isn't loaded yet, but might be loaded later. hasAudio() returns false if the audio will never load, i.e. the hardware doesn't work or (maybe) the source has no audio. It's probably more a naming issue. Should I call it hasAudioError and flip the default return?

[Andrew Scherkus]

Kyle was my intern for the summer, I'll be taking over this patch.

To help explain the out-of-placeness, HTMLMediaElement is a base class to HTMLVideoElement and HTMLAudioElement.  MediaPlayerPrivateInterface is a base class to the different platform ports.

For HTMLMediaElement, we can probably implement the same method in both HTMLAudioElement and HTMLVideoElement, since and HTMLMediaElement itself cannot really exist and would never have a player associated with it.

For MediaPlayerPrivateInterface, Kyle chose to keep the existing behaviour (audio is assumed to always be present) without being forced to implement the actual behaviour for every port.  If it is preferred, we can have hasAudio() be pure virtual and implement it as "notImplemented(); return true;" for the ports we are unfamiliar with.

[Andrew Scherkus]

Created attachment 35005 [details]
hasAudio() implemented for all ports

Taking over the patch from [e-mail] as he has returned to school

[Andrew Scherkus]

Eric Carlson implemented hasAudio() in r47515.  This issue can be closed.

[David Levin]

Comment on attachment 35005 [details]
hasAudio() implemented for all ports

Obsoleting (and removing r?) patch to move out of review queue per comments in
the bug.

[Stephen Chenney]

LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.