[webkit-reviews] review denied: [Bug 190454] Add an SPI to allow WebView clients to add additional supported image formats : [Attachment 352845] 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 190454: Add an SPI to allow WebView clients to add additional supported
image formats
https://bugs.webkit.org/show_bug.cgi?id=190454

Attachment 352845: Patch

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




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

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

Seems mostly OK but let's see one more version.

> Source/WebCore/platform/MIMETypeRegistry.cpp:61
> +static HashSet<String, ASCIICaseInsensitiveHash>*
supportedImageAdditionalMIMETypes;

This doesn't read well. Maybe supportedAdditionalImageMIMETypes (though the
"supported" is then confusing).

> Source/WebCore/platform/MIMETypeRegistry.cpp:194
> +	   supportedImageAdditionalMIMETypes = new HashSet<String,
ASCIICaseInsensitiveHash>;

The modern way to do all of these would be with a static NeverDestroyed<> and a
std::once in the caller.

> Source/WebCore/platform/MIMETypeRegistry.cpp:683
> +    // Reinitialize the supportedImageAdditionalMIMETypes if they have not
been set or were set to an empty list.
> +    if (!supportedImageAdditionalMIMETypes ||
!supportedImageAdditionalMIMETypes->isEmpty())
> +	   return;
> +    initializeSupportedImageAdditionalMIMETypes();

Why doesn't this just null out supportedImageAdditionalMIMETypes so that the
next call triggers a rebuild? The function can be called
supportedImageAdditionalMIMETypesChanged().

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:88
> +    // We are suppoed to set the additionalAllowedImageFormats only once
even
> +    // if multiple WebPages are created.

It's not clear if this comment is describing a bug (in which case it should be
a FIXME) or explaining the code.

> Source/WebCore/platform/graphics/cg/UTIRegistry.h:36
> +const HashSet<String>& defaultAllowedImageFormats();
> +const HashSet<String>& additionalAllowedImageFormats();
> +WEBCORE_EXPORT void setAdditionalAllowedImageFormats(const Vector<String>&);
> +bool isAllowedImageFormat(const String&);

The rename from "UTI" to "format" makes it less clear that these return UTIs.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
> + at property (nonatomic, copy, setter=_setAdditionalAllowedImageFormats:)
NSArray<NSString *> *_additionalAllowedImageFormats
WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

We might want to think about this naming. Does the caller of the API understand
the "additional" part?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/AdditionalAllowedImageFormats.mm:40
> +    [configuration
_setAdditionalAllowedImageFormats:@[@"com.truevision.tga-image"]];

Tests should also cover
* adding a format that is already supported
* adding a format that webkit can't render
* passing an array with the same format listed twice