Bug 24988 - The Webkit plugin mime type matching code should allow the wildcards
Summary: The Webkit plugin mime type matching code should allow the wildcards
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ananta Iyengar
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-01 09:43 PDT by Ananta Iyengar
Modified: 2022-06-23 18:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch for supporting wildcards in the plugin mime type matching code (14.53 KB, patch)
2009-04-01 11:28 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff
Updated patch (13.62 KB, patch)
2009-04-02 10:22 PDT, Ananta Iyengar
darin: review-
Details | Formatted Diff | Diff
Updated patch (12.68 KB, patch)
2009-04-02 13:25 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff
Updated patch with the wildcard mime type matching behavior on the mac. (14.26 KB, patch)
2009-04-02 17:36 PDT, Ananta Iyengar
darin: review-
Details | Formatted Diff | Diff
Updated patch with review comments addressed (14.26 KB, patch)
2009-04-02 23:07 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff
Updated patch with the layout test LayoutTests/plugins/plugin-javascript-access.html rebaselined (14.86 KB, patch)
2009-04-03 14:46 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff
Updated patch with review comments addressed (15.15 KB, patch)
2009-04-07 16:28 PDT, Ananta Iyengar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ananta Iyengar 2009-04-01 09:43:04 PDT
The Webkit plugin mime type matching code should allow plugins to specify the wildcard mime type (*) on the same lines as Firefox and Chromium. Firefox uses the npnul32 plugin which specifies its mime type as * for NPAPI plugin installation. Chromium does the same. The wildcard mime type is also useful for NPAPI spy applications, which would be very useful for debugging.
Comment 1 Ananta Iyengar 2009-04-01 11:28:28 PDT
Created attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code
Comment 2 Ananta Iyengar 2009-04-02 10:21:18 PDT
Comment on attachment 29166 [details]
Patch for supporting wildcards in the plugin mime type matching code

Will upload a new patch.
Comment 3 Ananta Iyengar 2009-04-02 10:22:03 PDT
Created attachment 29194 [details]
Updated patch
Comment 4 Darin Adler 2009-04-02 10:32:08 PDT
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.
Comment 5 Darin Adler 2009-04-02 10:32:40 PDT
There's a tab in the ChangeLog too. That should be fixed.
Comment 6 Darin Adler 2009-04-02 10:36:42 PDT
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 7 Ananta Iyengar 2009-04-02 13:23:40 PDT
Comment on attachment 29194 [details]
Updated patch

Will upload a new patch addressing the comments on this one.
Comment 8 Ananta Iyengar 2009-04-02 13:25:15 PDT
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 9 Ananta Iyengar 2009-04-02 17:35:08 PDT
Comment on attachment 29206 [details]
Updated patch

Will upload a new patch emulating the wildcard plugin matching behavior on the Mac.
Comment 10 Ananta Iyengar 2009-04-02 17:36:25 PDT
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 11 Darin Adler 2009-04-02 17:38:34 PDT
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.
Comment 12 Ananta Iyengar 2009-04-02 23:07:13 PDT
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
Comment 13 Ananta Iyengar 2009-04-03 14:46:09 PDT
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
Comment 14 Darin Fisher (:fishd, Google) 2009-04-07 15:15:08 PDT
How about the slightly shorter name supportsMimeTypeIgnoringWildCards instead of supportsMimeTypeWithoutConsideringWildCards?

nit: "mimeiter" should probably be "mimeIter" or maybe just "iter"

otherwise, LGTM
Comment 15 Ananta Iyengar 2009-04-07 16:28:04 PDT
Created attachment 29320 [details]
Updated patch with review comments addressed
Comment 16 Darin Fisher (:fishd, Google) 2009-04-08 09:05:27 PDT
Landed as:  http://trac.webkit.org/changeset/42321
Comment 18 Darin Fisher (:fishd, Google) 2009-04-08 10:09:31 PDT
The revert was http://trac.webkit.org/changeset/42324
Comment 19 David Levin 2009-04-08 16:48:46 PDT
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 20 David Levin 2009-04-08 16:51:18 PDT
Comment on attachment 29206 [details]
Updated patch

Clear r+ to move out of commit queue.
Comment 21 Ahmad Saleem 2022-06-23 04:14:07 PDT
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!