[webkit-reviews] review granted: [Bug 147309] [Cocoa] canPlayType('audio/mpeg; codecs="mp3"') returns "" : [Attachment 409050] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 147309: [Cocoa] canPlayType('audio/mpeg; codecs="mp3"') returns ""
https://bugs.webkit.org/show_bug.cgi?id=147309

Attachment 409050: Patch

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




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

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

> Source/WebCore/platform/graphics/MIMETypeCache.cpp:145
> +    if (type.containerType().convertToASCIILowercase() == "audio/mpeg") {

Can be more efficient to use equalLettersIgnoringASCIICase or
equalIgnoringASCIICase in a case like this.

> Source/WebCore/platform/graphics/MIMETypeCache.h:60
> +    Optional<bool> overrideExtendedType(const ContentType&);

This should be a static member function; it is a rule, not something  based on
the contents of the cache itself. Or it could be a free function not mentioned
in the header at all.

Sam has pointed out that Optional<bool> is confusing and often reminds us not
to use it. We don’t *really* need it here. Have false mean don’t override,
since we’re not using false at this time.

The name of this function has the form we try to avoid where it sounds like it
might be the caller telling the cache to override, but really we are asking if
we should override. So it could be more like shouldOverrideExtendedType?