Bug 27651 – [Qt] QWebPluginDatabase API

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit Qt

[Jakub Wieczorek]

Following https://bugs.webkit.org/show_bug.cgi?id=27210 and after the decision made during IRC discussions, I wrote a new class, QWebPluginDatabase, that provides a basic API for managing Netscape plugins picked up by WebKit. 

Currently it allows:
- getting a list of plugins picked up by WebKit, with information about supported MIME types
- changing the search paths
- disabling particular plugins
- choosing a preferred plugin for a MIME type

Some of the features required changes in the PluginDatabase and PluginPackage classes, hence I've separated the general patches from the Qt-specific one.

[Jakub Wieczorek]

Created attachment 33435 [details]
When clearing the plugin database, clear also the timestamp map.

[Jakub Wieczorek]

Created attachment 33436 [details]
Allow to enable/disable particular plugin packages.

[Jakub Wieczorek]

Created attachment 33438 [details]
Allow to explicitly choose a preferred plugin for a mimetype.

[Jakub Wieczorek]

Created attachment 33440 [details]
Expose the PluginDatabase::pluginForMIMEType() function as public API.

[Jakub Wieczorek]

Created attachment 33441 [details]
Expose the default plugin directories and the current directory set of the plugin database as public API.

[Jakub Wieczorek]

Created attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.

[Jakub Wieczorek]

Created attachment 33443 [details]
Add QWebPluginDatabase class to the Qt API.

And this is the main, Qt-specific patch that adds the QWebPluginDatabase API.

It's not ready for review yet, first it needs some more API review and it needs docs.

[Adam Treat]

Comment on attachment 33435 [details]
When clearing the plugin database, clear also the timestamp map.

Landed.

[Simon Hausmann]

Comment on attachment 33435 [details]
When clearing the plugin database, clear also the timestamp map.

Clearing review on this one as it has been landed

[Eric Seidel (no email)]

Comment on attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.

Looks fine.

[Kenneth Rohde Christiansen]

Fix style in PluginPackage and PluginDatabase is landed in 46499.

Please someone remove the r+ as I'm not authorized to do so.

[Jakub Wieczorek]

Created attachment 33704 [details]
Add QWebPluginDatabase class to the Qt API (with API docs).

[Jakub Wieczorek]

Comment on attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.

Clearing review.

[Adam Treat]

Comment on attachment 33436 [details]
Allow to enable/disable particular plugin packages.

> +        bool isEnabled() const { return m_isEnabled; }
> +        void setEnabled(bool enabled);

Minor nit: "Leave meaningless variable names out of function declarations."  from style guide...

r+ with this minor change which can be made upon landing.

[Kenneth Rohde Christiansen]

https://bugs.webkit.org/attachment.cgi?id=33436 was landed in 46540. Please clear flags as I do not have access to do so.

[Simon Hausmann]

Comment on attachment 33440 [details]
Expose the PluginDatabase::pluginForMIMEType() function as public API.

r=me

[Simon Hausmann]

Comment on attachment 33441 [details]
Expose the default plugin directories and the current directory set of the plugin database as public API.

r=me

[Kenneth Rohde Christiansen]

https://bugs.webkit.org/attachment.cgi?id=33438

You introduce a new hashmap, should be cleared in clear(). r- if I could review :-)

[Simon Hausmann]

Comment on attachment 33438 [details]
Allow to explicitly choose a preferred plugin for a mimetype.

The patch looks good, but I think there's one bug left: The m_preferredPlugins hash keeps PluginPackage instances alife, through the use of RefPtr. When calling clear() as well as refresh() we may have to free the references to these plugins, to allow the pluginpackage objects to be freed.

[Kenneth Rohde Christiansen]

https://bugs.webkit.org/attachment.cgi?id=33440 landed in 46541.

[Kenneth Rohde Christiansen]

https://bugs.webkit.org/attachment.cgi?id=33441 landed in 46546

[Jakub Wieczorek]

Created attachment 33730 [details]
Allow to explicitly choose a preferred plugin for a mimetype - r2.

(In reply to comment #19)
> (From update of attachment 33438 [details])
> The patch looks good, but I think there's one bug left: The m_preferredPlugins
> hash keeps PluginPackage instances alife, through the use of RefPtr. When
> calling clear() as well as refresh() we may have to free the references to
> these plugins, to allow the pluginpackage objects to be freed.

Fixed, thanks. :)

