Bug 100678

Created attachment 171426 [details] Patch

Comment on attachment 171426 [details] Patch DId you test this on multiple platforms?

Comment on attachment 171426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171426&action=review > Tools/WebKitTestRunner/TestController.cpp:334 > +#if OS(WINDOWS) > + const char separator = '\\'; > +#else > + const char separator = '/'; > +#endif > + iconDatabaseFileTemp = iconDatabaseFileTemp + separator + "WebpageIcons.db"; > + WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str()); Really? WebCore has methods for doing these things, look in FileSystem.h

Comment on attachment 171426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171426&action=review >> Tools/WebKitTestRunner/TestController.cpp:334 >> + WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str()); > > Really? WebCore has methods for doing these things, look in FileSystem.h I would love to use pathByAppendingComponent here, but we can't use the namespace here. I had to do the same way as Tools/WebKitTestRunner/TestInvocation.cpp

(In reply to comment #4) > (From update of attachment 171426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171426&action=review > > >> Tools/WebKitTestRunner/TestController.cpp:334 > >> + WKRetainPtr<WKStringRef> iconDatabaseFileTempWK = WKStringCreateWithUTF8CString(iconDatabaseFileTemp.c_str()); > > > > Really? WebCore has methods for doing these things, look in FileSystem.h > > I would love to use pathByAppendingComponent here, but we can't use the namespace here. I had to do the same way as Tools/WebKitTestRunner/TestInvocation.cpp Maybe it is time we move FileSystem.h to WTF. This is not the first case.

Comment on attachment 171426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171426&action=review r=me then > Tools/WebKitTestRunner/TestController.cpp:328 > +#if OS(WINDOWS) Add a comment why not using pathByAppending...

(In reply to comment #2) > (From update of attachment 171426 [details]) > DId you test this on multiple platforms? I tested it on Linux only with the following ports: Qt, GTK and EFL.

Created attachment 171615 [details] Patch

Comment on attachment 171615 [details] Patch Clearing flags on attachment: 171615 Committed r133024: <http://trac.webkit.org/changeset/133024>

All reviewed patches have been landed. Closing bug.

This patch is causing random crashes at least on the EFL port. See, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5289/steps/layout-test/logs/stdio>. WKContextSetIconDatabasePath() sets a database path, but when we later create our ewk view the call to Ewk_Context::faviconDatabase() ends up calling it again and reaching the assertion in IconDatabase's destructor.

(In reply to comment #11) > This patch is causing random crashes at least on the EFL port. See, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/5289/steps/layout-test/logs/stdio>. > > WKContextSetIconDatabasePath() sets a database path, but when we later create our ewk view the call to Ewk_Context::faviconDatabase() ends up calling it again and reaching the assertion in IconDatabase's destructor. This is probably EFL specific. We should create a separated bug for that.

Comment on attachment 171615 [details] Patch Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path?

(In reply to comment #13) > (From update of attachment 171615 [details]) > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path? Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 171615 [details] [details]) > > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path? > > Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script: Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py: environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache') We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports.

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (From update of attachment 171615 [details] [details] [details]) > > > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path? > > > > Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc > > Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script: > > Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py: environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache') > > We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports. No, we should not. If you "fix" this on xvfbdriver.py, if you happen to use another driver at some point, you will have to move these hacks around. If Chromium or Qt decides to use the xvfbdriver, they might get some unexpected behavior because of these variables on the system environment. The only reason why we still have XDG_CACHE_HOME on the driver (I removed the others and fixed the XDG_CACHE_HOME - bug 100864 and bug 100864) is because we don't have an WK2 API for setting the application cache folder yet. Please, let's keep the drivers and WTR platform neutral and EFL aligned with other ports.

(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #13) > > > > (From update of attachment 171615 [details] [details] [details] [details]) > > > > Why not use IconDatabase::removeAllIcons() to clear the database instead of messing with the path? > > > > > > Nobody is "messing with the paths" here. The WTR was creating folders on your computer for every run and not deleting them afterwards. This is what this patch is fixing, by making WTR handle the icon database path in the same way as it does for local storage, database, cookies, etc > > > > Well, this is not the way EFL / GTK ports have done this so far. We are using XDG paths for databases and config. We usually just override the XDG environment variable in the test runner python script: > > > > Tools/Scripts/webkitpy/layout_tests/port/xvfbdriver.py: environment['XDG_CACHE_HOME'] = self._port.host.filesystem.join(str(self._driver_tempdir), 'appcache') > > > > We should probably override the 'XDG_DATA_HOME' environment variable to achieve the expected behavior on EFL and GTK ports. > > No, we should not. If you "fix" this on xvfbdriver.py, if you happen to use another driver at some point, you will have to move these hacks around. If Chromium or Qt decides to use the xvfbdriver, they might get some unexpected behavior because of these variables on the system environment. > > The only reason why we still have XDG_CACHE_HOME on the driver (I removed the others and fixed the XDG_CACHE_HOME - bug 100864 and bug 100864) is because we don't have an WK2 API for setting the application cache folder yet. > > Please, let's keep the drivers and WTR platform neutral and EFL aligned with other ports. I see. The issue is that the database path can only be set once. If you set it in WKTR then we can no longer set it in WebKit2 EFL. We would probably need to expose a settings API to set the database path and let the application take care of it. Right?

*** Bug 99448 has been marked as a duplicate of this bug. ***