WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
50228
Theme not updated when MediaPlayer m_private engine changes
https://bugs.webkit.org/show_bug.cgi?id=50228
Summary
Theme not updated when MediaPlayer m_private engine changes
Philippe Normand
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Eric Carlson
Comment 1
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.
Philippe Normand
Comment 2
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.
Philippe Normand
Comment 3
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
Philippe Normand
Comment 4
2010-12-02 07:36:34 PST
Created
attachment 75376
[details]
proposed patch
Eric Seidel (no email)
Comment 5
2010-12-02 09:08:29 PST
Attachment 75376
[details]
did not build on mac: Build output:
http://queues.webkit.org/results/6839008
Philippe Normand
Comment 6
2010-12-02 09:19:56 PST
Created
attachment 75385
[details]
proposed patch
Eric Carlson
Comment 7
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.
Philippe Normand
Comment 8
2010-12-03 00:40:12 PST
Thanks, I went for mediaPlayerEngineUpdated. Landed in
http://trac.webkit.org/changeset/73249
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug