[webkit-reviews] review granted: [Bug 190902] [MSE][GStreamer] Introduce AbortableTaskQueue : [Attachment 354478] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 190902: [MSE][GStreamer] Introduce AbortableTaskQueue
https://bugs.webkit.org/show_bug.cgi?id=190902

Attachment 354478: Patch

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




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

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

> Source/WebCore/platform/AbortableTaskQueue.h:122
> +    void tryEnqueueTask(WTF::Function<void()>&& mainThreadTaskHandler)

I would just name this enqueueTask. I don't think we need the try here, it is
obvious because of the nature of this ATQ that we'll fail if it's aborting.

> Source/WebCore/platform/AbortableTaskQueue.h:124
> +	   LockHolder lockHolder(m_mutex);

I'd suggest to ASSERT(!isMainThread()); before this as well.

> Source/WebCore/platform/AbortableTaskQueue.h:136
> +    std::optional<R> tryEnqueueTaskAndWaitForReply(WTF::Function<R()>&&
mainThreadTaskHandler)

Similar to what I commented above, I would call this enqueueTaskAndWait. I
think we neither need the try nor the ForReply.

> Source/WebCore/platform/AbortableTaskQueue.h:191
> +	       if (!isCancelled()) {

We better bail out here if it'd cancelled.

> Source/WebCore/platform/AbortableTaskQueue.h:229
> +    WTF::Deque<Ref<Task>> m_channel;

I guess what made you switch to Ref was actually switching to Deque, right?

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:140
> +    g_signal_connect(m_bus.get(), "sync-message::need-context",
G_CALLBACK(+[](GstBus*, GstMessage* message, AppendPipeline* appendPipeline) {
> +	   appendPipeline->handleNeedContextSyncMessage(message);
> +    }), this);
> +    g_signal_connect(m_bus.get(), "message::state-changed",
G_CALLBACK(+[](GstBus*, GstMessage* message, AppendPipeline* appendPipeline) {
> +	   appendPipeline->handleStateChangeMessage(message);
> +    }), this);

I still see no references to this on the changelog.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:333
> -    ASSERT(WTF::isMainThread());
> +    ASSERT(isMainThread());

Please, comment about this in the Changelog as well.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:872
> +    if (!response.has_value()) {

if (!response) {
...
}

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h:60
> +private:

You made lost of methods private, please comment it on the changelog.