[webkit-reviews] review granted: [Bug 175640] [Cache API] Add a WK2 implementation of CacheStorageConnection : [Attachment 318466] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 175640: [Cache API] Add a WK2 implementation of CacheStorageConnection
https://bugs.webkit.org/show_bug.cgi?id=175640

Attachment 318466: Patch

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




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

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

> Source/WebKit/ChangeLog:81
> +	   * Shared/WebCoreArgumentCoders.h:

I think we should move away from WebCoreArgumentCoders and towards template
encoders in WebCore headers.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:43
> +    default:

This should be case Error::Internal: instead of default: so that if we add a
new error type in the future it will cause a compiler error if we forget to
update this, too.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:103
> +    Cache* cache(uint64_t cacheIdentifier);

I think it would make sense to pass a PAL::SessionID here instead of a uint64_t
with a name of cacheIdentifier to make it clear that the SessionID is the
cacheIdentifier.  Same with other functions in this class, and even the
functions called through IPC.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:116
> +    WebKit::CacheStorageEngine::Error,
> +    WebKit::CacheStorageEngine::Error::Internal

Elsewhere in the patch you indent these more.

> Source/WebKit/Platform/IPC/ArgumentCoders.h:384
> +template<typename ValueType, typename ErrorType> struct
ArgumentCoder<WTF::Expected<ValueType, ErrorType>> {

Cool!

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:1
> +

Extra space

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:55
> +    void doOpen(uint64_t requestIdentifier, const String& /* origin */,
const String& /* cacheName */) final;

I think the /* */ are unnecessary.