See https://developers.google.com/speed/webp/docs/riff_container.
<rdar://problem/63031187>
Created attachment 399154 [details] Patch
Created attachment 399155 [details] Patch
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.
Created attachment 399187 [details] Patch
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.
Created attachment 399188 [details] Patch
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.
Comment on attachment 399188 [details] Patch Please consider the review comments I attached to an earlier version of this patch.
Created attachment 399208 [details] Patch
Committed r261594: <https://trac.webkit.org/changeset/261594> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399208 [details].