[webkit-reviews] review granted: [Bug 213826] MIMETypeRegistry::getExtensionsForMIMEType() needs to handle wildcard MIME types : [Attachment 403263] 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 213826: MIMETypeRegistry::getExtensionsForMIMEType() needs to handle
wildcard MIME types
https://bugs.webkit.org/show_bug.cgi?id=213826

Attachment 403263: Patch

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




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

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

Saying r=me, but really concerned that this is all untested.

> Source/WebCore/ChangeLog:3
> +	   MIMETypeRegistry::getExtensionsForMIMEType() needs to handle
wildcard MIME types

Where are the tests?

I think this requires a lot more tests. I am simply guessing that maybe this
code works.

> Source/WebCore/platform/MIMETypeRegistry.h:60
> +    WEBCORE_EXPORT static Vector<String> getExtensionsForMIMEType(const
String& type);

All the "get" in the function names in this class annoy me since that’s not
WebKit coding style, but not new.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:39
> +	   HashMap<String, HashSet<String>> extensionsForMIMETypeMap;

Seems like we could use a shorter name for this, in the context here.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:41
> +	   auto addExtensionForMIMEType = [&](const String& type, const String&
extension) {

Seems like we could use a shorter name for this, maybe just "add".

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:44
> +	       auto addResult = extensionsForMIMETypeMap.ensure(type, []() {
> +		   return HashSet<String>();
> +	       });

Doesn’t seem like we need to use ensure here. I think this is the same as
calling add({ }) and not necessarily more efficient. Can be written as this
one-liner, I think.

    extensionsForMIMETypeMap.add(type, { }).iterator->value.add(extension);

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:48
> +	   auto addExtensionsForMIMEType = [&](const String& type, const
CFArrayRef& extensions) {

Why "const CArrayRef&"? Just CFArrayRef.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:49
> +	       auto wildcardMIMEType = makeString(type.split('/').first(),
"/*"_s);

String::split().first() is an especially inefficient implementation of "get the
part before the first slash". We can be much more efficient by using
StringView::split or StringView::find/StringView::left.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:51
> +	       for (CFIndex i = 0, count = CFArrayGetCount(extensions); i <
count; i++) {

Seems like an Objective-C for loop would be nicer and likely also more
efficient. We keep having to cast back and forth between CF and NS if we use
that, but we already have to cast to convert CFTypeRef to CFStringRef, so it’s
not so bad.

And now I see, looking at the old code, that we used to do that. I think it
would be better.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:53
> +		   CFStringRef extension =
reinterpret_cast<CFStringRef>(CFArrayGetValueAtIndex(extensions, i));

Why reinterpret_cast here and static_cast below?

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:59
> +		   // Add extension to itsmimeType, for example add "png" to
"image/png"

Typo: "itsmimeType".

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:64
> +	   RetainPtr<CFArrayRef> allUTIs =
adoptCF(_UTCopyDeclaredTypeIdentifiers());

auto

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:66
> +	   for (CFIndex i = 0, count = CFArrayGetCount(allUTIs.get()); i <
count; ++i) {

Same thought about Objective-C for loop.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:87
> +    auto iter = extensionsForMIMETypeMap().find(type);

Why "iter"? Lets use a word instead of an abbreviation.

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:89
> +	   extensions.appendRange(iter->value.begin(), iter->value.end());

Isn’t there a more idiomatic way to make a Vector from a HashSet?

> Source/WebCore/platform/cocoa/MIMETypeRegistryCocoa.mm:101
> +    if (type.endsWith('*'))

This seems strangely broad. Why is it right to check only "ends with *"? Is it
really valid to return this result for, say, "text/*"? Seems like a bad idea to
me.