Summary: | The Webkit plugin mime type matching code should allow the wildcards | ||
---|---|---|---|
Product: | WebKit | Reporter: | Ananta Iyengar <ananta> |
Component: | Plug-ins | Assignee: | Ananta Iyengar <ananta> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | ahmad.saleem792, ap, rniwa |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
Attachments: |
Description
Ananta Iyengar
2009-04-01 09:43:04 PDT
Created attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code
Comment on attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code
Will upload a new patch.
Created attachment 29194 [details]
Updated patch
Comment on attachment 29194 [details] Updated patch > -bool PluginData::supportsMimeType(const String& mimeType) const > +bool PluginData::supportsMimeType(const String& mimeType, bool allowWildCard) const I suggest two separate functions rather than adding a boolean argument to this function. The boolean is extremely unclear at the call site. You see a "false" or "true" and have no idea what it means, requiring a comment at the call site. Instead the version that doesn't allow wild cards could be named supportsMimeTypeWithoutConsideringWildCards, perhaps. Other aspects of this patch look good. There's a tab in the ChangeLog too. That should be fixed. Comment on attachment 29194 [details] Updated patch > + if (m_mimes[i]->type == mimeType) { > + return true; > + } else if (allowWildCard && m_mimes[i]->type == "*") { > return true; > + } Single line if statements bodies don't get braces in the WebKit coding style (documented in the style document). No else after return in the WebKit coding style. > + PluginPackage* pluginPackageSupportingWildcards = NULL; WebKit coding style uses 0, not NULL. > + // We check for exact mime type matches and wildcard (*) matches. > + if ((mimeiter->first == key) || (mimeiter->first == "*")) We normally don't use the extra parentheses as done here. > - if (pluginChoices.isEmpty()) > + if (pluginChoices.isEmpty()) { > return 0; > + } This changes code that's correct according to the WebKit style to a different style, perhaps one you prefer. But we have a standard you should adhere to. > } else if (fileName() == "npmozax.dll") > // Bug 15217: Mozilla ActiveX control complains about missing xpcom_core.dll > return true; > + else if (fileName() == "npnul32.dll") > + // We don't want to load the Mozilla NULL plugin, which is used to install > + // NPAPI plugins. > + return true; These multiple line bodies need braces to fit the style. Both the npmozax.dll case that's already. I don't think the name of the plug-in is "NULL". You can call it the "null plug-in" instead of the "NULL plugin". > - return pluginData && !type.isEmpty() && pluginData->supportsMimeType(type); > + // We pass in true to the supportsMimeType function in PluginData to allow > + // plugins which support the wildcard mime type * to be instantiated. > + return pluginData && !type.isEmpty() && pluginData->supportsMimeType(type, true); We want to avoid having to write two-line comments just to explain the boolean, so please take my suggestion about not using a boolean. Do we want other styles of wildcard to work, such as "text/*", or is it just "*" that we need? Comment on attachment 29194 [details]
Updated patch
Will upload a new patch addressing the comments on this one.
Created attachment 29206 [details]
Updated patch
Hi Darin
Thanks for the review. I have uploaded a new patch which addresses your comments. We only need to match the * mime type on the same lines as Firefox.
Thanks
Ananta
Comment on attachment 29206 [details]
Updated patch
Will upload a new patch emulating the wildcard plugin matching behavior on the Mac.
Created attachment 29209 [details]
Updated patch with the wildcard mime type matching behavior on the mac.
Hi Darin
I have uploaded a new patch containing similar changes on the Mac plugin branch.
Thanks
Ananta
Comment on attachment 29209 [details] Updated patch with the wildcard mime type matching behavior on the mac. > - if ([[[plugin performSelector:enumeratorSelector] allObjects] containsObject:key]) { > + if ([[[plugin performSelector:enumeratorSelector] allObjects] containsObject:key] || > + [[[plugin performSelector:enumeratorSelector] allObjects] containsObject:@"*"]) { This is inefficient. We don't want to call allObjects twice. We should put the result of allObjects into a local variable and then use it twice. Created attachment 29221 [details]
Updated patch with review comments addressed
I have uploaded a new patch which addresses the comments on the earlier patch.
-Ananta
Created attachment 29244 [details]
Updated patch with the layout test LayoutTests/plugins/plugin-javascript-access.html rebaselined
Hi Darin
Please take a look at the updated patch. I had to rebaseline the LayoutTests/plugins/plugin-javascript-access.html file as we support an additional mime type * in the layout test plugin.
Thanks
Ananta
How about the slightly shorter name supportsMimeTypeIgnoringWildCards instead of supportsMimeTypeWithoutConsideringWildCards? nit: "mimeiter" should probably be "mimeIter" or maybe just "iter" otherwise, LGTM Created attachment 29320 [details]
Updated patch with review comments addressed
Landed as: http://trac.webkit.org/changeset/42321 Reverted because layout tests broke http://build.webkit.org/builders/Tiger%20Intel%20Release/builds/205/steps/layout-test/logs/stdio http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/163/steps/layout-test/logs/stdio http://build.webkit.org/builders/Windows%20Release%20%28Tests%29/builds/46/steps/layout-test/logs/stdio http://build.webkit.org/builders/Windows%20Debug%20%28Tests%29/builds/52/steps/layout-test/logs/stdio The revert was http://trac.webkit.org/changeset/42324 Comment on attachment 29320 [details]
Updated patch with review comments addressed
Cleared r+ to move out of commit queue (since it was landed and then backed out due to layout tests regressions).
Comment on attachment 29206 [details]
Updated patch
Clear r+ to move out of commit queue.
NPAPI Plugin support has been removed from Safari 14 onward (and for JAVA, it was dropped in 12). Further, Webkit Plugin is not support since Safari 5.1 (based on below link). Can this be marked as "RESOLVED WONTFIX"? If I am wrong, please ignore my comment. Thanks! Link - https://developer.apple.com/library/archive/documentation/InternetWeb/Conceptual/WebKit_PluginProgTopic/WebKitPluginTopics.html Although - if I search Webkit GitHub, I do see "PluginData.cpp": https://github.com/WebKit/WebKit/blob/c68178ddc01939b4bce3cdb9718c62087649dad4/Source/WebCore/plugins/PluginData.cpp and "SupportsMIMEType" etc. Is it something which can be enhanced via this patch in current code? If yes, please ignore my "WONTFIX" suggestion. Thanks! |