[webkit-reviews] review granted: [Bug 212482] [macOS] Drag/drop an image of a unsupported format to an file input element should convert it to a supported format : [Attachment 406235] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 212482: [macOS] Drag/drop an image of a unsupported format to an file input
element should convert it to a supported format
https://bugs.webkit.org/show_bug.cgi?id=212482

Attachment 406235: Patch

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




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 406235
  --> https://bugs.webkit.org/attachment.cgi?id=406235
Patch

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

> Source/WebCore/html/FileInputType.cpp:504
> +    sharedImageTranscodingQueue()->dispatch([this, protectedThis =
makeRef(*this), paths = paths.isolatedCopy(), transcodingPaths =
transcodingPaths.isolatedCopy(), transcodingUTI =
transcodingUTI.isolatedCopy(), transcodingExtension =
transcodingExtension.isolatedCopy()]() mutable {

Not 100% sure about the efficiency/thread-safety here.

With protectedThis seems like we are carefully passing this in a way that does
not require a thread-safe reference count. Perhaps we could use the same
approach with "paths" that we use with protectedThis, and not make an isolated
copy, since it’s only going to be used back on the main thread. I think that
means that here we would write "paths" and below we would write paths =
WTFMove(paths) and it would be safe for the same reason that protectedThis is
safe.

Alternatively we could change InputType to use ThreadSafeRefCounted, to make
this more obviously thread safe.

Maybe another way to write this that’s more obviously safe would be to create
another inner lambda, one that takes a replacementPaths argument, as a local
variable and capture *that* in the the outer lambda. Then we would not need to
capture "this", "protectedThis", or "paths" in the outer lambda:

    auto callFilesChosen = [protectedThis = makeRef(this), paths] (const
Vector<String>& replacementPaths) {
	protectedThis->filesChosen(paths, replacementPaths);
    };
    sharedImageTranscodingQueue()->dispatch([callFilesChosen =
WTFMove(callFilesChosen), transcodingPaths = transcodingPaths.isolatedCopy(),
transcodingUTI = transcodingUTI.isolatedCopy(), transcodingExtension =
transcodingExtension.isolatedCopy()]() mutable {
	ASSERT(!RunLoop::isMain());
	auto replacementPaths = transcodeImages(transcodingPaths,
transcodingUTI, transcodingExtension);
	ASSERT(transcodingPaths.size() == replacementPaths.size());
	RunLoop::main().dispatch([callFilesChosen = WTFMove(callFilesChosen),
replacementPaths = replacementPaths.isolatedCopy()]() {
	    callFilesChosen(replacementPaths);
	});
    });

I think the thread safety is easier to understand in that version.

> Source/WebCore/platform/graphics/ImageUtilities.h:30
> +WEBCORE_EXPORT Ref<WorkQueue> sharedImageTranscodingQueue();

I suggest this return WorkQueue& instead of Ref<WorkQueue> since the queue in
question is global and immortal. No reason to return a newly protected pointer
each time.