Bug 111435 – [Qt][WK2] Specify storage paths through the C API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jocelyn Turcotte]

[Qt][WK2] Specify storage paths through the C API

[Jocelyn Turcotte]

Created attachment 191494 [details]
Patch

[Jocelyn Turcotte]

This follows the input given in bug 109455.

[Simon Hausmann]

Comment on attachment 191494 [details]
Patch

LGTM

[Benjamin Poulain]

Comment on attachment 191494 [details]
Patch

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

No problem with this per se, but I am unhappy there are both C APIs and platformDefault. It would be good if we could get rid of platformDefault for everyone.

Okay for this patch on WebKit2.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:66
> +    QString cachePath = QStandardPaths::writableLocation(QStandardPaths::CacheLocation);
> +    ASSERT(!cachePath.isEmpty());

I don't know if the user has any control over QStandardPaths::writableLocation.

If there is any way for cachePath.isEmpty() to be empty due to a user mistake, it would be good to have a fall back in addition to the assertion.

> Source/WebKit2/UIProcess/qt/QtWebContext.cpp:161
> +    Q_ASSERT(!path.isEmpty());

ditto

> Source/WebKit2/UIProcess/qt/QtWebIconDatabaseClient.cpp:57
> +    WKContextSetIconDatabasePath(toAPI(context), adoptWK(WKStringCreateWithQString(QtWebContext::preparedStoragePath(QtWebContext::IconDatabaseStorage))).get());

I would split this line to have
    [...]
    WKContextSetIconDatabasePath((toAPI(context), path.get());

[Jocelyn Turcotte]

Created attachment 192457 [details]
Patch

Added empty cache file path handling in PluginProcessProxyQt.cpp.
I'd prefer to keep the assert instead of handling an empty data location in QtWebContext since it complexifies the code a lot more, and there is no way to test this code, so I'd prefer to see if it is really needed before. From my research in the QStandardPaths implementation, there should always be a non-empty path returned for DataLocation and CacheLocation.

[Simon Hausmann]

Comment on attachment 192457 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).
> +

Missing sign-off line :). Please fix before landing.

[Jocelyn Turcotte]

Committed r145517: <http://trac.webkit.org/changeset/145517>