Bug 212482

<rdar://problem/63731672>

Created attachment 402432 [details]
Patch

Created attachment 406041 [details]
Patch

WIP patch

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

Created attachment 406044 [details]
Patch

Created attachment 406047 [details]
Patch

Created attachment 406051 [details]
Patch

Created attachment 406141 [details]
Patch

Layout test were added.

Created attachment 406235 [details]
Patch

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.

Created attachment 406273 [details]
Patch

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.

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

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