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

[bugzilla-daemon at webkit.org]

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


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #183671|review?                     |review-
               Flag|                            |




--- Comment #3 from Gustavo Noronha (kov) <gns at gnome.org>  2013-01-20 09:41:14 PST ---
(From update of attachment 183671)
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

> 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?

> Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp:29
> +

No newline

> Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:237
> +    // Unlink and release request pad.
> +    gst_pad_unlink(srcPad.get(), sinkPad.get());
> +    gst_element_release_request_pad(tee.get(), srcPad.get());
> +
> +    // Unlink, remove and cleanup queue, ffmpegcolorspace, videoScale and sink.

I know they were there before, but these comments do not seem to add anything, what do you think of removing them?

> 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

> 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

> 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?

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