Bug 190912 – http/wpt/cache-storage/cache-quota.any.html is failing

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKitGTK

[darshan]

http/wpt/cache-storage/cache-quota.any.html test is failing,
This can be fixed by setting maximum cache quota limit in testcontroller of gtk

[darshan]

Created attachment 353098 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 353098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353098&action=review

You need to remove the failures from LayoutTests/platform/gtk/TestExpectations and LayoutTests/platform/wpe/TestExpectations. Let's do that on bug 178316 directly.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:116
> +WK_EXPORT void WKWebsiteDataStoreSetQuota(WKWebsiteDataStoreRef dataStoreRef, uint64_t quota);

I'm not familiar with the rules to introduce new API, so let's see what other reviewers say.

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:117
> +    WKWebsiteDataStoreSetQuota(websiteDataStore, 400 * 1024);

I think you should do the same for WPE.

[Frédéric Wang (:fredw)]

*** This bug has been marked as a duplicate of bug 178316 ***

[Michael Catanzaro]

Comment on attachment 353098 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353098&action=review

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:577
> +void WKWebsiteDataStoreSetQuota(WKWebsiteDataStoreRef dataStoreRef, uint64_t quota)
> +{
> +    WebKit::toImpl(dataStoreRef)->websiteDataStore().setCacheStoragePerOriginQuota(quota); 
> +}

This isn't good naming, because it's extremely unclear what quota is being set. You wouldn't know from the name of the function that it's only setting the cache storage quota. I'm also concerned this doesn't match the Mac API, which is called _setCacheStoragePerOriginQuota. So the solution is to name this function WKWebsiteDataStoreSetCacheStoragePerOriginQuota. When we have plumbing like this, with several layers of similar API calls, it's important to keep the naming consistent or else reading the code becomes very confusing.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:116
>> +WK_EXPORT void WKWebsiteDataStoreSetQuota(WKWebsiteDataStoreRef dataStoreRef, uint64_t quota);
> 
> I'm not familiar with the rules to introduce new API, so let's see what other reviewers say.

It's internal API for use by the TestController. No rules other than that an owner (e.g. Youenn in this case) must approve. Remember the C API is not actually public or stable.

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:31
> +#include "UIProcess/API/C/WKWebsiteDataStoreRef.h"

#include <WebKit/WKWebsiteDataStoreRef.h>

If it's not in the include path (should already be), then it should be added in CMakeLists.txt.

>> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:117
>> +    WKWebsiteDataStoreSetQuota(websiteDataStore, 400 * 1024);
> 
> I think you should do the same for WPE.

This should be done in the main TestController.cpp, not here. Then remove the Mac-specific API call from TestControllerCocoa.mm. And then remove the Mac-specific API entirely (from WKWebsiteDataStorePrivate.h and WKWebsiteDataStore.mm), since it's only used in one place by TestController and there's nothing platform-specific about it.

[Frédéric Wang (:fredw)]

(In reply to Michael Catanzaro from comment #4)
> >> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:117
> >> +    WKWebsiteDataStoreSetQuota(websiteDataStore, 400 * 1024);
> > 
> > I think you should do the same for WPE.
> 
> This should be done in the main TestController.cpp, not here. Then remove
> the Mac-specific API call from TestControllerCocoa.mm. And then remove the
> Mac-specific API entirely (from WKWebsiteDataStorePrivate.h and
> WKWebsiteDataStore.mm), since it's only used in one place by TestController
> and there's nothing platform-specific about it.

I agree with Michael and we had already discussed with Darshan about moving to the main TestController class. Since he is new to WebKit, I think it's ok to start with just the minimal changes to make tests pass on WPE/GTK and then do follow-up refactoring so that the quota limit is set on all platforms at one place.