[webkit-reviews] review granted: [Bug 147309] [Cocoa] canPlayType('audio/mpeg; codecs="mp3"') returns "" : [Attachment 409050] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 17 11:58:15 PDT 2020
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?
More information about the webkit-reviews
mailing list