Bug 88535 - [Qt][WK2] Scanning plugins blocks the UI for a long time
Summary: [Qt][WK2] Scanning plugins blocks the UI for a long time
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
Keywords: Qt, QtTriaged
Depends on:
Reported: 2012-06-07 07:10 PDT by Balazs Kelemen
Modified: 2012-06-13 07:39 PDT (History)
6 users (show)

See Also:

Patch (10.01 KB, patch)
2012-06-07 07:54 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (16.45 KB, patch)
2012-06-13 07:09 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2012-06-07 07:10:49 PDT
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.
Comment 1 Balazs Kelemen 2012-06-07 07:54:42 PDT
Created attachment 146290 [details]
Comment 2 Simon Hausmann 2012-06-13 03:53:44 PDT
Comment on attachment 146290 [details]

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());


    OwnPtr<QFile> file(cacheFile());


    if (cacheFile().exists())


    if (cacheFile()->exists())





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.
Comment 3 Balazs Kelemen 2012-06-13 07:09:11 PDT
Created attachment 147310 [details]
Comment 4 Balazs Kelemen 2012-06-13 07:39:42 PDT
Comment on attachment 147310 [details]

Clearing flags on attachment: 147310

Committed r120205: <http://trac.webkit.org/changeset/120205>
Comment 5 Balazs Kelemen 2012-06-13 07:39:49 PDT
All reviewed patches have been landed.  Closing bug.