Bug 91719

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

92378

Bug Blocks:

61838

Attachments:

Description	Flags
patch	none
patch	none

Zoltan Nyul

Reported 2012-07-19 00:18:48 PDT

The testrunner script is may be running multiple processes in parallel, and it makes appcache tests fail if they are using the same directory. I modified the EFL's LayoutTestController to use the DUMPRENDERTREE_TEMP for application cache directory because it's different for each process. It fixes these tests: http/tests/appcache/access-via-redirect.php http/tests/appcache/auth.html http/tests/appcache/crash-when-navigating-away-then-back.html http/tests/appcache/credential-url.html http/tests/appcache/cyrillic-uri.html http/tests/appcache/different-scheme.html http/tests/appcache/document-write-html-element-2.html http/tests/appcache/empty-manifest.html http/tests/appcache/fail-on-update-2.html http/tests/appcache/fail-on-update.html http/tests/appcache/foreign-fallback.html http/tests/appcache/foreign-iframe-main.html http/tests/appcache/idempotent-update.html http/tests/appcache/interrupted-update.html http/tests/appcache/local-content.html http/tests/appcache/main-resource-hash.html http/tests/appcache/main-resource-redirect.html http/tests/appcache/manifest-containing-itself.html http/tests/appcache/manifest-parsing.html http/tests/appcache/manifest-with-empty-file.html http/tests/appcache/non-html.xhtml http/tests/appcache/offline-access.html http/tests/appcache/online-fallback-layering.html http/tests/appcache/online-whitelist.html http/tests/appcache/progress-counter.html http/tests/appcache/reload.html http/tests/appcache/remove-cache.html http/tests/appcache/simple.html http/tests/appcache/top-frame-1.html http/tests/appcache/top-frame-2.html http/tests/appcache/top-frame-3.html http/tests/appcache/top-frame-4.html http/tests/appcache/update-cache.html http/tests/appcache/whitelist-wildcard.html http/tests/appcache/wrong-content-type.html http/tests/appcache/xhr-foreign-resource.html

Attachments
patch (4.23 KB, patch) 2012-07-19 00:30 PDT, Zoltan Nyul	no flags	Details Formatted Diff Diff
patch (11.40 KB, patch) 2012-07-26 00:48 PDT, Zoltan Nyul	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Zoltan Nyul

Comment 1 2012-07-19 00:30:17 PDT

Created attachment 153195 [details] patch

Chris Dumez

Comment 2 2012-07-19 00:48:07 PDT

Comment on attachment 153195 [details] patch LGTM. Good patch!

Ryuan Choi

Comment 3 2012-07-19 00:50:03 PDT

LGTM, too.

WebKit Review Bot

Comment 4 2012-07-19 02:59:51 PDT

Comment on attachment 153195 [details] patch Clearing flags on attachment: 153195 Committed r123085: <http://trac.webkit.org/changeset/123085>

WebKit Review Bot

Comment 5 2012-07-19 02:59:56 PDT

All reviewed patches have been landed. Closing bug.

Dominik Röttsches (drott)

Comment 6 2012-07-20 02:41:37 PDT

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

Chris Dumez

Comment 7 2012-07-20 02:57:28 PDT

(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.

Chris Dumez

Comment 8 2012-07-20 03:11:14 PDT

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.

Gyuyoung Kim

Comment 9 2012-07-20 03:29:23 PDT

Committed r123200: <http://trac.webkit.org/changeset/123200>

Chris Dumez

Comment 10 2012-07-20 06:10:49 PDT

The patch has been rolled out, reopening the bug.

Chris Dumez

Comment 11 2012-07-23 08:47:32 PDT

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.

Raphael Kubo da Costa (:rakuco)

Comment 12 2012-07-23 12:24:12 PDT

Sorry for chiming in so late in the game. I'm all in for portability changes :-)

Zoltan Nyul

Comment 13 2012-07-26 00:48:28 PDT

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.

Chris Dumez

Comment 14 2012-07-26 01:03:23 PDT

Comment on attachment 154564 [details] patch LGTM.

Gyuyoung Kim

Comment 15 2012-07-26 01:17:10 PDT

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.

Chris Dumez

Comment 16 2012-07-26 01:20:07 PDT

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

Gyuyoung Kim

Comment 17 2012-07-26 01:22:13 PDT

(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.

WebKit Review Bot

Comment 18 2012-07-26 05:01:59 PDT

Comment on attachment 154564 [details] patch Clearing flags on attachment: 154564 Committed r123731: <http://trac.webkit.org/changeset/123731>

WebKit Review Bot

Comment 19 2012-07-26 05:02:06 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.