Bug 50228 - Theme not updated when MediaPlayer m_private engine changes
Summary: Theme not updated when MediaPlayer m_private engine changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 50382
  Show dependency treegraph
 
Reported: 2010-11-30 05:45 PST by Philippe Normand
Modified: 2010-12-03 00:40 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (4.55 KB, patch)
2010-12-02 07:36 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (4.55 KB, patch)
2010-12-02 09:19 PST, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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