[webkit-reviews] review granted: [Bug 133079] Use C++11 lambdas to construct FileThread::Task objects : [Attachment 231708] 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 133079: Use C++11 lambdas to construct FileThread::Task objects
https://bugs.webkit.org/show_bug.cgi?id=133079

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

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


>> Source/WebCore/fileapi/AsyncFileStream.cpp:81
>> +	AsyncFileStream* proxyPtr = proxy.get();
> 
> This code looks very suspicious. Both main thread and file thread have
pointers to AsyncFileStream, potentially referencing and dereferencing it
concurrently. But it's not even ThreadSafeRefCounted.

Alexey, is that a comment on some new aspect of the code or just on the code as
it already was?

> Source/WebCore/fileapi/AsyncFileStream.cpp:84
> +	   if (!proxyPtr->client())
> +	       return;

Why did you delete the comment about a race condition that was in the
startOnFileThread function?

> Source/WebCore/fileapi/AsyncFileStream.cpp:171
> +    URL blobURLCopy = blobURL.copy();

I wonder why we need isolatedCopy for String, but not for URL. Maybe URL needs
an isolatedCopy function too. That’s not something that’s changing in this
patch, but might be worth thinking through.

> Source/WebCore/fileapi/FileThread.h:71
> +	   Task(Task&& other)
> +	       : m_task(std::move(other.m_task))
> +	       , m_instance(other.m_instance)
> +	   {
> +	   }

Do we need to write this explicitly? Won’t the compiler generate this if we
don’t write it?