[Qt][WK2] ApplicationCache LayoutTests failed

• Component: New Bugs
• Status: RESOLVED FIXED

[qi]

Most of the ApplicationCache LayoutTests failed on Qt Webkit2, currently the whole directory is in the skip list.

[qi]

Created attachment 109978 [details]
patch

[Chang Shu]

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.

[Alexis Menard (darktears)]

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.

[Alexis Menard (darktears)]

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

[Chang Shu]

Alexis, please update the flags with caution.

[Alexis Menard (darktears)]

(In reply to comment #5)
> Alexis, please update the flags with caution.

Yep sorry I messed up :(

[qi]

Created attachment 110002 [details]
patch2

[qi]

Created attachment 110003 [details]
patch2

Changed based on Chang and Alexis comments.

[Alexis Menard (darktears)]

Adding Anders as he is the dungeon keeper of WebProcess.

[Chang Shu]

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.

[qi]

Created attachment 110140 [details]
patch3

[WebKit Review Bot]

Comment on attachment 110140 [details]
patch3

Clearing flags on attachment: 110140

Committed r96938: <http://trac.webkit.org/changeset/96938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[qi]

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.

[Adam Barth]

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.

[Chang Shu]

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

[Luciano Wolf]

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.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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