[webkit-reviews] review denied: [Bug 88535] [Qt][WK2] Scanning plugins blocks the UI for a long time : [Attachment 146290] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 13 03:53:44 PDT 2012

Simon Hausmann <hausmann at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 88535: [Qt][WK2] Scanning plugins blocks the UI for a long time

Attachment 146290: Patch

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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
, 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

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

More information about the webkit-reviews mailing list