[webkit-reviews] review denied: [Bug 39474] [GStreamer] GTK XOverlay support in GStreamerGWorld : [Attachment 57849] proposed patch

[bugzilla-daemon at webkit.org]

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

Attachment 57849: proposed patch
https://bugs.webkit.org/attachment.cgi?id=57849&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:113
 +	tee = gst_bin_get_by_name(GST_BIN(videoSink.get()), "t");
Variables should be declared at first usage in cases like this.

WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:142
 +	// See https://bugzilla.gnome.org/show_bug.cgi?id=620490.
Perhaps you can just do a check, conditional on having a new enough gstreamer,
so we have the feature in builds with newer versions, without the need to
require them?

 165	 GOwnPtr<GstElement> videoSink;
 166	 GstElement* tee;
 167	 GstElement* colorspace;
 168	 GstElement* platformVideoSink;
 169	 GstElement* queue;
 170	 GstElement* videoScale;
 171	 GstPad* srcPad;
 172	 GstPad* sinkPad;

Same thing here, many of these declarations should be done at first usage - I
know some of them need to be pre-declared, like videoSink, but they can be
pre-declared in the line immediately above the function that gets them -
g_object_get in this case.

I can't find what makes the m_fullscreenWindow go fullscreen (like a call to
gtk_window_fullscreen), is this planned to be handled elsewhere? I also did not
find who calls enterFullscreen, I think you're planning to land this first, and
then wiring it up? I'd prefer to have the wiring up in this patch, otherwise
we're landing dead code. I'll mark this r- so we can work on the issues I
mentioned, thanks for working on this! =)