Bug 27651 - [Qt] QWebPluginDatabase API
Summary: [Qt] QWebPluginDatabase API
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other All
: P2 Enhancement
Assignee: Jakub Wieczorek
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-07-24 06:51 PDT by Jakub Wieczorek
Modified: 2009-11-08 16:23 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Wieczorek 2009-07-24 06:51:16 PDT
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 Jakub Wieczorek 2009-07-24 06:56:25 PDT
Created attachment 33435 [details]
When clearing the plugin database, clear also the timestamp map.
Comment 2 Jakub Wieczorek 2009-07-24 07:02:31 PDT
Created attachment 33436 [details]
Allow to enable/disable particular plugin packages.
Comment 3 Jakub Wieczorek 2009-07-24 07:10:51 PDT
Created attachment 33438 [details]
Allow to explicitly choose a preferred plugin for a mimetype.
Comment 4 Jakub Wieczorek 2009-07-24 07:15:22 PDT
Created attachment 33440 [details]
Expose the PluginDatabase::pluginForMIMEType() function as public API.
Comment 5 Jakub Wieczorek 2009-07-24 07:19:05 PDT
Created attachment 33441 [details]
Expose the default plugin directories and the current directory set of the plugin database as public API.
Comment 6 Jakub Wieczorek 2009-07-24 07:26:06 PDT
Created attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.
Comment 7 Jakub Wieczorek 2009-07-24 07:30:19 PDT
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 8 Adam Treat 2009-07-27 14:22:11 PDT
Comment on attachment 33435 [details]
When clearing the plugin database, clear also the timestamp map.

Landed.
Comment 9 Simon Hausmann 2009-07-28 02:30:40 PDT
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 10 Eric Seidel (no email) 2009-07-28 11:04:42 PDT
Comment on attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.

Looks fine.
Comment 11 Kenneth Rohde Christiansen 2009-07-28 13:31:15 PDT
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 Jakub Wieczorek 2009-07-29 02:30:16 PDT
Created attachment 33704 [details]
Add QWebPluginDatabase class to the Qt API (with API docs).
Comment 13 Jakub Wieczorek 2009-07-29 05:39:48 PDT
Comment on attachment 33442 [details]
Fix style in PluginPackage and PluginDatabase.

Clearing review.
Comment 14 Adam Treat 2009-07-29 06:34:21 PDT
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 15 Kenneth Rohde Christiansen 2009-07-29 07:38:03 PDT
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 Simon Hausmann 2009-07-29 07:41:37 PDT
Comment on attachment 33440 [details]
Expose the PluginDatabase::pluginForMIMEType() function as public API.

r=me
Comment 17 Simon Hausmann 2009-07-29 07:41:58 PDT
Comment on attachment 33441 [details]
Expose the default plugin directories and the current directory set of the plugin database as public API.

r=me
Comment 18 Kenneth Rohde Christiansen 2009-07-29 07:44:55 PDT
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 Simon Hausmann 2009-07-29 07:49:49 PDT
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.
Comment 20 Kenneth Rohde Christiansen 2009-07-29 07:55:53 PDT
https://bugs.webkit.org/attachment.cgi?id=33440 landed in 46541.
Comment 21 Kenneth Rohde Christiansen 2009-07-29 10:09:41 PDT
https://bugs.webkit.org/attachment.cgi?id=33441 landed in 46546
Comment 22 Jakub Wieczorek 2009-07-29 12:16:21 PDT
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 23 Jakub Wieczorek 2009-07-30 03:43:16 PDT
Created attachment 33766 [details]
Allow to explicitly choose a preferred plugin for a mimetype - r3.

ChangeLog.
Comment 24 Simon Hausmann 2009-07-30 05:51:07 PDT
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 :-)
Comment 25 Jakub Wieczorek 2009-07-30 05:57:59 PDT
(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);

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

Update the patch with more test cases.
Comment 29 Simon Hausmann 2009-08-03 13:00:16 PDT
(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 30 Simon Hausmann 2009-08-03 14:34:32 PDT
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 31 Jakub Wieczorek 2009-08-03 14:46:47 PDT
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.
Comment 32 Kenneth Rohde Christiansen 2009-08-06 19:27:50 PDT
Simon, didn't you actually land this last patch? Please clear tag then.
Comment 33 Eric Seidel (no email) 2009-08-06 20:37:37 PDT
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. :)
Comment 34 Simon Hausmann 2009-08-07 00:52:39 PDT
Comment on attachment 33941 [details]
Add QWebPluginDatabase class to the Qt API - r3.

