Bug 50228 – Theme not updated when MediaPlayer m_private engine changes

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

RenderTheme relies on the mediaElement supportsFullscreen and supportMuting to know wether it should paint the fullscreen and mute buttons or not. The problem is that the MediaPlayer first loads a NullMediaPlayerPrivate which doesn't support those. So if you display a page with a video element and controls enabled (no autoplay), the theme will be rendered without those buttons until video playback starts and the controls are painted again.

So I think we could have a mediaPlayerBackendChanged() callback in the mediaPlayerClient interface which in the case of the HTMLMediaElement trigger an update of the theme. Would that make sense, Eric? The new callback would be called by the MediaPlayer each time m_private variable is cleared/changed.

[Eric Carlson]

(In reply to comment #0)
> RenderTheme relies on the mediaElement supportsFullscreen and supportMuting to know wether 
> it should paint the fullscreen and mute buttons or not. The problem is that the MediaPlayer first 
> loads a NullMediaPlayerPrivate which doesn't support those. So if you display a page with a video
> element and controls enabled (no autoplay), the theme will be rendered without those buttons 
> until video playback starts and the controls are painted again.
> 
> So I think we could have a mediaPlayerBackendChanged() callback in the mediaPlayerClient 
> interface which in the case of the HTMLMediaElement trigger an update of the theme. Would 
> that make sense, Eric? The new callback would be called by the MediaPlayer each time 
> m_private variable is cleared/changed.

Sounds like a good idea.

[Philippe Normand]

Sorry this bug is invalid, the issue of controls buttons not displayed some times is specific to the GStreamer media-player. Will file a separate bug.

[Philippe Normand]

Turns out this is really needed, at least in the GStreamer player case where hasVideo and hasAudio are dependant on the player state. So when these values are updated the controls update needs to be triggered as well. See also bug 50382

[Philippe Normand]

Created attachment 75376 [details]
proposed patch

[Eric Seidel (no email)]

Attachment 75376 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6839008

[Philippe Normand]

Created attachment 75385 [details]
proposed patch

[Eric Carlson]

Comment on attachment 75385 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75385&action=review

> WebCore/html/HTMLMediaElement.cpp:1964
> +void HTMLMediaElement::mediaPlayerBackendUpdated(MediaPlayer*)

I am not sure about this function name, the term "backend" isn't used anywhere else in this file. "Media engine" is used in both comments and "mediaEngineError", so maybe "mediaPlayerEngineUpdated" or "mediaPlayerEngineChanged" instead?

> WebCore/html/HTMLMediaElement.cpp:1969
> +        toRenderMedia(renderer())->updateFromElement();

Why is this cast necessary, updateFromElement() is called elsewhere in the file without it?


r+, though I would prefer a different name for the callback.

[Philippe Normand]

Thanks, I went for mediaPlayerEngineUpdated. Landed in http://trac.webkit.org/changeset/73249