[webkit-reviews] review denied: [Bug 177552] Add quota to cache API : [Attachment 322189] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 3 18:13:54 PDT 2017


Alex Christensen <achristensen at apple.com> has denied youenn fablet
<youennf at gmail.com>'s request for review:
Bug 177552: Add quota to cache API
https://bugs.webkit.org/show_bug.cgi?id=177552

Attachment 322189: Patch

https://bugs.webkit.org/attachment.cgi?id=322189&action=review




--- Comment #26 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 322189
  --> https://bugs.webkit.org/attachment.cgi?id=322189
Patch

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

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:101
> +	   // Padding the size as per
https://github.com/whatwg/storage/issues/31.
> +	   uint64_t sizeWithPadding = realSize +
static_cast<uint64_t>(randomNumber() * 128000);
> +	   sizeWithPadding -= (sizeWithPadding % 32000);

There's a chance sizeWithPadding will be less than realSize.  The GitHub issue
says to round up, and I think we should.
We should also test that the visible size is always a multiple of 32k.

> Source/WebCore/Modules/cache/DOMCache.cpp:559
>	       auto response = FetchResponse::create(*scriptExecutionContext(),
std::nullopt, WTFMove(responseHeaders), WTFMove(record.response));
>	       response->setBodyData(WTFMove(record.responseBody));
> +	       response->setBodySizeWithPadding(record.responseBodySize);

Could we include these in the constructor so we don't accidentally forget to
call additional setters every time we make a FetchResponse?

> Source/WebKit/UIProcess/API/C/WKContextConfigurationRef.h:47
>  WK_EXPORT WKStringRef
WKContextConfigurationCopyCacheStorageDirectory(WKContextConfigurationRef
configuration);
>  WK_EXPORT void
WKContextConfigurationSetCacheStorageDirectory(WKContextConfigurationRef
configuration, WKStringRef cacheStorageDirectory);
>  
> +WK_EXPORT void
WKContextConfigurationSetCacheStoragePerOriginQuota(WKContextConfigurationRef
configuration, uint64_t quota);

I have two problems with this:
1) There's no corresponding ObjC SPI.  Doing this makes it even harder to adopt
the ObjC SPI.
2) This shouldn't be on WKContext.  It should be on
WKWebsiteDataStore/WKWebsiteDataStoreRef.  These functions should be removed. 
Everything else here having to do with storage is on its way to being moved
there.	Putting more things here makes that transition harder.


More information about the webkit-reviews mailing list