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.
<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].