Bug 50228

Summary: Theme not updated when MediaPlayer m_private engine changes
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, eric.carlson, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 50382    
Attachments:
Description Flags
proposed patch
none
proposed patch eric.carlson: review+

Description Philippe Normand 2010-11-30 05:45:28 PST
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.
Comment 1 Eric Carlson 2010-11-30 07:42:48 PST
(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.
Comment 2 Philippe Normand 2010-12-02 04:03:52 PST
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.
Comment 3 Philippe Normand 2010-12-02 07:24:19 PST
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
Comment 4 Philippe Normand 2010-12-02 07:36:34 PST
Created attachment 75376 [details]
proposed patch
Comment 5 Eric Seidel (no email) 2010-12-02 09:08:29 PST
Attachment 75376 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6839008
Comment 6 Philippe Normand 2010-12-02 09:19:56 PST
Created attachment 75385 [details]
proposed patch
Comment 7 Eric Carlson 2010-12-02 09:52:18 PST
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.
Comment 8 Philippe Normand 2010-12-03 00:40:12 PST
Thanks, I went for mediaPlayerEngineUpdated. Landed in http://trac.webkit.org/changeset/73249