Bug 212482 – [macOS] Drag/drop an image of a unsupported format to an file input element should convert it to a supported format

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

If a page has an <input> file element which accepts an image like one of these:

<input type="file" accept="image/*">
<input type="file" accept="image/png, image/jpeg">

And the user drags/drops an image of unsupported format, HEIF for example, the image should be converted to one of the formats in the "accept" attribute if possible. This will be similar to what happens when dragging and dropping a photo from the "Photos" app which is converting the HEIF image to JPEG image.

[Radar WebKit Bug Importer]

<rdar://problem/63731672>

[Said Abou-Hallawa]

Created attachment 402432 [details]
Patch

[Said Abou-Hallawa]

Created attachment 406041 [details]
Patch

WIP patch

[Said Abou-Hallawa]

This patch is mostly working. It still needs some clean-up and maybe threading.

[Said Abou-Hallawa]

Created attachment 406044 [details]
Patch

[Said Abou-Hallawa]

Created attachment 406047 [details]
Patch

[Said Abou-Hallawa]

Created attachment 406051 [details]
Patch

[Said Abou-Hallawa]

Created attachment 406141 [details]
Patch

[Said Abou-Hallawa]

Layout test were added.

[Said Abou-Hallawa]

Created attachment 406235 [details]
Patch

[Darin Adler]

Comment on attachment 406235 [details]
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.

[Said Abou-Hallawa]

Created attachment 406273 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 406235 [details]
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.

This is neat.

I have just realized that lambda captures its parameters when it's defined not when it is called. Using this solution, we do not have to capture *this" twice: (1) from the main thread to the transcoding thread and (2) form the transcoding thread back to the main thread.

[EWS]

Committed r265422: <https://trac.webkit.org/changeset/265422>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406273 [details].