[webkit-reviews] review denied: [Bug 27215] [QT] suppress (un)desired launcher output that can make layout test to fail with stderr : [Attachment 32796] For Qt, overwrite the plugin directories for the DRT to only those defined in QTWEBKIT_PLUGIN_PATH

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Kenneth Rohde Christiansen
<kenneth.christiansen at openbossa.org>'s request for review:
Bug 27215: [QT] suppress (un)desired launcher output that can make layout test
to fail with stderr
https://bugs.webkit.org/show_bug.cgi?id=27215

Attachment 32796: For Qt,overwrite the plugin directories for the DRT to only
those defined in QTWEBKIT_PLUGIN_PATH
https://bugs.webkit.org/attachment.cgi?id=32796&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
I like the approach of the patch and think this will work, but a few problems:

> -PluginDatabase* PluginDatabase::installedPlugins()
> +PluginDatabase* PluginDatabase::installedPlugins(bool populate)

This reason for this populate arg should be documented in code as it is not
obvious.

> +void PluginDatabase::clear()
> +{
> +    m_plugins.clear();
> +    m_pluginsByPath.clear();
> +    m_registeredMIMETypes.clear();
> +}

Sure, but no deletion needed?

> +	   void setPluginDirectories(const Vector<String>& directories)
> +	   {
> +	       clear();
> +	       m_pluginDirectories = directories;
> +	   }

Looks suspect as you're changing the behavior of this method.  Where is this
method currently called from?  Any obvious side effects?
  
> +	   bool m_pluginSetChanged;

Adding a member variable that is never used or initialized?

> +void QWEBKIT_EXPORT qt_drt_overwritePluginDirectories()
> +{
> +    PluginDatabase *db = PluginDatabase::installedPlugins(/* populate */
false);

The decorator is in wrong place according to webkit coding style guidelines.

With these things fixed I think it'll be in good shape.

Cheers,
Adam