[webkit-reviews] review granted: [Bug 129795] ScriptExecutionContext::Task should work well with C++11 lambdas : [Attachment 229400] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 129795: ScriptExecutionContext::Task should work well with C++11 lambdas
https://bugs.webkit.org/show_bug.cgi?id=129795

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229400&action=review


This change is a very good idea. I think we can do even more simplification,
but this is a huge step in the right direction.

As a future step, I think we can remove the CrossThreadCopier and CallbackTask
and createCallback machinery entirely. No reason callers can’t just copy
arguments explicitly rather than using this massive machinery to generate the
calls to copy. I tried it and the code comes out much more readable.

> Source/WebCore/dom/ScriptExecutionContext.h:131
> +	   template<typename T, typename = typename
std::enable_if<!std::is_base_of<Task, T>::value && std::is_convertible<T,
std::function<void (ScriptExecutionContext*)>>::value>::type>

I don’t think these functions should take a ScriptExecutionContext*. It would
be cleaner to have them take no argument at all. Or ScriptExecutionContext&.

The few tasks that need a to get at the document or script execution context
could just pass it in. Passing it to all tasks makes them all uglier and more
complex for no reason.

> Source/WebCore/dom/ScriptExecutionContext.h:149
> +	   Task(Task&& task)
> +	       : m_task(std::move(task.m_task))
> +	       , m_isCleanupTask(task.m_isCleanupTask)
> +	   {
> +	   }

Won’t the compiler generate this automatically? I don’t think we need to write
this code.

> Source/WebCore/dom/ScriptExecutionContext.h:159
> +    virtual void postTask(Task) = 0; // Executes the task on context's
thread asynchronously.

I think this should take a Task&& rather than a Task. Doesn’t have to be done
on this first check-in, but I think it would be more efficient and also clearer
in concept.

> Source/WebCore/workers/WorkerMessagingProxy.cpp:191
> +	   Vector<std::unique_ptr<ScriptExecutionContext::Task>>
queuedEarlyTasks = std::move(m_queuedEarlyTasks);

Seems like we should use auto here instead of writing out that entire type.

> Source/WebCore/workers/WorkerRunLoop.h:63
> +	   void postTask(ScriptExecutionContext::Task);
> +	   void postTaskAndTerminate(ScriptExecutionContext::Task);
> +	   void postTaskForMode(ScriptExecutionContext::Task, const String&
mode);

Again, I think Task&& is better than Task for these argument types.