[webkit-reviews] review denied: [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe : [Attachment 208831] Updated patch against upstream master

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied  review:
Bug 115352: [gstreamer] Make sure gstreamer source element is thread-safe
https://bugs.webkit.org/show_bug.cgi?id=115352

Attachment 208831: Updated patch against upstream master
https://bugs.webkit.org/attachment.cgi?id=208831&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208831&action=review


> Source/WebCore/ChangeLog:7
> +	   Patch by Andre Moreira Magalhaes <andre.magalhaes at collabora.co.uk>
on 2013-08-14
> +	   Reviewed by Philippe Normand.

Please, don't submit this as reviewed, since it contains new code that hasn't
been reviewed.

> Source/WebCore/ChangeLog:37
> +	   * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> +	   (_WebKitWebSrcPrivate):
> +	   (webkit_web_src_init):
> +	   (webKitWebSrcFinalize):
> +	   (webKitWebSrcSetProperty):
> +	   (webKitWebSrcGetProperty):
> +	   (webKitWebSrcStop):
> +	   (webKitWebSrcStart):
> +	   (webKitWebSrcChangeState):
> +	   (webKitWebSrcQueryWithParent):
> +	   (webKitWebSrcGetUri):
> +	   (webKitWebSrcSetUri):
> +	   (webKitWebSrcNeedDataMainCb):
> +	   (webKitWebSrcEnoughDataMainCb):
> +	   (webKitWebSrcSeekMainCb):
> +	   (webKitWebSrcSeekDataCb):
> +	   (webKitWebSrcSetMediaPlayer):
> +	   (StreamingClient::StreamingClient):
> +	   (StreamingClient::~StreamingClient):
> +	   (StreamingClient::didReceiveResponse):
> +	   (StreamingClient::didReceiveData):
> +	   (StreamingClient::didFinishLoading):
> +	   (StreamingClient::wasBlocked):
> +	   (StreamingClient::cannotShowURL):

This doesn't look up to date. You should explain briefly here what you are
changing.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:71
> +class StreamingClientHandler {
> +    public:
> +	   StreamingClientHandler(WebKitWebSrc*);
> +	   virtual ~StreamingClientHandler();
> +
> +	   char* getOrCreateReadBuffer(size_t requestedSize, size_t&
actualSize);
> +	   void responseReceived(const ResourceResponse&);
> +	   void dataReceived(const char*, int);
> +	   void notifyFinished();
> +
> +    private:
> +	   WebKitWebSrc* m_src;
> +};

I wonder if we could add the common implementation to the base class instead of
using a new class.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:73
> +// common StreamingClient interface

Comments should start with capital letter and finish with a period.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:80
> +	   virtual bool loadFailed() const = 0;
> +	   virtual void setDefersLoading(bool) = 0;
> +};

So, instead of an abstract class, we could probably add the common
implementation here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:98
>	   WebKitWebSrc* m_src;

This is already in the handler, maybe we could add a getter to the handler, or
if we end up using a base class, move this to the base class as protected
member.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:99
> +	   StreamingClientHandler* m_handler;

If we use the handler this should be OwnPtr<StreamingClientHandler>

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:124
> +
> +	   WebKitWebSrc* m_src;
> +	   StreamingClientHandler* m_handler;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-288
> -    if (priv->buffer) {
> -#ifdef GST_API_VERSION_1
> -	   unmapGstBuffer(priv->buffer.get());
> -#endif
> -	   priv->buffer.clear();
> -    }

Why don't we need this anymore?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371
> +	   GST_OBJECT_LOCK(src);
>	   priv->iradioMode = g_value_get_boolean(value);
> +	   GST_OBJECT_UNLOCK(src);

I wonder if we could add a simple GstMutexLocker class, to simply the locks,
you wouldn't need to unlock the object on every function return, for example.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:439
> +// must be called on main thread and with object unlocked

Comments should start with capital letter and finish with a period.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:443
> +    gboolean seeking;

Declare this when first used, and use bool instead of gboolean.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:500
> +// must be called on main thread and with object unlocked

Comments should start with capital letter and finish with a period.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558
> +	   CachedResourceLoader* loader = priv->player->cachedResourceLoader();

> +	   if (loader)
> +	       priv->client = new CachedResourceStreamingClient(src, loader,
request);

if (CachedResourceLoader* loader = priv->player->cachedResourceLoader())
    priv->client = new CachedResourceStreamingClient(src, loader, request);

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:607
> +	   priv->startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0,
(GSourceFunc) webKitWebSrcStart, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

You can use g_idle_add_full with G_PRIORITY_DEFAULT instead of a timeout source
with 0 delay. You should use C++ style casts for GSourceFunc and
GDestroyNotify.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:615
> +	   priv->stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0,
(GSourceFunc) webKitWebSrcStop, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:638
> +	   if ((format == GST_FORMAT_BYTES) && (src->priv->size > 0)) {

I know this was already like this, but now that you are modifying it we can
take advantage to remove the extra parentheses.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926
> +// StreamingClientHandler

This comment looks redundant to me.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
> +    gst_object_unref(m_src);
> +    m_src = 0;

We should use GRefPtr for this

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1124
> +// CachedResourceStreamingClient

This comment looks redundant too.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1127
> +    , m_handler(new StreamingClientHandler(src))

This should use OwnPtr and adopt the pointer here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1138
> +    gst_object_unref(m_src);
> +    m_src = 0;

This should use GRefPtr. I think this should be done in the parent class or the
handler, but not in both places.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1151
> +    return (!m_resource);

I don't think the parentheses are need here.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1191
> +// ResourceHandleStreamingClient

This comment looks also redundant.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1194
> +    , m_handler(new StreamingClientHandler(src))

Same comment about OwnPtr