[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]

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
https://bugs.webkit.org/show_bug.cgi?id=88535

Attachment 146290: Patch
https://bugs.webkit.org/attachment.cgi?id=146290&action=review

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