Bug 27651 - [Qt] QWebPluginDatabase API
: [Qt] QWebPluginDatabase API
Status: RESOLVED LATER
: WebKit
WebKit Qt
: 528+ (Nightly build)
: Other All
: P2 Enhancement
Assigned To:
:
: Qt
:
:
  Show dependency treegraph
 
Reported: 2009-07-24 06:51 PST by
Modified: 2009-11-08 16:23 PST (History)


Attachments
When clearing the plugin database, clear also the timestamp map. (1.12 KB, patch)
2009-07-24 06:56 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Allow to enable/disable particular plugin packages. (4.02 KB, patch)
2009-07-24 07:02 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Allow to explicitly choose a preferred plugin for a mimetype. (4.12 KB, patch)
2009-07-24 07:10 PST, Jakub Wieczorek
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Expose the PluginDatabase::pluginForMIMEType() function as public API. (1.63 KB, patch)
2009-07-24 07:15 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Expose the default plugin directories and the current directory set of the plugin database as public API. (1.53 KB, patch)
2009-07-24 07:19 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Fix style in PluginPackage and PluginDatabase. (6.55 KB, patch)
2009-07-24 07:26 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add QWebPluginDatabase class to the Qt API. (31.53 KB, patch)
2009-07-24 07:30 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add QWebPluginDatabase class to the Qt API (with API docs). (36.18 KB, patch)
2009-07-29 02:30 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Allow to explicitly choose a preferred plugin for a mimetype - r2. (4.61 KB, patch)
2009-07-29 12:16 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Allow to explicitly choose a preferred plugin for a mimetype - r3. (5.81 KB, patch)
2009-07-30 03:43 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add normalizePath() function to the FileSystem files. (5.72 KB, patch)
2009-07-31 13:01 PST, Jakub Wieczorek
eric: review-
Review Patch | Details | Formatted Diff | Diff
Don't clear the plugin database when changing search paths. (5.92 KB, patch)
2009-07-31 13:01 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add QWebPluginDatabase class to the Qt API - r3. (38.66 KB, patch)
2009-08-01 13:34 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Remove the private classes from QWebPluginDatabase. (12.91 KB, patch)
2009-08-12 04:47 PST, Jakub Wieczorek
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Optimize the QWebPluginInfo::supportsMimeType() function. (1.98 KB, patch)
2009-08-12 04:49 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Optimize the QWebPluginInfo::mimeTypes() function. (6.77 KB, patch)
2009-08-12 04:50 PST, Jakub Wieczorek
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Move the PluginDatabase::setPluginDirectories() function definition out of the header. (2.95 KB, patch)
2009-08-12 04:53 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add arePathsEqual() function to the FileSystem files. (7.18 KB, patch)
2009-08-15 08:32 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Don't clear the plugin database when changing search paths. (5.10 KB, patch)
2009-08-15 09:06 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Remove the private classes from QWebPluginDatabase. (11.06 KB, patch)
2009-09-05 04:53 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Remove the private classes from QWebPluginDatabase. (12.96 KB, patch)
2009-09-05 04:59 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Optimize the QWebPluginInfo::mimeTypes() function. (5.94 KB, patch)
2009-09-05 05:26 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Optimize the QWebPluginInfo::mimeTypes() function. (5.95 KB, patch)
2009-09-05 05:30 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add normalizePathForComparison() function to platform/FileSystem. (7.22 KB, patch)
2009-09-06 01:51 PST, Jakub Wieczorek
no flags Review Patch | Details | Formatted Diff | Diff
Add normalizePathForComparison() function to platform/FileSystem. (7.18 KB, patch)
2009-09-06 02:21 PST, Jakub Wieczorek
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Don't clear the plugin database when changing search paths. (4.94 KB, patch)
2009-09-06 02:23 PST, Jakub Wieczorek
hausmann: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-24 06:51:16 PST
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.
------- Comment #1 From 2009-07-24 06:56:25 PST -------
Created an attachment (id=33435) [details]
When clearing the plugin database, clear also the timestamp map.
------- Comment #2 From 2009-07-24 07:02:31 PST -------
Created an attachment (id=33436) [details]
Allow to enable/disable particular plugin packages.
------- Comment #3 From 2009-07-24 07:10:51 PST -------
Created an attachment (id=33438) [details]
Allow to explicitly choose a preferred plugin for a mimetype.
------- Comment #4 From 2009-07-24 07:15:22 PST -------
Created an attachment (id=33440) [details]
Expose the PluginDatabase::pluginForMIMEType() function as public API.
------- Comment #5 From 2009-07-24 07:19:05 PST -------
Created an attachment (id=33441) [details]
Expose the default plugin directories and the current directory set of the plugin database as public API.
------- Comment #6 From 2009-07-24 07:26:06 PST -------
Created an attachment (id=33442) [details]
Fix style in PluginPackage and PluginDatabase.
------- Comment #7 From 2009-07-24 07:30:19 PST -------
Created an attachment (id=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 #8 From 2009-07-27 14:22:11 PST -------
(From update of attachment 33435 [details])
Landed.
------- Comment #9 From 2009-07-28 02:30:40 PST -------
(From update of attachment 33435 [details])
Clearing review on this one as it has been landed
------- Comment #10 From 2009-07-28 11:04:42 PST -------
(From update of attachment 33442 [details])
Looks fine.
------- Comment #11 From 2009-07-28 13:31:15 PST -------
Fix style in PluginPackage and PluginDatabase is landed in 46499.

Please someone remove the r+ as I'm not authorized to do so.
------- Comment #12 From 2009-07-29 02:30:16 PST -------
Created an attachment (id=33704) [details]
Add QWebPluginDatabase class to the Qt API (with API docs).
------- Comment #13 From 2009-07-29 05:39:48 PST -------
(From update of attachment 33442 [details])
Clearing review.
------- Comment #14 From 2009-07-29 06:34:21 PST -------
(From update of attachment 33436 [details])
> +        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 #15 From 2009-07-29 07:38:03 PST -------
https://bugs.webkit.org/attachment.cgi?id=33436 was landed in 46540. Please clear flags as I do not have access to do so.
------- Comment #16 From 2009-07-29 07:41:37 PST -------
(From update of attachment 33440 [details])
r=me
------- Comment #17 From 2009-07-29 07:41:58 PST -------
(From update of attachment 33441 [details])
r=me
------- Comment #18 From 2009-07-29 07:44:55 PST -------
https://bugs.webkit.org/attachment.cgi?id=33438

You introduce a new hashmap, should be cleared in clear(). r- if I could review :-)
------- Comment #19 From 2009-07-29 07:49:49 PST -------
(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.
------- Comment #20 From 2009-07-29 07:55:53 PST -------
https://bugs.webkit.org/attachment.cgi?id=33440 landed in 46541.
------- Comment #21 From 2009-07-29 10:09:41 PST -------
https://bugs.webkit.org/attachment.cgi?id=33441 landed in 46546
------- Comment #22 From 2009-07-29 12:16:21 PST -------
Created an attachment (id=33730) [details]
Allow to explicitly choose a preferred plugin for a mimetype - r2.

(In reply to comment #19)
> (From update of attachment 33438 [details] [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 #23 From 2009-07-30 03:43:16 PST -------
Created an attachment (id=33766) [details]
Allow to explicitly choose a preferred plugin for a mimetype - r3.

ChangeLog.
------- Comment #24 From 2009-07-30 05:51:07 PST -------
(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?

The rest looks good to me :-)
------- Comment #25 From 2009-07-30 05:57:59 PST -------
(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);

?
------- Comment #26 From 2009-07-31 13:01:24 PST -------
Created an attachment (id=33894) [details]
Add normalizePath() function to the FileSystem files.
------- Comment #27 From 2009-07-31 13:01:52 PST -------
Created an attachment (id=33895) [details]
Don't clear the plugin database when changing search paths.
------- Comment #28 From 2009-08-01 13:34:10 PST -------
Created an attachment (id=33941) [details]
Add QWebPluginDatabase class to the Qt API - r3.

Update the patch with more test cases.
------- Comment #29 From 2009-08-03 13:00:16 PST -------
(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 33766 [details] [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 #30 From 2009-08-03 14:34:32 PST -------
(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==.

> +/*!
> +    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 #31 From 2009-08-03 14:46:47 PST -------
(From update of attachment 33895 [details])
Clearing review as I want to improve the patch a bit.
------- Comment #32 From 2009-08-06 19:27:50 PST -------
Simon, didn't you actually land this last patch? Please clear tag then.
------- Comment #33 From 2009-08-06 20:37:37 PST -------
(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. :)
------- Comment #34 From 2009-08-07 00:52:39 PST -------
(From update of attachment 33941 [details])
Clearing review after landing (Thanks Kenneth :)
------- Comment #35 From 2009-08-12 04:47:50 PST -------
Created an attachment (id=34653) [details]
Remove the private classes from QWebPluginDatabase.
------- Comment #36 From 2009-08-12 04:49:43 PST -------
Created an attachment (id=34654) [details]
Optimize the QWebPluginInfo::supportsMimeType() function.
------- Comment #37 From 2009-08-12 04:50:59 PST -------
Created an attachment (id=34655) [details]
Optimize the QWebPluginInfo::mimeTypes() function.
------- Comment #38 From 2009-08-12 04:53:44 PST -------
Created an attachment (id=34656) [details]
Move the PluginDatabase::setPluginDirectories() function definition out of the header.
------- Comment #39 From 2009-08-15 06:53:33 PST -------
(In reply to comment #30)
> (From update of attachment 33941 [details] [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.
------- Comment #40 From 2009-08-15 08:32:53 PST -------
Created an attachment (id=34896) [details]
Add arePathsEqual() function to the FileSystem files.

(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'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 #41 From 2009-08-15 09:06:23 PST -------
Created an attachment (id=34899) [details]
Don't clear the plugin database when changing search paths.
------- Comment #42 From 2009-09-03 12:08:11 PST -------
(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 :)
------- Comment #43 From 2009-09-03 12:10:24 PST -------
(From update of attachment 34654 [details])
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 #44 From 2009-09-03 12:13:13 PST -------
(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?
------- Comment #45 From 2009-09-03 12:19:34 PST -------
(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 :)
------- Comment #46 From 2009-09-05 04:53:23 PST -------
Created an attachment (id=39110) [details]
Remove the private classes from QWebPluginDatabase.

(In reply to comment #42)
> (From update of attachment 34653 [details] [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.
------- Comment #47 From 2009-09-05 04:59:59 PST -------
Created an attachment (id=39111) [details]
Remove the private classes from QWebPluginDatabase.

ChangeLog.
------- Comment #48 From 2009-09-05 05:26:48 PST -------
Created an attachment (id=39112) [details]
Optimize the QWebPluginInfo::mimeTypes() function.

(In reply to comment #44)
> (From update of attachment 34655 [details] [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. :)
------- Comment #49 From 2009-09-05 05:30:26 PST -------
Created an attachment (id=39113) [details]
Optimize the QWebPluginInfo::mimeTypes() function.
------- Comment #50 From 2009-09-06 01:51:11 PST -------
Created an attachment (id=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] [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
------- Comment #51 From 2009-09-06 02:21:23 PST -------
Created an attachment (id=39121) [details]
Add normalizePathForComparison() function to platform/FileSystem.

Changed the stub implementations to return an unnormalized path instead of an empty string.
------- Comment #52 From 2009-09-06 02:23:52 PST -------
Created an attachment (id=39122) [details]
Don't clear the plugin database when changing search paths.
------- Comment #53 From 2009-09-07 06:15:42 PST -------
(From update of attachment 39111 [details])
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 #54 From 2009-09-07 06:27:42 PST -------
(From update of attachment 39113 [details])
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 #55 From 2009-09-07 07:15:14 PST -------
(From update of attachment 39111 [details])
Clearing flags on attachment: 39111

Committed r48112: <http://trac.webkit.org/changeset/48112>
------- Comment #56 From 2009-09-07 07:22:00 PST -------
(From update of attachment 39113 [details])
Clearing flags on attachment: 39113

Committed r48113: <http://trac.webkit.org/changeset/48113>
------- Comment #57 From 2009-09-07 07:22:06 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #58 From 2009-09-07 13:15:20 PST -------
Re-opening as Jakub has two more patches up for review in this bug :)
------- Comment #59 From 2009-09-08 11:56:06 PST -------
(From update of attachment 39121 [details])
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 #60 From 2009-09-27 14:16:45 PST -------
(From update of attachment 39122 [details])
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!
------- Comment #61 From 2009-10-05 10:54:00 PST -------
Please stop adding new patches to this bug... it's getting out of control. :)  Generally we try to do one change per bug.
------- Comment #62 From 2009-10-19 05:14:16 PST -------
(From update of attachment 39122 [details])
Rejecting patch 39122 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39122 from bug 27651 failed to download and apply.
------- Comment #63 From 2009-10-19 16:57:46 PST -------
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.
------- Comment #64 From 2009-11-08 10:42:24 PST -------
Any thoughts from Jakub?  It's been nearly 3 weeks.
------- Comment #65 From 2009-11-08 11:28:25 PST -------
(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.
------- Comment #66 From 2009-11-08 16:23:30 PST -------
Closing per Jakub's direction.