Bug 88535 – [Qt][WK2] Scanning plugins blocks the UI for a long time

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Balazs Kelemen]

This is sad, since one of the main benefits of WebKit2 is the availability of an always responsive UI. A relatively simple solution for this is a custom persistent cache where we store the meta data of plugins (supported mime types, etc.). This is what we did in WebCore back in time under the NETSCAPE_PLUGIN_METADATA_CACHE flag. I am going to implement something similar for Qt-WK2.

[Balazs Kelemen]

Created attachment 146290 [details]
Patch

[Simon Hausmann]

Comment on attachment 146290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146290&action=review

Looks good in general, I think this is a good approach. But I think the cacheFile() implementation and usage should be improved, in particular I think we should use the same directory that we use for other storage/cache related things in QtWebKit.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:57
> +static QFileInfo cacheFile()

It appears to me that the callers of this function do only three things with the return value:

    1) Call filePath() and the construct a QFile object
    2) Call exists() to check if the file exists
    3) Call QFile::remove on the filePath() to remove the file

In the light of that it seems to me that a better return type for this function would be a PassOwnPtr<QFile> object, so that the code becomes simpler:

    QFile file(cacheFile().filePath());
    file.open(...);

becomes

    OwnPtr<QFile> file(cacheFile());
    file->open();

and

    if (cacheFile().exists())

becomes

    if (cacheFile()->exists())

and

    QFile::remove(cacheFile().filePath());

becomes

    cacheFile()->remove();

However please feel absolutely free to ignore this suggestion :)

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:61
> +    return QFileInfo(QDir(QStandardPaths::writableLocation(QStandardPaths::CacheLocation)), QLatin1String("plugin_meta_data.json"));

For other storage items we use CacheLocation + ".QtWebKit/", see WebContextQt.cpp. I think code from there should be re-used in determining the base directory. That also includes the path creation and I think it would make the function a lot cheaper. In some functions below it's called repeatedly as if it were very cheap to call, but depending on the backend the implementation of writableLocation(CacheLocation) may be more expensive than it seems.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:99
> +        isWritable = QDir::root().mkpath(cacheFileInfo.dir().path());

Wouldn't it be easier here to use absolutePath() instead of .dir().path() ?

Also I think using the return value if mkpath() has no effect here because in line 101 the value is discarded with the result of the open() call.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:101
> +    isWritable = file.open(QFile::WriteOnly) && file.isWritable();

Shouldn't this be WriteOnly | Truncate, to handle the case where the file exists already?

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:137
> +static MetaDataResult::Tag tryReadPluginMetaDataFromCacheFile(QString canonicalPluginPath, RawPluginMetaData& result)

QString canonicalPluginPath -> const QString& canonicalPluginPath

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:140
> +    if (!cacheFile().exists())
> +        return MetaDataResult::NotAvailable;

I think readMetaDataFromCacheFile will fail accordingly if the file doesn't exist (open() will fail), and therefore you can avoid this check and call to cacheFile().

This way in the common case of success the cacheFile() function is called only one single time.

[Balazs Kelemen]

Created attachment 147310 [details]
Patch

[Balazs Kelemen]

Comment on attachment 147310 [details]
Patch

Clearing flags on attachment: 147310

Committed r120205: <http://trac.webkit.org/changeset/120205>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.