Load and paint a mediaSoundDisabled image in RenderThemeChromiumSkia::paintMediaMuteButton() to display when the mediaElement cannot load audio.
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.
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.
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?
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.
Created attachment 35005 [details] hasAudio() implemented for all ports Taking over the patch from kylep@chromium.org as he has returned to school
Eric Carlson implemented hasAudio() in r47515. This issue can be closed.
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.
LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.