[webkit-reviews] review granted: [Bug 56930] [GStreamer] RTSP playback broken : [Attachment 88592] proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 56930: [GStreamer] RTSP playback broken
https://bugs.webkit.org/show_bug.cgi?id=56930

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88592&action=review

Looks good, though please make callback static, unless there is a reason they
should be exposed in the header.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:215
> +void mediaPlayerPrivateVideoChangedCallback(GObject* element,
MediaPlayerPrivateGStreamer* player)

You should omit "element" here. Why aren't all of these callbacks defined
statically inside this file? Are they shared anywhere?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:220
> +void mediaPlayerPrivateAudioChangedCallback(GObject* element,
MediaPlayerPrivateGStreamer* player)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:55
> +void mediaPlayerPrivateVideoChangedCallback(GObject* element,
MediaPlayerPrivateGStreamer*);
> +void mediaPlayerPrivateAudioChangedCallback(GObject* element,
MediaPlayerPrivateGStreamer*);
> +gboolean
mediaPlayerPrivateAudioChangeTimeoutCallback(MediaPlayerPrivateGStreamer*);
> +gboolean
mediaPlayerPrivateVideoChangeTimeoutCallback(MediaPlayerPrivateGStreamer*);

As above, couldn't these just be defined statically in the C++ file? Why make
them public? Also, you should omit the "element" variable.