[webkit-reviews] review granted: [Bug 195631] [GStreamer] Rewrite HTTP source element using pushsrc base class : [Attachment 364970] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 195631: [GStreamer] Rewrite HTTP source element using pushsrc base class
https://bugs.webkit.org/show_bug.cgi?id=195631

Attachment 364970: Patch

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




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

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

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:72
> +	   notify(notificationType, [functor = WTFMove(callbackFunctor),
condition = &condition, mutex = &mutex] {

nit: isn't it enough with writing ..., &condition, &muter] ? Then they should
be passed by reference.

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:74
> +	       LockHolder holder(*mutex);

Nit: I don't think you need the * here (if you pass parameters as reference
instead of pointer copy as you're doing above).

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:101
> +    bool durationSet;

isDurationSet :)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371
> +	       if (available && (available < size))

nit: do you need the inner () ?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:936
> +	   gst_structure_set(httpHeaders, "uri", G_TYPE_STRING,
priv->originalURI.data(),
> +	       "http-status-code", G_TYPE_UINT, response.httpStatusCode(),
nullptr);

Nit: this can probably be just one-line.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:951
> +	       bool ok = false;
> +	       uint64_t convertedValue = header.value.toUInt64(&ok);
> +	       if (ok)
> +		   gst_structure_set(headers.get(), header.key.utf8().data(),
G_TYPE_UINT64, convertedValue, nullptr);
> +	       else
> +		   gst_structure_set(headers.get(), header.key.utf8().data(),
G_TYPE_STRING, header.value.utf8().data(), nullptr);

Why do you do this?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:960
> +	   gst_element_post_message(GST_ELEMENT_CAST(src),
gst_message_new_element(GST_OBJECT_CAST(src),
> +	       gst_structure_copy(httpHeaders)));

Nit: this can probably be just one-line.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:979
> +    if (G_LIKELY (priv->requestedPosition == priv->readPosition))

I think we have LIKELY in WebKit