Bug 49497 – getMIMEType(s)ForExtension should consult system mapping

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

MIMETypeRegistry::getMediaMIMETypeForExtension and MIMETypeRegistry::getMediaMIMETypesForExtension only look in the hard coded mapping table, but should also include the type returned by the system specific function MIMETypeRegistry::getMIMETypeForExtension.

[Eric Carlson]

Created attachment 73831 [details]
Proposed patch

[John Sullivan]

Comment on attachment 73831 [details]
Proposed patch

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

If the system-specific registry has priority, shouldn't it be consulted first in getMediaMIMETypesForExtension()?

> WebCore/platform/MIMETypeRegistry.cpp:364
> +        typeList.append(*mediaMIMETypeMap().get(ext));

Would it be faster to put the result of mediaMIMETypeMap().get() in a local variable and then test it for null, rather than calling both contains() and get()? (I realize this pattern already existed in this function.)

[Darin Adler]

Comment on attachment 73831 [details]
Proposed patch

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

> WebCore/platform/MIMETypeRegistry.cpp:349
> +    String type = MIMETypeRegistry::getMIMETypeForExtension(ext);

No need to explicitly qualify with MIMETypeRegistry since we are already in a function member of MIMETypeRegistry.

> WebCore/platform/MIMETypeRegistry.cpp:366
> +    String type = MIMETypeRegistry::getMIMETypeForExtension(ext);

No need to explicitly qualify with MIMETypeRegistry since we are already in a function member of MIMETypeRegistry.

[Eric Carlson]

(In reply to comment #2)
> (From update of attachment 73831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73831&action=review
> 
> If the system-specific registry has priority, shouldn't it be consulted first in
> getMediaMIMETypesForExtension()?
> 
The first type in the vector returned by mediaMIMETypeMap() is the system-specific type (if there is one). I will update the patch to return immediately if the type map entry exists.

> > WebCore/platform/MIMETypeRegistry.cpp:364
> > +        typeList.append(*mediaMIMETypeMap().get(ext));
> 
> Would it be faster to put the result of mediaMIMETypeMap().get() in a local variable and then test 
> it for null, rather than calling both contains() and get()? (I realize this pattern already existed in 
> this function.)
>
Good point, thanks.

(In reply to comment #3)
> (From update of attachment 73831 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73831&action=review
> 
> > WebCore/platform/MIMETypeRegistry.cpp:349
> > +    String type = MIMETypeRegistry::getMIMETypeForExtension(ext);
> 
> No need to explicitly qualify with MIMETypeRegistry since we are already in a function member 
> of MIMETypeRegistry.
> 
> > WebCore/platform/MIMETypeRegistry.cpp:366
> > +    String type = MIMETypeRegistry::getMIMETypeForExtension(ext);
> 
> No need to explicitly qualify with MIMETypeRegistry since we are already in a function member 
> of MIMETypeRegistry.
>
Indeed, thanks!

[Eric Carlson]

Created attachment 73961 [details]
Updated patch

[Eric Seidel (no email)]

Attachment 73961 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5985077

[Early Warning System Bot]

Attachment 73961 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5938081

[Eric Carlson]

Created attachment 73962 [details]
Updated patch (and it compiles!)

[Eric Carlson]

http://trac.webkit.org/changeset/72119