[webkit-reviews] review denied: [Bug 29959] [GTK] QoS support in the video sink : [Attachment 40640] updated patch

[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 29959: [GTK] QoS support in the video sink
https://bugs.webkit.org/show_bug.cgi?id=29959

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

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
Awesome stuff, and I'm happy to see the real fix for the race condition
happening! What do you think of moving this sink to WebKit/gtk/webkit/ like we
did with soup-dialog, by the way? The synchronization and object life time
handling look OK to me, I have just a couple nitpicks:

> +    // use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations
> +    priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0,
> +					     webkit_video_sink_timeout_func,
> +					     gst_object_ref(sink),
> +					     (GDestroyNotify)
gst_object_unref);

Additional space on the cast, and I suggest a normal sentence in the comment,
with capital U and period =).

>  
>  static void
> -webkit_video_sink_finalize(GObject* object)
> +_unlock_buffer_mutex(WebKitVideoSinkPrivate* priv)
> +{
> +
> +    g_mutex_lock(priv->buffer_mutex);
> +

There seems to be an unnecessary line at the start of this function. Also, I
don't think we use this _name pattern anywhere else, so I'd suggest just
dropping the underscore here. You are already declaring it static, which should
be enough.

Thanks for your work on this!