[Jakub Wieczorek]

Created attachment 33766 [details]
Allow to explicitly choose a preferred plugin for a mimetype - r3.

ChangeLog.

[Simon Hausmann]

Comment on attachment 33766 [details]
Allow to explicitly choose a preferred plugin for a mimetype - r3.

r=me

+    if (plugin && !plugin->mimeToExtensions().contains(mimeType))

I would formulate this one the other way around:

if (!plugin || plugin->mimeToExtension().contains(mimeType))
    return;

What do you think?

The rest looks good to me :-)

[Jakub Wieczorek]

(In reply to comment #24)
> (From update of attachment 33766 [details])
> r=me
> 
> +    if (plugin && !plugin->mimeToExtensions().contains(mimeType))
> 
> I would formulate this one the other way around:
> 
> if (!plugin || plugin->mimeToExtension().contains(mimeType))
>     return;
> 
> What do you think?

That was not the purpose. We want to set the plugin only if it supports the given mime type or is null (because that will reset the setting).

Or did you mean:

if (!plugin || plugin->mimeToExtension().contains(mimeType))
    m_preferredPlugins.set(mimeType.lower(), plugin);

?

[Jakub Wieczorek]

Created attachment 33894 [details]
Add normalizePath() function to the FileSystem files.

[Jakub Wieczorek]

Created attachment 33895 [details]
Don't clear the plugin database when changing search paths.

[Jakub Wieczorek]

Created attachment 33941 [details]
Add QWebPluginDatabase class to the Qt API - r3.

Update the patch with more test cases.

[Simon Hausmann]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 33766 [details] [details])
> > r=me
> > 
> > +    if (plugin && !plugin->mimeToExtensions().contains(mimeType))
> > 
> > I would formulate this one the other way around:
> > 
> > if (!plugin || plugin->mimeToExtension().contains(mimeType))
> >     return;
> > 
> > What do you think?
> 
> That was not the purpose. We want to set the plugin only if it supports the
> given mime type or is null (because that will reset the setting).
> 
> Or did you mean:
> 
> if (!plugin || plugin->mimeToExtension().contains(mimeType))
>     m_preferredPlugins.set(mimeType.lower(), plugin);
> 
> ?

Ahh, yes and no. I think what I found strange was that it would allow for a null plugin pointer to be passed, which doesn't seem intentional. Perhaps it would make sense to separate the null pointer check from the extension check, i.e. make the method return if (!plugin) regardless. Does that make any sense? :)

[Simon Hausmann]

Comment on attachment 33941 [details]
Add QWebPluginDatabase class to the Qt API - r3.

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 :)

[Jakub Wieczorek]

Comment on attachment 33895 [details]
Don't clear the plugin database when changing search paths.

Clearing review as I want to improve the patch a bit.

[Kenneth Rohde Christiansen]

Simon, didn't you actually land this last patch? Please clear tag then.

[Eric Seidel (no email)]

Comment on attachment 33894 [details]
Add normalizePath() function to the FileSystem files.

This seems OK.  Maybe it should be called normalizePathForComparison if it's only for comparison.  It's unclear what "normalize" should actually do?  Does it resolve symlinks?  Does it remove traililing '/' chars?  Does it return the ~1 windows form of paths?  r- for more explanation and or a better name...

In general I'm in favor of the concept though.  Another way would be to to just have a arePathsEqual function. :)

[Simon Hausmann]

Comment on attachment 33941 [details]
Add QWebPluginDatabase class to the Qt API - r3.

Clearing review after landing (Thanks Kenneth :)

[Jakub Wieczorek]

Created attachment 34653 [details]
Remove the private classes from QWebPluginDatabase.

[Jakub Wieczorek]

Created attachment 34654 [details]
Optimize the QWebPluginInfo::supportsMimeType() function.

[Jakub Wieczorek]

Created attachment 34655 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

[Jakub Wieczorek]

Created attachment 34656 [details]
Move the PluginDatabase::setPluginDirectories() function definition out of the header.

[Jakub Wieczorek]

