Most of the ApplicationCache LayoutTests failed on Qt Webkit2, currently the whole directory is in the skip list.
Created attachment 109978 [details] patch
Comment on attachment 109978 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=109978&action=review > Source/WebKit2/ChangeLog:9 > + set and we didn't set yet. Shall we set the applicationCacheDirectory somewhere like WK1 does? What if the UI client indeed sets the cacheDirectory and this code change will break it. > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:39 > + return QDesktopServices::storageLocation(QDesktopServices::DataLocation); Even if the approach is correct, I think we should call the existing function defaultDataLocation() in the same file instead.
Comment on attachment 109978 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=109978&action=review > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:39 > + return QDesktopServices::storageLocation(QDesktopServices::DataLocation); I don't think this is the right fix because you don't want to call QDesktopServices every single time. Also this file changed have a look at http://trac.webkit.org/changeset/96702 to use the same pattern as the other.
(In reply to comment #2) > (From update of attachment 109978 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109978&action=review > > > Source/WebKit2/ChangeLog:9 > > + set and we didn't set yet. > > Shall we set the applicationCacheDirectory somewhere like WK1 does? What if the UI client indeed sets the cacheDirectory and this code change will break it. I think the problem is that unlike Database, DefaultIconDataBasePath, LocalStorageDirectory, there is no default (or fallback if a custom one is not set). I think we should do the same as the others : String WebContext::localStorageDirectory() const { if (!m_overrideLocalStorageDirectory.isEmpty()) return m_overrideLocalStorageDirectory; return platformDefaultLocalStorageDirectory(); } and add a platformDefaultApplicationCacheDirectory. > > > Source/WebKit2/UIProcess/qt/WebContextQt.cpp:39 > > + return QDesktopServices::storageLocation(QDesktopServices::DataLocation); > > Even if the approach is correct, I think we should call the existing function defaultDataLocation() in the same file instead.
Alexis, please update the flags with caution.
(In reply to comment #5) > Alexis, please update the flags with caution. Yep sorry I messed up :(
Created attachment 110002 [details] patch2
Created attachment 110003 [details] patch2 Changed based on Chang and Alexis comments.
Adding Anders as he is the dungeon keeper of WebProcess.
Comment on attachment 110003 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=110003&action=review Please update ChangeLog descriptions before landing. > Source/WebKit2/ChangeLog:12 > + 3. Add interface setApplicationCacheDirectory. It seems we need a better wording here. :) How about this: 1. Moved applicationCacheDirectory to common code. 2. Added interface setApplicationCacheDirectory to allow UI client overriding. 3. Renamed all platform-dependent implementations of applicationCacheDirectory to platformDefaultApplicationCacheDirectory. 4. On Qt, set the platformDefaultApplicationCacheDirectory to the default data location. > LayoutTests/ChangeLog:7 > + Add "Unskip passed tests." here.
Created attachment 110140 [details] patch3
Comment on attachment 110140 [details] patch3 Clearing flags on attachment: 110140 Committed r96938: <http://trac.webkit.org/changeset/96938>
All reviewed patches have been landed. Closing bug.
Reopen, because it was rolled out by http://trac.webkit.org/changeset/96969 Guys, you should have check buildbots after landing ... :-/ Check http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/13015 It made many tests fail and made testing time 2,5 hours ...
Not reproducible on my environment:( On buildbot, it has 12 crashs and some failed cases outside of the ApplicationCache. On my environment, if I used new-run-webkit-tests, no crash! but all the ApplicationCache test cases failed. If I used old-run-webkit-tests, all the ApplicationCache passed. But anyway, not able to reproduce the buildbot result.
Comment on attachment 110003 [details] patch2 Cleared Chang Shu's review+ from obsolete attachment 110003 [details] so that this bug does not appear in http://webkit.org/pending-commit.
(In reply to comment #15) > Not reproducible on my environment:( > > On buildbot, it has 12 crashs and some failed cases outside of the ApplicationCache. > > On my environment, if I used new-run-webkit-tests, no crash! but all the ApplicationCache test cases failed. If I used old-run-webkit-tests, all the ApplicationCache passed. But anyway, not able to reproduce the buildbot result. The NRWT was failing for other reasons and it's fixed now. Please try again.
Created attachment 161312 [details] Patch that properly updates app cache dir Returns defaultDiskCacheDirectory when no cache directory was provided, this way setOfflineWebApplicationCacheEnabled works as expected.
Comment on attachment 161312 [details] Patch that properly updates app cache dir Clearing flags on attachment: 161312 Committed r127091: <http://trac.webkit.org/changeset/127091>
(In reply to comment #19) > (From update of attachment 161312 [details]) > Clearing flags on attachment: 161312 > > Committed r127091: <http://trac.webkit.org/changeset/127091> It made layout testing extremely slow: https://bugs.webkit.org/show_bug.cgi?id=95450 Could you check it, please?