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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser 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 403709: Patch

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




--- Comment #35 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 403709
  --> https://bugs.webkit.org/attachment.cgi?id=403709
Patch

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

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

It's not really clear from the signature that allowedMIMETypes is the set of
possible destination MIME types, and that the return value is a set of new
paths. If one of the input paths is not transcodable, how does the return value
relate to the input paths?

I feel like this needs to be a more expressive API, maybe even a simple class,
that makes it easier to use without error.

Also are these absolute paths or URLs? The call site makes them look like URLs.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:59
> +    auto suffix = makeString('.',
WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType));

What if preferredExtensionForMIMEType() is empty?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:65
> +	   return nullString();

This error case returns the same result as the "no translation needed" case, so
the caller can't tell them apart.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:91
> +    String mimeTypeForEncoding =
WebCore::MIMETypeRegistry::preferredImageMIMETypeForEncoding(allowedMIMETypes,
{ });
> +    if (mimeTypeForEncoding.isNull())
> +	   return { };

Oh, all the images get the same destination MIME type? That's not what I
expected from the signature. I thought it would be one entry in the vector per
image.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:98
> +	       replacementPaths.uncheckedAppend(nullString());

so nullString in the result means "no transcoding"?

> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
> +    auto replacementURLs = transcodeImages(fileURLs, allowedMIMETypes);

You're calling these URLs but they are urlStrings; not URL objects.