Summary: | Attribute lost with Flash plugin without mime type set | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nicolas Dufresne <nicolas> | ||||||||
Component: | Plug-ins | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, aestes, andersca, ap, aroben, commit-queue, eric, gustavo, japhet, jhoneycutt, mrobinson, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | 52288 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Nicolas Dufresne
2010-12-07 16:57:16 PST
So it sounds like this should affect Safari on Windows. (In reply to comment #1) > So it sounds like this should affect Safari on Windows. Right, it should if you don't already have the extension/mime information in your win registy. IIRC the windows Flash plugin installer adds an entry to it, so probably that no windows users has been affected so far. Created attachment 76140 [details]
Patch
Seems sane to me. Comment on attachment 76140 [details]
Patch
Presumably we'll need to do something similar for WebKit2?
(In reply to comment #5) > (From update of attachment 76140 [details]) > Presumably we'll need to do something similar for WebKit2? Best way to know is to test it. The patch contains a unit test so you can check what happens on platforms running WebKit2 (I'm not doing WebKit2 dev atm). I just realized the patch is missing the expected results, will update soon ... Created attachment 76888 [details]
Patch
Comment on attachment 76888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76888&action=review > LayoutTests/plugins/no-mime-with-valid-extension-expected.txt:2 > +This test checks that bug 50568 is fixed. It runs logSrc test as found in the test plugin which prints the src attribute to stdout. If the bug is present, the logSrc attrbitute that triggers the test will be lost and nothing will be displayed on screen. Uppon success, this test should display the src attribute. I believe it's "upon" :) > LayoutTests/plugins/no-mime-with-valid-extension.html:16 > +and nothing will be displayed on screen. Uppon success, this test should display the src Here is the original Upon > LayoutTests/plugins/no-mime-with-valid-extension.html:22 > + finishTest(); You could just inline the code instead of making it a function call. :) Created attachment 78531 [details]
patch
- Rebase against current tree reorganisation
- Inline code in test
- Fixed Uppon -> Upon
Comment on attachment 78531 [details]
patch
Forwarding Eric's R+.
Comment on attachment 78531 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=78531&action=review > Source/WebCore/loader/FrameLoader.cpp:972 > +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM) && !PLATFORM(EFL) // Mac has no PluginDatabase, nor does Chromium or EFL > + if (mimeType.isEmpty()) > + mimeType = PluginDatabase::installedPlugins()->MIMETypeForExtension(extension); > +#endif Shouldn't this code be in the relevant platforms' implementations of MIMETypeRegistry so as to not litter cross-platform code with ifdefs? I know you're probably just following a convention already established in this method, but it still seems ugly. (In reply to comment #12) > (From update of attachment 78531 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78531&action=review > > > Source/WebCore/loader/FrameLoader.cpp:972 > > +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM) && !PLATFORM(EFL) // Mac has no PluginDatabase, nor does Chromium or EFL > > + if (mimeType.isEmpty()) > > + mimeType = PluginDatabase::installedPlugins()->MIMETypeForExtension(extension); > > +#endif > > Shouldn't this code be in the relevant platforms' implementations of MIMETypeRegistry so as to not litter cross-platform code with ifdefs? I know you're probably just following a convention already established in this method, but it still seems ugly. The ifdef are only there because MAC, Chromium and ELF port have decided not to use the plugin database (we can only blame those port). Also, doing it in the paltform specific MIME database implementation is complex and forces plug-ins to be loaded at start instead of on-demand. This was my conclusion after trying it in GTK port. It fully convinced me that this approach is the right one, and it fixes bug for many port at once. (In reply to comment #13) > The ifdef are only there because MAC, Chromium and ELF port have decided not to use the plugin database (we can only blame those port). Also, doing it in the paltform specific MIME database implementation is complex and forces plug-ins to be loaded at start instead of on-demand. This was my conclusion after trying it in GTK port. It fully convinced me that this approach is the right one, and it fixes bug for many port at once. I wasn't suggesting making a change that affects when plugins load. But it does seem that it would be more elegant to have only one call to make when mapping extensions to MIME types. There would be a way to provide a cross-platform implementation (e.g. MIMETypeRegistry.cpp) with exceptions for the ports that need it (e.g. MIMETypeRegistryWin.cpp), so it wouldn't be that much more complex. Also, there are other places that call getMIMETypeForExtension() (createBlobDataForFile() for instance). Is it correct that this function doesn't also consult the plugin database? Could that lead to bugs if a blob is created with an extension for which a MIME type is registered in the plugin database but not the MIMETypeRegistry? I'm not suggesting this patch isn't correct as-is in that it fixes a bug; I'm just thinking of ways it could perhaps be improved as a follow-on. Comment on attachment 78531 [details] patch Clearing flags on attachment: 78531 Committed r75567: <http://trac.webkit.org/changeset/75567> All reviewed patches have been landed. Closing bug. |