| Summary: | [EFL] EFL port should use XDG paths | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zoltan Nyul <zoltan.nyul> | ||||||
| Component: | WebKit2 | Assignee: | Zoltan Nyul <zoltan.nyul> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, cdumez, dpranke, d-r, gyuyoung.kim, kenneth, ojan, rakuco, ryuan.choi, sw0524.lee, tmpsantos, tonikitoo, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | 92378 | ||||||||
| Bug Blocks: | 61838 | ||||||||
| Attachments: |
|
||||||||
|
Description
Zoltan Nyul
2012-07-19 00:18:48 PDT
Created attachment 153195 [details]
patch
Comment on attachment 153195 [details]
patch
LGTM. Good patch!
LGTM, too. Comment on attachment 153195 [details] patch Clearing flags on attachment: 153195 Committed r123085: <http://trac.webkit.org/changeset/123085> All reviewed patches have been landed. Closing bug. Could you try $ run-webkit-tests -2 --efl --debug i.e. using this patch on a debug build? I get assertion failures: STDERR: ASSERTION FAILED: m_cacheDirectory.isNull() STDERR: /fast/dominik/dev/WebKitGit_EFL/Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp(366) : void WebCore::ApplicationCacheStorage::setCacheDirectory(const WTF::String&) STDERR: 1 0x7f572b3fe393 WebCore::ApplicationCacheStorage::setCacheDirectory(WTF::String const&) STDERR: 2 0x7f56d7f7bb19 WTR::LayoutTestController::platformInitialize() STDERR: 3 0x7f56d7f68e58 WTR::LayoutTestController::LayoutTestController() STDERR: 4 0x7f56d7f68d4c WTR::LayoutTestController::create() (In reply to comment #6) > Could you try > $ run-webkit-tests -2 --efl --debug > i.e. using this patch on a debug build? I get assertion failures: > > STDERR: ASSERTION FAILED: m_cacheDirectory.isNull() > STDERR: /fast/dominik/dev/WebKitGit_EFL/Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp(366) : void WebCore::ApplicationCacheStorage::setCacheDirectory(const WTF::String&) > STDERR: 1 0x7f572b3fe393 WebCore::ApplicationCacheStorage::setCacheDirectory(WTF::String const&) > STDERR: 2 0x7f56d7f7bb19 WTR::LayoutTestController::platformInitialize() > STDERR: 3 0x7f56d7f68e58 WTR::LayoutTestController::LayoutTestController() > STDERR: 4 0x7f56d7f68d4c WTR::LayoutTestController::create() As I explained to Zoltan yesterday, the application cache directory can only be set once. If you try to set it more than once on a debug build, you will hit this ASSERT. The weird thing is that I saw Zoltan running all the tests and I thought he had debug enabled. Comment on attachment 153195 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=153195&action=review > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:43 > + We need to return an empty String if WebCore::cacheStorage().cacheDirectory() is non-empty (else case). Otherwise, the appcache directory gets set twice and we crash in debug mode. Committed r123200: <http://trac.webkit.org/changeset/123200> The patch has been rolled out, reopening the bug. As discussed, the port port should use XDG paths (cache, config, data) instead of hardcoding $HOME+".webkit/xxx" everywhere. EFREET library should probably be used. We can then override the XDG_CACHE_HOME variable in the test runner to make sure each process has its own cache path. Sorry for chiming in so late in the game. I'm all in for portability changes :-) Created attachment 154564 [details]
patch
I've modified the patch to use xdg paths instead of $HOME+".webkit/xxx and i've also fixed the assert.
Comment on attachment 154564 [details]
patch
LGTM.
Comment on attachment 154564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review > Source/WebKit/efl/ewk/ewk_main.cpp:163 > + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; I would recommend to use makeString(). > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:37 > + return String::fromUTF8(efreet_cache_home_get()) + "/WebKitEfl/Applications"; ditto. Comment on attachment 154564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review >> Source/WebKit/efl/ewk/ewk_main.cpp:163 >> + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; > > I would recommend to use makeString(). MakeString is deprecated as it brings no performance improvement over + operator: https://bugs.webkit.org/show_bug.cgi?id=62527#c15 (In reply to comment #16) > (From update of attachment 154564 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154564&action=review > > >> Source/WebKit/efl/ewk/ewk_main.cpp:163 > >> + String localStorageDirectory = String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl/LocalStorage"; > > > > I would recommend to use makeString(). > > MakeString is deprecated as it brings no performance improvement over + operator: https://bugs.webkit.org/show_bug.cgi?id=62527#c15 Oops. makeString is deprecated. Looks fine now. Comment on attachment 154564 [details] patch Clearing flags on attachment: 154564 Committed r123731: <http://trac.webkit.org/changeset/123731> All reviewed patches have been landed. Closing bug. |