(In reply to comment #30)
> (From update of attachment 33941 [details])
> 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==.

Already fixed in http://trac.webkit.org/changeset/46763.

> 
> > +/*!
> > +    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.

Done.

> 
> > +/*!
> > +    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...

Right. It's now optimized.

> 
> 
> > +
> > +/*!
> > +    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.

Done.

> 
> > +/*!
> > +    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 :)

refresh() doesn't need to be called afterwards.

Disabling plugins has only effect on the searching stage, i.e. when WebKit picks a plugin for a given type, at which point this setting is checked. It is not cached so the database doesn't need to be refreshed.

[Jakub Wieczorek]

Created attachment 34896 [details]
Add arePathsEqual() function to the FileSystem files.

(In reply to comment #33)
> (From update of attachment 33894 [details])
> This seems OK.  Maybe it should be called normalizePathForComparison if it's
> only for comparison.  It's unclear what "normalize" should actually do?  Does
> it resolve symlinks?  Does it remove traililing '/' chars?  Does it return the
> ~1 windows form of paths?  r- for more explanation and or a better name...
> 
> In general I'm in favor of the concept though.  Another way would be to to just
> have a arePathsEqual function. :)

I'm not so sure myself what normalizePath() should exactly do. :) Definitely remove a trailing slash, strip trailing whitespaces, resolve relative subpaths (e.g. /foo/bar/../baz)... Not necessarily resolve symbolic links. Not sure what else and what directory separators it should use.

I gave the arePathsEqual() concept a try. It seems to be a good idea to leave the comparison part up to the ports so they can decide what they should do before comparing the strings.

[Jakub Wieczorek]

Created attachment 34899 [details]
Don't clear the plugin database when changing search paths.

[Simon Hausmann]

Comment on attachment 34653 [details]
Remove the private classes from QWebPluginDatabase.

Jakub, the patch looks great to me, except for one _tiny_ buglet. I'll r+ it if you
could leave the d pointers in there. Add the m_plugin, m_database, etc. members, that's
great. But keep the (forward declared) d-pointers, too, even if they're unused. _If_ 
something goes wrong we can still extend with the d-pointer and keep binary compatibility.

Sorry :)

[Simon Hausmann]

Comment on attachment 34654 [details]
Optimize the QWebPluginInfo::supportsMimeType() function.

r=me. This patch won't apply as-is as it relies on the earlier d-pointer -> m_package change, which needs a minor tweak. Otherwise this is good stuff :)

[Simon Hausmann]

Comment on attachment 34655 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

Hmm, wouldn't it be better to do this on-demand by having m_mimeTypes mutable? Otherwise it becomes much more expensive to construct a QWebPluginInfo object even if you never call mimeTypes() on it?

[Simon Hausmann]

(In reply to comment #33)
> (From update of attachment 33894 [details])
> This seems OK.  Maybe it should be called normalizePathForComparison if it's
> only for comparison.  It's unclear what "normalize" should actually do?  Does
> it resolve symlinks?  Does it remove traililing '/' chars?  Does it return the
> ~1 windows form of paths?  r- for more explanation and or a better name...
> 
> In general I'm in favor of the concept though.  Another way would be to to just
> have a arePathsEqual function. :)

I admit I prefer a function that does one thing, normalize a path by for example removing redundant pieces. arePathsEqual does two things, normalize and then compare. Unless the other operating systems have one function for both I can imagine that all the FileSystemXXX.cpp implementations are going to add an internal function to normalize and then do a string comparision :)

[Jakub Wieczorek]

Created attachment 39110 [details]
Remove the private classes from QWebPluginDatabase.

(In reply to comment #42)
> (From update of attachment 34653 [details])
> Jakub, the patch looks great to me, except for one _tiny_ buglet. I'll r+ it if
> you
> could leave the d pointers in there. Add the m_plugin, m_database, etc.
> members, that's
> great. But keep the (forward declared) d-pointers, too, even if they're unused.
> _If_ 
> something goes wrong we can still extend with the d-pointer and keep binary
> compatibility.
> 
> Sorry :)

No problem, I agree with that.

Done.

[Jakub Wieczorek]

Created attachment 39111 [details]
Remove the private classes from QWebPluginDatabase.

ChangeLog.

[Jakub Wieczorek]

Created attachment 39112 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

(In reply to comment #44)
> (From update of attachment 34655 [details])
> Hmm, wouldn't it be better to do this on-demand by having m_mimeTypes mutable?
> Otherwise it becomes much more expensive to construct a QWebPluginInfo object
> even if you never call mimeTypes() on it?

Right, makes sense to me. :)

[Jakub Wieczorek]

Created attachment 39113 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

[Jakub Wieczorek]

Created attachment 39120 [details]
Add normalizePathForComparison() function to platform/FileSystem.

(In reply to comment #45)
> (In reply to comment #33)
> > (From update of attachment 33894 [details] [details])
> > This seems OK.  Maybe it should be called normalizePathForComparison if it's
> > only for comparison.  It's unclear what "normalize" should actually do?  Does
> > it resolve symlinks?  Does it remove traililing '/' chars?  Does it return the
> > ~1 windows form of paths?  r- for more explanation and or a better name...
> > 
> > In general I'm in favor of the concept though.  Another way would be to to just
> > have a arePathsEqual function. :)
> 
> I admit I prefer a function that does one thing, normalize a path by for
> example removing redundant pieces. arePathsEqual does two things, normalize and
> then compare. Unless the other operating systems have one function for both I
> can imagine that all the FileSystemXXX.cpp implementations are going to add an
> internal function to normalize and then do a string comparision :)

Alright, I got back to the old patch and renamed the function to normalizePathForComparison(), according to Eric's suggestion.
For Qt it uses QDir::cleanPath(). If other ports could use something similar, it should be sufficient.
http://doc.trolltech.com/4.5/qdir.html#cleanPath

[Jakub Wieczorek]

Created attachment 39121 [details]
Add normalizePathForComparison() function to platform/FileSystem.

Changed the stub implementations to return an unnormalized path instead of an empty string.

[Jakub Wieczorek]

Created attachment 39122 [details]
Don't clear the plugin database when changing search paths.

[Simon Hausmann]

Comment on attachment 39111 [details]
Remove the private classes from QWebPluginDatabase.

r=me

Come to think of it I realize that this optimization may not be actually worth it for QWebPluginDatabase itself, as it's a singleton. But for QWebPluginInfo it's worth it I think.

[Simon Hausmann]

Comment on attachment 39113 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

Looks good to me, r=me

You _could_ probably formulate the QString("%1=%2").arg(s1).arg(s2) stuff easier by writing first.name() + "=" + second.name(), but that's minor :)

[Eric Seidel (no email)]

Comment on attachment 39111 [details]
Remove the private classes from QWebPluginDatabase.

Clearing flags on attachment: 39111

Committed r48112: <http://trac.webkit.org/changeset/48112>

[Eric Seidel (no email)]

Comment on attachment 39113 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

Clearing flags on attachment: 39113

Committed r48113: <http://trac.webkit.org/changeset/48113>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Re-opening as Jakub has two more patches up for review in this bug :)

[Simon Hausmann]

Comment on attachment 39121 [details]
Add normalizePathForComparison() function to platform/FileSystem.

As briefly discussed on the IRC, the patch looks good to me, but it needs a clear explanation of the expected semantics. The other port maintainers should feel encouraged to implement this function, knowing _why_ and _what_ to replace "notImplemented()" with :)

[Simon Hausmann]

Comment on attachment 39122 [details]
Don't clear the plugin database when changing search paths.

r=me, looks good to me. It's sad that this is an virtually un-layouttest-able codepath ;(

However landing this patch requires approval of the earlier normalizePathForComparision first!

[Eric Seidel (no email)]

Please stop adding new patches to this bug... it's getting out of control. :)  Generally we try to do one change per bug.

[WebKit Commit Bot]

Comment on attachment 39122 [details]
Don't clear the plugin database when changing search paths.

Rejecting patch 39122 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39122 from bug 27651 failed to download and apply.

[Eric Seidel (no email)]

Jakub will need to post a new patch.  Once we land this patch this bug should be closed and a new one used for further issues.

[Eric Seidel (no email)]

Any thoughts from Jakub?  It's been nearly 3 weeks.

[Jakub Wieczorek]

(In reply to comment #64)
> Any thoughts from Jakub?  It's been nearly 3 weeks.

Sorry for being silent about this bug for the last weeks.

The remaining patch was quite relevant for the particular Qt API but that has been made private for the next release of QtWebKit. I don't think it is essential for any other ports at all? The current version of the patch adds some additional assumptions, which currently the plugin loading process is free of. Moreover, as Simon has pointed out, this code path is hardly testable with the current testing capabilities. Therefore, I'm not sure if it is worth it. I'll try to get back to this one in some future, hopefully with a better solution.

For now, the bug can be closed.

[Eric Seidel (no email)]

Closing per Jakub's direction.