Bug 111435

Summary: [Qt][WK2] Specify storage paths through the C API
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Severity: Normal CC: jturcotte
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 108475, 112223, 121022    
Bug Blocks: 109564    
Description Flags
Patch hausmann: review+

Description Jocelyn Turcotte 2013-03-05 06:44:14 PST
[Qt][WK2] Specify storage paths through the C API
Comment 1 Jocelyn Turcotte 2013-03-05 07:15:50 PST
Created attachment 191494 [details]
Comment 2 Jocelyn Turcotte 2013-03-05 07:17:10 PST
This follows the input given in bug 109455.
Comment 3 Simon Hausmann 2013-03-06 06:01:46 PST
Comment on attachment 191494 [details]

Comment 4 Benjamin Poulain 2013-03-07 14:10:27 PST
Comment on attachment 191494 [details]

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


> 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());
Comment 5 Jocelyn Turcotte 2013-03-11 06:40:02 PDT
Created attachment 192457 [details]

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.
Comment 6 Simon Hausmann 2013-03-11 07:07:59 PDT
Comment on attachment 192457 [details]

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.
Comment 7 Jocelyn Turcotte 2013-03-12 04:18:18 PDT
Committed r145517: <http://trac.webkit.org/changeset/145517>