[webkit-reviews] review denied: [Bug 141469] [GTK] Layout Test http/tests/media/hls/hls-progress.html is failing : [Attachment 315936] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 141469: [GTK] Layout Test http/tests/media/hls/hls-progress.html is failing
https://bugs.webkit.org/show_bug.cgi?id=141469

Attachment 315936: Patch

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




--- Comment #4 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 315936
  --> https://bugs.webkit.org/attachment.cgi?id=315936
Patch

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

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1198
> +    char* name = gst_element_get_name(element);

Please use GST_ELEMENT_NAME instead and you won't have to free the name later.
Make it const.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1202
> +	   GstElement* parent = GST_ELEMENT(gst_element_get_parent(element));

gst_element_get_parent is transfer full, you're leaking an element here. Please
use GST_ELEMENT_PARENT instead.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1207
> +	   char* grandpaName = gst_element_get_name(grandparent);

When using GST_ELEMENT_NAME, probably you won't need to use this variable, but
if you decided to keep it, use complete words.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1220
> +static bool hlsIsProgressing(GstElement* playbin, GstQuery** query)

If we are not freeing the query, it can be GstQuery* query.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1223
> +    GstIterator* it = gst_bin_iterate_recurse(GST_BIN(playbin));

Please use iterator (complete words).

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1224
> +    bool foundHLSElement = gst_iterator_find_custom(it,
reinterpret_cast<GCompareFunc>(findHLSElement), &item, NULL);

NULL -> nullptr

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1230
> +    GstElement* result =
static_cast<GstElement*>(g_value_get_object(&item));

Use the GNOME style cast there.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1246
> +	   if (!hlsIsProgressing(m_pipeline.get(), &query))

Please, add a comment here explaining why we are doing this hack.

Please, free the query out of the hlsIsProgressing function, which should be
renamed to isHLSProgressing.