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.
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.
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
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.
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.
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. :)
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 :-)
(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);
?
(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? :)
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 :)
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. :)
(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.
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.
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 :)
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 :)
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?
(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 :)
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.
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. :)
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
Created attachment 39121[details]
Add normalizePathForComparison() function to platform/FileSystem.
Changed the stub implementations to return an unnormalized path instead of an empty string.
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.
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 :)
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 :)
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!
(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.
2009-07-24 06:56 PDT, Jakub Wieczorek
2009-07-24 07:02 PDT, Jakub Wieczorek
2009-07-24 07:10 PDT, Jakub Wieczorek
2009-07-24 07:15 PDT, Jakub Wieczorek
2009-07-24 07:19 PDT, Jakub Wieczorek
2009-07-24 07:26 PDT, Jakub Wieczorek
2009-07-24 07:30 PDT, Jakub Wieczorek
2009-07-29 02:30 PDT, Jakub Wieczorek
2009-07-29 12:16 PDT, Jakub Wieczorek
2009-07-30 03:43 PDT, Jakub Wieczorek
2009-07-31 13:01 PDT, Jakub Wieczorek
2009-07-31 13:01 PDT, Jakub Wieczorek
2009-08-01 13:34 PDT, Jakub Wieczorek
2009-08-12 04:47 PDT, Jakub Wieczorek
2009-08-12 04:49 PDT, Jakub Wieczorek
2009-08-12 04:50 PDT, Jakub Wieczorek
2009-08-12 04:53 PDT, Jakub Wieczorek
2009-08-15 08:32 PDT, Jakub Wieczorek
2009-08-15 09:06 PDT, Jakub Wieczorek
2009-09-05 04:53 PDT, Jakub Wieczorek
2009-09-05 04:59 PDT, Jakub Wieczorek
2009-09-05 05:26 PDT, Jakub Wieczorek
2009-09-05 05:30 PDT, Jakub Wieczorek
2009-09-06 01:51 PDT, Jakub Wieczorek
2009-09-06 02:21 PDT, Jakub Wieczorek
2009-09-06 02:23 PDT, Jakub Wieczorek
commit-queue: commit-queue-