[webkit-reviews] review denied: [Bug 32196] [Qt] createPlugin() should decide whether Qt Plugins can be created when pluginsEnabled is false : [Attachment 44363] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied robert <robert at roberthogan.net>'s
request for review:
Bug 32196: [Qt] createPlugin() should decide whether Qt Plugins can be created
when pluginsEnabled is false
https://bugs.webkit.org/show_bug.cgi?id=32196

Attachment 44363: Patch
https://bugs.webkit.org/attachment.cgi?id=44363&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This patch should be written in a more platform-independent fashion with fewer
#if statements.

> +#include <wtf/Platform.h>

There's no need to add this include. Platform.h is already included, as you can
tell from the use of the #if ENABLE in the file. It's almost never necessary to
add new includes of Platform.h.

> -	   if (!settings || !settings->arePluginsEnabled() || 
> +	   if (!settings ||
> +#if PLATFORM(QT)
> +	      // Qt clients 'disable' Qt plugins by not implementing them
> +	      (!settings->arePluginsEnabled() &&
!MIMETypeRegistry::isQtPluginMIMEType(mimeType)) ||
> +#else
> +	      !settings->arePluginsEnabled() ||
> +#endif

Instead of calling this isQtPluginMIMEType, it should be isClientPluginMIMEType
and should be called on all platforms. On non-Qt ones it can just return false.


> +#include <wtf/Platform.h>

No need for this.

> +#if PLATFORM(QT)
> +    // Check to see if a mime type is a valid Java applet mime type
> +    static bool isQtPluginMIMEType(const String& mimeType);
> +#endif

Comment here is wrong (copied from Java).

Then we need versions of isClientPluginMIMEType that return false on all the
non-Qt platforms.

I'm not sure "client plug-in" is the best name for this concept, so you can use
a better one if you can think of it.