Bug 102667

Summary: [Qt] Lookup mimetypes using QMimeDatabase
Product: WebKit Reporter: Allan Sandfeld Jensen <allan.jensen>
Component: Page LoadingAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, hausmann, japhet, ossy, webkit.review.bot
Priority: P2    
Version: 420+   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 103069    
Bug Blocks: 103054, 103056    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch hausmann: review+

Allan Sandfeld Jensen
Reported 2012-11-19 02:21:05 PST
With the recent addition of QMimeDatabase in QtCore we can get rid of the static table of mimetypes and extensions which is constantly in risk of becoming outdated and instead use QMimeDatabase which uses the system mimetype database if available. Note that to begin with this should probably be restricted to filename detection only. Detection by content has historical had some security issues due to some loaders not verifying against context. Benifits should be better detection of downloaded files (offers of applications), and ability to recognize common aliases for supported mimetypes.
Attachments
Patch (14.12 KB, patch)
2012-11-19 02:28 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (18.34 KB, patch)
2012-11-19 04:59 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (19.09 KB, patch)
2012-11-19 05:24 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (19.53 KB, patch)
2012-11-19 05:37 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (18.41 KB, patch)
2012-11-22 03:22 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (18.57 KB, patch)
2012-11-22 03:28 PST, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (6.87 KB, patch)
2012-11-22 04:38 PST, Allan Sandfeld Jensen 		hausmann: review+
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1 2012-11-19 02:28:54 PST
Created attachment 174926 [details] Patch
Simon Hausmann
Comment 2 2012-11-19 04:02:38 PST
Comment on attachment 174926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174926&action=review There's a lot of cross-platform code changed. Is there a way to avoid that and make our MIMETypeRegistry.cpp behave more like the other platforms? > Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp:42 > +inline static QMimeDatabase& mimeDatabase() > +{ > + DEFINE_STATIC_LOCAL(QMimeDatabase, s_mimeDatabase, ()); > + return s_mimeDatabase; > +} I don't think you need this optimization. AFAICS QMimeDatabase is really cheap to construct because it internally uses exactly the same pattern already.
Allan Sandfeld Jensen
Comment 3 2012-11-19 04:37:19 PST
(In reply to comment #2) > (From update of attachment 174926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174926&action=review > > There's a lot of cross-platform code changed. Is there a way to avoid that and make our MIMETypeRegistry.cpp behave more like the other platforms? > We still behave the same for most parts. The only behaviour change is the attempt to detect multi-part suffixes (.tar.gz), and this code was changed so that other platforms can do the same if they want, but it is kind of a Unix tradition only. > > Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp:42 > > +inline static QMimeDatabase& mimeDatabase() > > +{ > > + DEFINE_STATIC_LOCAL(QMimeDatabase, s_mimeDatabase, ()); > > + return s_mimeDatabase; > > +} > > I don't think you need this optimization. AFAICS QMimeDatabase is really cheap to construct because it internally uses exactly the same pattern already. Why aren't the QMimeDatabase method static then?
Allan Sandfeld Jensen
Comment 4 2012-11-19 04:59:39 PST
Created attachment 174959 [details] Patch Removed static QMimeDatabase optimization, ensured WK2 uses the same logic as WK1 and added one mime-type feature from Qt backend
Build Bot
Comment 5 2012-11-19 05:15:28 PST
Comment on attachment 174959 [details] Patch Attachment 174959 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14910178
Allan Sandfeld Jensen
Comment 6 2012-11-19 05:24:18 PST
Created attachment 174960 [details] Patch Update exported symbols
Build Bot
Comment 7 2012-11-19 05:31:56 PST
Comment on attachment 174960 [details] Patch Attachment 174960 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14905192
Allan Sandfeld Jensen
Comment 8 2012-11-19 05:37:56 PST
Created attachment 174961 [details] Patch Update exported symbols
Allan Sandfeld Jensen
Comment 9 2012-11-22 03:22:54 PST
Created attachment 175637 [details] Patch Use cross-platform mimetype detection in File.cpp. Added another missing function in the Qt implementation to detect if extensions are valid for a given mimetype, and used that to set suggestedFilename
Allan Sandfeld Jensen
Comment 10 2012-11-22 03:28:40 PST
Created attachment 175638 [details] Patch Forgot to commit the last two lines
Allan Sandfeld Jensen
Comment 11 2012-11-22 04:38:04 PST
Created attachment 175645 [details] Patch Split patch in three. This is the first part only detecting by last extension but doing so using QMimeDatabase
Simon Hausmann
Comment 12 2012-11-22 04:56:00 PST
Comment on attachment 175645 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175645&action=review r=me with comments. Nice to see the list of hardcoded types finally go away! > Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp:41 > + const String filename = "filename." + ext.lower(); Since the only thing we do is to take the filename and convert it (allocatingly) to a QString, I suppose you could also construct a QString here right away. > Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp:56 > + return "application/octet-stream"; Why not return defaultMIMEType() here and safe the string construction? :)
Allan Sandfeld Jensen
Comment 13 2012-11-22 05:24:24 PST
Committed r135507: <http://trac.webkit.org/changeset/135507>
Csaba Osztrogonác
Comment 14 2012-11-22 07:51:40 PST
new bug report to track the regression caused by this patch - https://bugs.webkit.org/show_bug.cgi?id=103069
Note You need to log in before you can comment on or make changes to this bug.