Bug 111435

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

Jocelyn Turcotte
Reported 2013-03-05 06:44:14 PST
[Qt][WK2] Specify storage paths through the C API
Attachments
Patch (17.23 KB, patch)
2013-03-05 07:15 PST, Jocelyn Turcotte 		no flags
DetailsFormatted DiffDiff
Patch (20.20 KB, patch)
2013-03-11 06:40 PDT, Jocelyn Turcotte 		hausmann: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jocelyn Turcotte
Comment 1 2013-03-05 07:15:50 PST
Created attachment 191494 [details] Patch
Jocelyn Turcotte
Comment 2 2013-03-05 07:17:10 PST
This follows the input given in bug 109455.
Simon Hausmann
Comment 3 2013-03-06 06:01:46 PST
Comment on attachment 191494 [details] Patch LGTM
Benjamin Poulain
Comment 4 2013-03-07 14:10:27 PST
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
Comment 5 2013-03-11 06:40:02 PDT
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 6 2013-03-11 07:07:59 PDT
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
Comment 7 2013-03-12 04:18:18 PDT
Committed r145517: <http://trac.webkit.org/changeset/145517>
Note You need to log in before you can comment on or make changes to this bug.