Bug 111435 - [Qt][WK2] Specify storage paths through the C API
Summary: [Qt][WK2] Specify storage paths through the C API
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
Depends on: 108475 112223 121022
Blocks: 109564
  Show dependency treegraph
Reported: 2013-03-05 06:44 PST by Jocelyn Turcotte
Modified: 2013-09-09 02:23 PDT (History)
1 user (show)

See Also:

Patch (17.23 KB, patch)
2013-03-05 07:15 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (20.20 KB, patch)
2013-03-11 06:40 PDT, Jocelyn Turcotte
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>