Bug 211794 – [CG] Change the UTI of the "WebP" image to be "org.webmproject.webp"

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

See https://developers.google.com/speed/webp/docs/riff_container.

[Said Abou-Hallawa]

<rdar://problem/63031187>

[Said Abou-Hallawa]

Created attachment 399154 [details]
Patch

[Said Abou-Hallawa]

Created attachment 399155 [details]
Patch

[Darin Adler]

Comment on attachment 399155 [details]
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.

[Said Abou-Hallawa]

Created attachment 399187 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 399155 [details]
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.

I changed this to be a HashSet<String>. Instead of adding the imageTypes from an array to a HashSet, the imageTypes will be removed from this HashSet if the system does not support them. And at the end, we are going to return this HashSet.

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

Done.

[Said Abou-Hallawa]

Created attachment 399188 [details]
Patch

[Darin Adler]

Comment on attachment 399187 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:44
> +            "com.compuserve.gif",

If we are building a HashSet<String> from this we should leave the _s on them because that will result in not copying the characters in the strings.

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

Should use auto here instead of writing RetainPtr<CFArrayRef>.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:67
> +            String imageType = String(static_cast<CFStringRef>(const_cast<void*>(value)));

No need for String() here, conversion from CFStringRef works. No need for const_cast, CFStringRef is already a const pointer type.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:69
> +            auto& systemSupportedImageTypes = *static_cast<HashSet<String>*>(context);
> +            systemSupportedImageTypes.add(imageType);

Suggest doing it in one line:

    static_cast<HashSet<String>*>(context)->add(imageType);

If we think that’s not clear enough, we could rename context to untypedSystemSupportedImageTypes.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:72
> +        defaultSupportedImageTypes.removeIf([&systemSupportedImageTypes] (String imageType) {

I think writing "const String&" would make this more efficient.

[Darin Adler]

Comment on attachment 399188 [details]
Patch

Please consider the review comments I attached to an earlier version of this patch.

[Said Abou-Hallawa]

Created attachment 399208 [details]
Patch

[EWS]

Committed r261594: <https://trac.webkit.org/changeset/261594>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399208 [details].