Bug 49497 - getMIMEType(s)ForExtension should consult system mapping
Summary: getMIMEType(s)ForExtension should consult system mapping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-13 12:23 PST by Eric Carlson
Modified: 2010-11-16 10:47 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (1.98 KB, patch)
2010-11-13 12:39 PST, Eric Carlson
sullivan: review-
Details | Formatted Diff | Diff
Updated patch (2.93 KB, patch)
2010-11-15 21:53 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (and it compiles!) (2.36 KB, patch)
2010-11-15 22:07 PST, Eric Carlson
sullivan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-11-13 12:23:15 PST
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.
Comment 1 Eric Carlson 2010-11-13 12:39:14 PST
Created attachment 73831 [details]
Proposed patch
Comment 2 John Sullivan 2010-11-15 10:42:22 PST
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.)
Comment 3 Darin Adler 2010-11-15 13:11:23 PST
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.
Comment 4 Eric Carlson 2010-11-15 16:49:23 PST
(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!
Comment 5 Eric Carlson 2010-11-15 21:53:29 PST
Created attachment 73961 [details]
Updated patch
Comment 6 Eric Seidel (no email) 2010-11-15 21:58:01 PST
Attachment 73961 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5985077
Comment 7 Early Warning System Bot 2010-11-15 22:04:12 PST
Attachment 73961 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5938081
Comment 8 Eric Carlson 2010-11-15 22:07:22 PST
Created attachment 73962 [details]
Updated patch (and it compiles!)
Comment 9 Eric Carlson 2010-11-16 10:47:12 PST
http://trac.webkit.org/changeset/72119