Bug 208038

Created attachment 391354 [details]
Patch

Created attachment 391364 [details]
Patch

Comment on attachment 391364 [details]
Patch

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

This patch looks good.

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:56
> +        "com.google.webp"_s,

Is this OK for older mac, iOS versions? If not, then compile time guards are needed.

Created attachment 392307 [details]
Patch

Comment on attachment 391364 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:56
>> +        "com.google.webp"_s,
> 
> Is this OK for older mac, iOS versions? If not, then compile time guards are needed.

You are right. But since WebP will be supported in future macOS/iOS releases I think we do not need to add compile time guards here. I think #if HAVE(WEBP) is enough but I will add both "public.webp" and "com.google.webp" to this list and take whatever CG supports.

<rdar://problem/59977846>

Comment on attachment 392307 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:67
> +            if (CFArrayContainsValue(systemImageTypes.get(), CFRangeMake(0, count), string.get()))

This is linear search. How big is systemImageTypes?

Comment on attachment 392307 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:67
>> +            if (CFArrayContainsValue(systemImageTypes.get(), CFRangeMake(0, count), string.get()))
> 
> This is linear search. How big is systemImageTypes?

Between 50 and 60 items. The problem is the items of this array are not sorted. So I can't use CFArrayBSearchValues().

Comment on attachment 392307 [details]
Patch

Clearing flags on attachment: 392307

Committed r257805: <https://trac.webkit.org/changeset/257805>

All reviewed patches have been landed.  Closing bug.

Comment on attachment 392307 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:59
> +        const String defaultSupportedImageTypes[] = {
> +            "com.compuserve.gif"_s,
> +            "com.microsoft.bmp"_s,
> +            "com.microsoft.cur"_s,
> +            "com.microsoft.ico"_s,
> +            "public.jpeg"_s,
> +            "public.png"_s,
> +            "public.tiff"_s,
>  #if !PLATFORM(WIN)
> -        "public.jpeg-2000"_s,
> -        "public.mpo-image"_s,
> +            "public.jpeg-2000"_s,
> +            "public.mpo-image"_s,
>  #endif
>  #if HAVE(WEBP)
> -        "public.webp"_s,
> +            "public.webp"_s,
> +            "com.google.webp"_s,
>  #endif
> -    };
> +        };

This is ok as-is. No change needed. A more efficient solution would use std::array<const char*,...> or just make this a const char* array with a nullptr for the last entry because:

1. No need to stack-allocate and initialize String, which does a strlen() <-- well, ASCIILiteral does that
2. Because of (1) there is a reduced memory footprint during the call.

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

This is ok as-is. No change needed. A more efficient solution would be to copy the array (not a deep copy) then sort it and binary search it below because:

1. Reduces linear search to logarithmic search <-- especially good if the array gets over a 100 or more items in the future.

This patch looks good.