[webkit-reviews] review denied: [Bug 190902] [MSE][GStreamer] Introduce AbortableTaskQueue : [Attachment 353136] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied 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 353136: Patch

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




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

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

You might need to rebase as well.

Amazing patch Alicia, thanks for writing it, it is a very good refactor. Just
some things need to be worked out and we will be good to go.

> Source/WebCore/platform/AbortableTaskQueue.h:24
> +#include <functional>
> +#include <memory>
> +#include <queue>

I don't think you need all these, do you?

> Source/WebCore/platform/AbortableTaskQueue.h:31
> +#include <wtf/StdLibExtras.h>
> +#include <wtf/ThreadSafeRefCounted.h>
> +#include <wtf/Vector.h>
> +#include <wtf/glib/GRefPtr.h>

Ditto?

> Source/WebCore/platform/AbortableTaskQueue.h:106
> +    void stopAborting()

I think I would call this finishAborting.

Besides this, I see this is not used in the AppendPipeline code yet, it's for
short term future patches. We should comment this in the changelog about this.

> Source/WebCore/platform/AbortableTaskQueue.h:149
> +	       return m_aborting || response.has_value();

has_value -> operator bool in all places

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

>From what I see, this is not an integral part of this patch and it could be
done in a separate one. I don't dislike it being here, it can fit, but please,
add a comment to the changelog.

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:191
> +    g_signal_connect(m_demux.get(), "pad-added", G_CALLBACK(+[](GstElement*,
GstPad* demuxerSrcPad, AppendPipeline* appendPipeline) {
> +	  
appendPipeline->connectDemuxerSrcPadToAppsinkFromAnyThread(demuxerSrcPad);
> +    }), this);

This *FromAnyThread functions could suffer a name change. Feel free to do it
now or consider it for another patch.

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

This is redundant since it is checked in the method called bellow.

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:30
> +#include <thread>

Not needed with WTF::Thread

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:40
> +    bool testFinished = false;
> +    int currentStep = 0;

You might want to use { } to initialize.

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:57
> +	   std::thread(backgroundThreadFunction).detach();

You might want to use WTF::Thread.

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:87
> +    bool testFinished = false;
> +    int currentStep = 0;

Ditto

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:96
> +	       FancyResponse ret(100);

returnValue

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:167
> +    bool testFinished = false;
> +    int currentStep = 0;

Ditto

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:181
> +	   // Main thread has called startAborting()

Period at the end.

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:187
> +	   // This call must return immediately because we are aborting

Ditto

> Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:197
> +	   // Main thread has called stopAborting()

Ditto