Bug 28310 - Chromium: Show a "Mute Disabled" button on audio error.
Summary: Chromium: Show a "Mute Disabled" button on audio error.
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Nobody
Depends on:
Reported: 2009-08-14 11:29 PDT by Kyle Prete
Modified: 2013-04-09 16:09 PDT (History)
1 user (show)

See Also:

Fix (3.85 KB, patch)
2009-08-14 11:37 PDT, Kyle Prete
eric: review-
Details | Formatted Diff | Diff
hasAudio() implemented for all ports (11.34 KB, patch)
2009-08-17 17:54 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Prete 2009-08-14 11:29:45 PDT
Load and paint a mediaSoundDisabled image in RenderThemeChromiumSkia::paintMediaMuteButton() to display when the mediaElement cannot load audio.
Comment 1 Kyle Prete 2009-08-14 11:37:55 PDT
Created attachment 34864 [details]

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 2 Eric Seidel (no email) 2009-08-14 15:14:18 PDT
Comment on attachment 34864 [details]

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.
Comment 3 Kyle Prete 2009-08-14 15:58:54 PDT
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?
Comment 4 Andrew Scherkus 2009-08-17 16:24:52 PDT
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.
Comment 5 Andrew Scherkus 2009-08-17 17:54:51 PDT
Created attachment 35005 [details]
hasAudio() implemented for all ports

Taking over the patch from kylep@chromium.org as he has returned to school
Comment 6 Andrew Scherkus 2009-08-19 17:06:10 PDT
Eric Carlson implemented hasAudio() in r47515.  This issue can be closed.
Comment 7 David Levin 2009-08-20 22:57:08 PDT
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.
Comment 8 Stephen Chenney 2013-04-09 16:09:37 PDT
LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.