[webkit-reviews] review denied: [Bug 204273] [GStreamer] Pipeline lifetime tracking : [Attachment 383700] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 204273: [GStreamer] Pipeline lifetime tracking
https://bugs.webkit.org/show_bug.cgi?id=204273

Attachment 383700: Patch

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




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

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:389
> +    String name = gst_object_get_name(GST_OBJECT_CAST(pipeline));

You're leaking the name here. gst_object_get_name returns a transfer full and
the String constructor is going to copy it.

You can either use GUniquePtr or something like:
String name(StringImpl::createWithoutCopying(gst_object_get_name...));

Besides, I would recommend first return on !GST_IS_ELEMENT (maybe
G_RETURN_IF_FAIL) just in case we get funny things. With nulls the best we can
get is a critical on gst_object_get_name and the worst a crash if checks are
disabled (rare but possible). By the code we're using it looks like we are not
getting nulls but for the future, better safe than sorry.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:401
> +    String name = gst_object_get_name(GST_OBJECT_CAST(pipeline));

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:415
> +    Vector<String> names;
> +    for (auto name : s_webKitGStreamerPipelines.keys())
> +	   names.append(name);

We can try to save some allocations if we check the size first, allocate the
vector and then appendUnchecked.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:424
> +	   return "";

emptyString()? Or even better nullString().

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:429
> +    GRefPtr<GstElement> child = gst_bin_get_by_name(pipeline,
subBinName.utf8().data());

gst_bin_get_by_name is transfer full, you need to adopt here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:433
> +    return "";

Ditto