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: ASSIGNED
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: 2010-06-10 16:56 PDT (History)
0 users

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.