[Webkit-unassigned] [Bug 190912] http/wpt/cache-storage/cache-quota.any.html is failing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190912

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com

--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 353098
  --> https://bugs.webkit.org/attachment.cgi?id=353098
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181025/18c4ebc5/attachment.html>