[webkit-reviews] review denied: [Bug 24988] The Webkit plugin mime type matching code should allow the wildcards : [Attachment 29194] Updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ananta Iyengar <ananta at chromium.org>'s
request for review:
Bug 24988: The Webkit plugin mime type matching code should allow the wildcards
https://bugs.webkit.org/show_bug.cgi?id=24988

Attachment 29194: Updated patch
https://bugs.webkit.org/attachment.cgi?id=29194&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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?