[webkit-reviews] review denied: [Bug 122831] [GStreamer] Store video-sink in a bin : [Attachment 215374] [player] make video-sink a bin

[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 122831: [GStreamer] Store video-sink in a bin
https://bugs.webkit.org/show_bug.cgi?id=122831

Attachment 215374: [player] make video-sink a bin
https://bugs.webkit.org/attachment.cgi?id=215374&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=215374&action=review


LGTM in general, but it lacks a changelog entry.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
589
>      }
>  
>      if (!m_fpsSink) {

This should be an else block. I think the actualVideoSink name is misleading
because it may be something that is not the actual video sink heh. Is videoSink
taken?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
603
> -    return actualVideoSink;
> +    firstChild = actualVideoSink;

You could initialize firstChild to this instead of doing it in the else block,
just a nit.