[webkit-reviews] review denied: [Bug 213347] [macOS]: A HEIF image, selected from the OpenPanel, should be converted to an accepted MIME type : [Attachment 403850] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 213347: [macOS]: A HEIF image, selected from the OpenPanel, should be
converted to an accepted MIME type
https://bugs.webkit.org/show_bug.cgi?id=213347

Attachment 403850: Patch

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




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

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

> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:118
> +    CString suffixUtf8 = suffix.utf8();

I don’t understand why this function is using "Utf8" capitalization instead of
"UTF8".

> Source/WTF/wtf/glib/FileSystemGlib.cpp:329
> -String openTemporaryFile(const String& prefix, PlatformFileHandle& handle)
> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle,
const String&)

Seems bad that this platform simply ignores the suffix. Only OK because the
feature isn’t used? Maybe:

    // FIXME: Suffix is not supported, but OK for now since the code using it
is macOS-port-only.
    ASSERT_UNUSED(suffix, suffix.isEmpty());

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:445
> -String openTemporaryFile(const String& prefix, PlatformFileHandle& handle)
> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle,
const String&)

Ditto.

> Source/WTF/wtf/win/FileSystemWin.cpp:407
> -String openTemporaryFile(const String&, PlatformFileHandle& handle)
> +String openTemporaryFile(const String&, PlatformFileHandle& handle, const
String&)

Ditto.

> Source/WebKit/Platform/ImageUtilities.h:30
> +Vector<String> findImagesForTranscoding(const Vector<String>& paths, const
Vector<String>& allowedMIMETypes);

Probably need a comment explaining what the return value is.

> Source/WebKit/Platform/ImageUtilities.h:31
> +Vector<String> transcodeImages(const Vector<String>& paths, const String&
uti, const String& extension);

Probably need a comment explaining what the return value is.

Maybe "destinationUTI" rather than "uti" and "destinationExtension" rather than
"extension".

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:41
> +    // This function transcodes files only.
> +    if (FileSystem::fileIsDirectory(path,
FileSystem::ShouldFollowSymbolicLinks::Yes))
> +	   return nullString();

Why do we need this code? Won’t CGImageSourceCreateWithURL fail without this
check?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:43
> +    // Ensure the 'path' is actually an image file.

This doesn’t seem like an accurate comment. This doesn’t "ensure it’s an image
file". I don’t think the comment is needed.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:49
> +    // Ensure the format of the image source is different from 'mimeType'.

Comment says nothing different from what the code below does. Why is it a
valuable to fail in this case? Do we have test coverage for this case? I
suggest omitting this code.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:54
> +    // The file extension is important because it reveals the MIME type of
the temporary file.

This is imprecise. What does "reveals" mean here? Do you mean that it helps
software select the appropriate MIME type? What software are we trying to help
here? Comment could cover that.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:67
> +	       return FileSystem::writeToFile(handle, static_cast<const
char*>(buffer), count);

Does this code do the correct thing when there is an error?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:75
> +    // Write the source image to the destination image.

Should remove this comment.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:78
> +    FileSystem::closeFile(destinationFileHandle);

Does this code do the correct thing when there is an error?

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6694
>> +	m_transcodingQueue->dispatch([this, protectedThis = makeRef(*this),
fileURLs = fileURLs, transcodingURLs = WTFMove(transcodingURLs), transcodingUTI
= transcodingUTI, transcodingExtension = transcodingExtension]() mutable {
> 
> mutable is not needed. I will remove it.

It’s not safe to pass strings across threads. Their reference counts are not
thread-safe. We need to pass isolated copies across threads.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
>> +	    RunLoop::main().dispatch([this, fileURLs = fileURLs, transcodedURLs
= WTFMove(transcodedURLs)]() mutable {
> 
> Ditto.

Ditto.