[webkit-reviews] review granted: [Bug 225650] [GStreamer] Add RAII lockers for GStreamer locks : [Attachment 428265] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Xabier Rodríguez
Calvar <calvaris at igalia.com>'s request for review:
Bug 225650: [GStreamer] Add RAII lockers for GStreamer locks
https://bugs.webkit.org/show_bug.cgi?id=225650

Attachment 428265: Patch

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




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

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:323
> +    explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); }

I wonder if we should ASSERT(lockable)...

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332
> +    T* lockable() { return m_lockable; }
> +
> +    explicit operator bool() const { return !!m_lockable; }

Why do you need these?

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:387
> +inline GstObjectLocker holdGstObjectLock(void* gstObject)
WARN_UNUSED_RETURN;
> +inline GstObjectLocker holdGstObjectLock(void* gstObject)
> +{
> +    return GstObjectLocker(gstObject);
> +}

Can't we collapse declaration and implementation?

Independently of asserting above, it could be interesting
ASSERT(GST_IS_OBJECT(gstObject))

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:393
> +inline GstPadStreamLocker holdGstPadStreamLock(GstPad*) WARN_UNUSED_RETURN;
> +inline GstPadStreamLocker holdGstPadStreamLock(GstPad* pad)
> +{
> +    return GstPadStreamLocker(pad);
> +}

Ditto.

Ditto.