[Webkit-unassigned] [Bug 56930] [GStreamer] RTSP playback broken

Thu Apr 7 16:45:14 PDT 2011

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

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88592|review?                     |review+
               Flag|                            |

--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2011-04-07 16:45:14 PST ---
(From update of attachment 88592)
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.

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