[webkit-reviews] review granted: [Bug 211794] [CG] Change the UTI of the "WebP" image to be "org.webmproject.webp" : [Attachment 399155] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 211794: [CG] Change the UTI of the "WebP" image to be
"org.webmproject.webp"
https://bugs.webkit.org/show_bug.cgi?id=211794

Attachment 399155: Patch

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




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

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:43
> +	   const char* defaultSupportedImageTypes[] = {

This is replacing a constant array with a non-constant array. Please add
constexpr or a const after the * so this array is constant. I think constexpr
is best.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:62
>	   RetainPtr<CFArrayRef> systemImageTypes =
adoptCF(CGImageSourceCopyTypeIdentifiers());

Why not convert this into a temporary HashSet<String> instead of using it as a
CFArrayRef? Because we are doing to be doing the "contains" operation on it,
and that’s what a set does.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:66
> +	   for (const auto* imageType : defaultSupportedImageTypes) {

We can use "auto", "auto&", "auto*", or "const char*" for this loop variable,
but I don’t think "const auto*" is a great choice.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:67
> +	       RetainPtr<CFStringRef> string =
adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, imageType,
kCFStringEncodingUTF8));

There’s no good reason to use kCFStringEncodingUTF8 here. I suggest using
kCFStringEncodingASCII. But also, lets just make a String directly, and use a
HashSet<String> of the system image types instead of doing array searches.