[Webkit-unassigned] [Bug 106760] [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106760





--- Comment #4 from Philippe Normand <pnormand at igalia.com>  2013-01-20 10:01:25 PST ---
(In reply to comment #3)
> (From update of attachment 183671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183671&action=review
> 
> LGTM in general, I'll r- just because I'm worried about the checks in PlatformVideoWindowGtk might be hiding a bug, might be better to figure it out
> 

Thanks for the review :)

> > Source/WebCore/ChangeLog:20
> > +        * platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp: Added.
> 
> Are you going to move the FullscreenVideoController from WebKit/gtk/WebCoreSupport into WebCore to reuse it for WebKit2 and so on?
> 

Yes! See also bug 107398. I haven't yet updated the WK2 code but it should be easy.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1760
> > +#if USE(NATIVE_FULLSCREEN_VIDEO)
> > +void MediaPlayerPrivateGStreamer::enterFullscreen()
> > +{
> > +    // TODO.
> > +}
> > +
> > +void MediaPlayerPrivateGStreamer::exitFullscreen()
> > +{
> > +    // TODO.
> > +}
> 
> We should use notImplemented() here
> 

Well the bug above fixes those already.

> > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:54
> >          gtk_container_remove(GTK_CONTAINER(m_window), m_videoWindow);
> > -        gtk_widget_destroy(m_videoWindow);
> > +        if (GTK_IS_WIDGET(m_videoWindow))
> > +            gtk_widget_destroy(m_videoWindow);
> 
> Is this protecting against m_videoWindow having been destroyed by being removed from the container? It looks a bit hacky
> 

Hacky indeed, I was getting some warnings when destroying a non-GtkWidget IIRC. I'll check those again.

> > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowGtk.cpp:58
> > -    if (m_window) {
> > +    if (GTK_IS_WIDGET(m_window)) {
> 
> Same here, the way I understand it m_window has to be a GtkWidget, am I misunderstanding?

No, something bad is going on indeed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.