Clearing review after landing (Thanks Kenneth :)
Comment 35 Jakub Wieczorek 2009-08-12 04:47:50 PDT
Created attachment 34653 [details]
Remove the private classes from QWebPluginDatabase.
Comment 36 Jakub Wieczorek 2009-08-12 04:49:43 PDT
Created attachment 34654 [details]
Optimize the QWebPluginInfo::supportsMimeType() function.
Comment 37 Jakub Wieczorek 2009-08-12 04:50:59 PDT
Created attachment 34655 [details]
Optimize the QWebPluginInfo::mimeTypes() function.
Comment 38 Jakub Wieczorek 2009-08-12 04:53:44 PDT
Created attachment 34656 [details]
Move the PluginDatabase::setPluginDirectories() function definition out of the header.
Comment 39 Jakub Wieczorek 2009-08-15 06:53:33 PDT
(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.
Comment 40 Jakub Wieczorek 2009-08-15 08:32:53 PDT
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 41 Jakub Wieczorek 2009-08-15 09:06:23 PDT
Created attachment 34899 [details]
Don't clear the plugin database when changing search paths.
Comment 42 Simon Hausmann 2009-09-03 12:08:11 PDT
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 43 Simon Hausmann 2009-09-03 12:10:24 PDT
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 44 Simon Hausmann 2009-09-03 12:13:13 PDT
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?
Comment 45 Simon Hausmann 2009-09-03 12:19:34 PDT
(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 :)
Comment 46 Jakub Wieczorek 2009-09-05 04:53:23 PDT
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.
Comment 47 Jakub Wieczorek 2009-09-05 04:59:59 PDT
Created attachment 39111 [details]
Remove the private classes from QWebPluginDatabase.

ChangeLog.
Comment 48 Jakub Wieczorek 2009-09-05 05:26:48 PDT
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. :)
Comment 49 Jakub Wieczorek 2009-09-05 05:30:26 PDT
Created attachment 39113 [details]
Optimize the QWebPluginInfo::mimeTypes() function.
Comment 50 Jakub Wieczorek 2009-09-06 01:51:11 PDT
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
Comment 51 Jakub Wieczorek 2009-09-06 02:21:23 PDT
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 52 Jakub Wieczorek 2009-09-06 02:23:52 PDT
Created attachment 39122 [details]
Don't clear the plugin database when changing search paths.
Comment 53 Simon Hausmann 2009-09-07 06:15:42 PDT
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 54 Simon Hausmann 2009-09-07 06:27:42 PDT
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 55 Eric Seidel (no email) 2009-09-07 07:15:14 PDT
Comment on attachment 39111 [details]
Remove the private classes from QWebPluginDatabase.

Clearing flags on attachment: 39111

Committed r48112: <http://trac.webkit.org/changeset/48112>
Comment 56 Eric Seidel (no email) 2009-09-07 07:22:00 PDT
Comment on attachment 39113 [details]
Optimize the QWebPluginInfo::mimeTypes() function.

Clearing flags on attachment: 39113

Committed r48113: <http://trac.webkit.org/changeset/48113>
Comment 57 Eric Seidel (no email) 2009-09-07 07:22:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 58 Simon Hausmann 2009-09-07 13:15:20 PDT
Re-opening as Jakub has two more patches up for review in this bug :)
Comment 59 Simon Hausmann 2009-09-08 11:56:06 PDT
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 60 Simon Hausmann 2009-09-27 14:16:45 PDT
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!
Comment 61 Eric Seidel (no email) 2009-10-05 10:54:00 PDT
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 WebKit Commit Bot 2009-10-19 05:14:16 PDT
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.
Comment 63 Eric Seidel (no email) 2009-10-19 16:57:46 PDT
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 Eric Seidel (no email) 2009-11-08 10:42:24 PST
Any thoughts from Jakub?  It's been nearly 3 weeks.
Comment 65 Jakub Wieczorek 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 Eric Seidel (no email) 2009-11-08 16:23:30 PST
Closing per Jakub's direction.