[webkit-reviews] review granted: [Bug 123804] Manage ScriptExecutionContext::Task derivatives through std::unique_ptr : [Attachment 216720] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 123804: Manage ScriptExecutionContext::Task derivatives through
std::unique_ptr
https://bugs.webkit.org/show_bug.cgi?id=123804

Attachment 216720: Patch
https://bugs.webkit.org/attachment.cgi?id=216720&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216720&action=review


This looks fine, so I'm gonna say r=me.
You'll probably need to fix up some things, like s/OVERRIDE/override/ for it to
build.

It would also be good to go over these classes and spam final/override as
appropriate.

> Source/WebCore/Modules/webdatabase/Database.cpp:89
> +    DerefContextTask(PassRefPtr<ScriptExecutionContext> context)

explicit

> Source/WebCore/Modules/webdatabase/Database.cpp:203
> +    DeliverPendingCallbackTask(PassRefPtr<SQLTransaction> transaction)

explicit

>
Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:2
37
> +    Vector<std::unique_ptr<ScriptExecutionContext::Task>> tasks =
std::move(m_pendingTasks);
> +    for (auto iter = tasks.begin(); iter != tasks.end(); ++iter)
>	   (*iter)->performTask(0);

I'd go all in and write this like so:

auto tasks = std::move(m_pendingTasks);
for (auto& task : tasks)
    task.performTask(0);

> Source/WebCore/bindings/js/JSCallbackData.h:80
> +    DeleteCallbackDataTask(JSCallbackData* data) : m_data(data) { }

explicit

> Source/WebCore/workers/WorkerMessagingProxy.cpp:139
>      WorkerGlobalScopeDestroyedTask(WorkerMessagingProxy* messagingProxy)

explicit

> Source/WebCore/workers/WorkerMessagingProxy.cpp:212
>      NotifyNetworkStateChangeTask(bool isOnLine)

explicit