[webkit-reviews] review denied: [Bug 185787] [GTK][WPE] Start implementing MediaStream API : [Attachment 342085] [GTK][WPE]: Implement MediaStream API

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 185787: [GTK][WPE] Start implementing MediaStream API
https://bugs.webkit.org/show_bug.cgi?id=185787

Attachment 342085: [GTK][WPE]: Implement MediaStream API

https://bugs.webkit.org/attachment.cgi?id=342085&action=review




--- Comment #5 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 342085
  --> https://bugs.webkit.org/attachment.cgi?id=342085
[GTK][WPE]: Implement MediaStream API

View in context: https://bugs.webkit.org/attachment.cgi?id=342085&action=review

Ok I think this is almost ready to land. Is this patch supposed to apply on
trunk? The EWS fails.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:55
> +#include "gstreamer/GStreamerMediaStreamSource.h"

No prefix please ;)

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:177
> +    GST_INFO_OBJECT((gpointer) pipeline(), "Going to PLAYING!");

This cast is really needed?

> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:186
> +    GST_INFO_OBJECT((gpointer) pipeline(), "Tearing down!");

Ditto

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:30
2
> +    GstElementClass* gstelement_klass = (GstElementClass*)klass;

Use GST_ELEMENT_CLASS(klass)

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:35
6
> +	   (GstPadChainFunction)webkitMediaStreamSrcChain);

You'll think I'm obsessed with casts ;)

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:35
8
> +    g_assert(gst_element_add_pad(GST_ELEMENT(self), (GstPad*)ghostpad));

GST_PAD_CAST() :)
Also please use ASSERT instead of g_assert