[webkit-reviews] review denied: [Bug 106760] [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support : [Attachment 183671] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 20 09:39:23 PST 2013


Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 106760: [GStreamer] USE(NATIVE_FULLSCREEN_VIDEO) support
https://bugs.webkit.org/show_bug.cgi?id=106760

Attachment 183671: Patch
https://bugs.webkit.org/attachment.cgi?id=183671&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.c
pp: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?


More information about the webkit-reviews mailing list