[webkit-reviews] review granted: [Bug 27651] [Qt] QWebPluginDatabase API : [Attachment 33941] Add QWebPluginDatabase class to the Qt API - r3.

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Jakub Wieczorek
<faw217 at gmail.com>'s request for review:
Bug 27651: [Qt] QWebPluginDatabase API
https://bugs.webkit.org/show_bug.cgi?id=27651

Attachment 33941: Add QWebPluginDatabase class to the Qt API - r3.
https://bugs.webkit.org/attachment.cgi?id=33941&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
r=me. There's a few things that would be nice to clean up in follow-up patches.


> +bool QWebPlugin::MimeType::operator==(const MimeType& other) const
> +{
> +    return name == other.name
> +	      && description == other.description
> +	      && fileExtensions == other.fileExtensions;
> +}

We really should also have an operator!= if we offer operator==.

> +/*!
> +    Constructs a null QWebPlugin.
> +*/
> +QWebPlugin::QWebPlugin()
> +    : d(new QWebPluginPrivate(0))
> +{
> +}

It would be nice if this class could be constructed without a malloc, i.e. if
the null QWebPlugin would be null due to it's null d pointer. That makes it
cheaper and avoids unnecessary mallocs.
In fact I would even go as far as having a private d-pointer but also the
plugin pointer directly as member, similar to QWebElement, to reduce the number
of mallocs even more.

> +/*!
> +    Returns a list of MIME types that are supported by the plugin.
> +
> +    \sa supportsMimeType()
> +*/
> +QList<QWebPlugin::MimeType> QWebPlugin::mimeTypes() const
> +{
> +    if (!d->plugin)
> +	   return QList<MimeType>();
> +
> +    QList<MimeType> mimeTypes;
> +    const MIMEToDescriptionsMap& mimeToDescriptions =
d->plugin->mimeToDescriptions();
> +    MIMEToDescriptionsMap::const_iterator end = mimeToDescriptions.end();
> +    for (MIMEToDescriptionsMap::const_iterator it =
mimeToDescriptions.begin(); it != end; ++it) {
> +	   MimeType mimeType;
> +	   mimeType.name = it->first;
> +	   mimeType.description = it->second;
> +
> +	   QStringList fileExtensions;
> +	   Vector<String> extensions =
d->plugin->mimeToExtensions().get(mimeType.name);
> +
> +	   for (unsigned i = 0; i < extensions.size(); ++i)
> +	       fileExtensions.append(extensions[i]);
> +
> +	   mimeType.fileExtensions = fileExtensions;
> +	   mimeTypes.append(mimeType);
> +    }
> +
> +    return mimeTypes;
> +}

It might be worth mentioning in the docs that this method is not as fast as it
seems...


> +
> +/*!
> +    Returns true if the plugin supports a specific \a mimeType, false
otherwise.
> +
> +    \sa mimeTypes()
> +*/
> +bool QWebPlugin::supportsMimeType(const QString& mimeType) const
> +{
> +    QList<MimeType> types = mimeTypes();
> +    foreach (const MimeType& type, types) {
> +	   if (type.name == mimeType)
> +	       return true;
> +    }

Would it be possible to implement this more efficiently? It seems expensive to
call mimeTypes() every time.

> +/*!
> +    Enables or disables the plugin.
> +
> +    Disabled plugins will not be picked up by WebKit when looking for a
plugin supporting
> +    a particular MIME type.
> +
> +    \sa isEnabled()
> +*/
> +void QWebPlugin::setEnabled(bool enabled)
> +{
> +    if (!d->plugin)
> +	   return;
> +    d->plugin->setEnabled(enabled);
> +}

Shouldn't the docs mention that refresh() needs to be called afterwards? But
perhaps I'm missing something :)