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

Thu Aug 17 15:36:15 PDT 2017

Chris Dumez <cdumez at apple.com> has denied 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 318331: Patch

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

--- Comment #17 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 318331
  --> https://bugs.webkit.org/attachment.cgi?id=318331
Patch

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:454
> +    m_cacheStorageConnections.take(identifier);

remove() since you're not using the value returned by take().

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:37
> +static std::unique_ptr<CacheStorageEngine>& defaultCacheStorageEngine()

Should be merged into CacheStorageEngine::defaultEngine(), there is no need for
2 functions.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:40
> +    static NeverDestroyed<std::unique_ptr<CacheStorageEngine>> engine;

You can initialize right away: = std::make_unique<CacheStorageEngine>();

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:75
> +    readCaches([this, origin, cacheName, callback =
WTFMove(callback)](std::optional<Error>&& error) mutable {

Based on the readCaches() name and the fact that it takes in a lambda, it is
not clear to me:
1. What readCaches() actually does
2. What the passed lambda is for

Should readCaches() pass what was read to the lambda?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:77
> +	       callback(UnexpectedType<Error> { error.value() });

makeUnexpected() ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:82
> +	   auto& caches = result.iterator->value;

Could be merged into the previous line.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:90
> +		       callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:113
> +	   callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:119
> +	       callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:130
> +	       callback(UnexpectedType<Error> { error.value() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:141
> +	       callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:151
> +    readCache(cacheIdentifier, [this, cacheIdentifier, records =
WTFMove(records), callback = WTFMove(callback)](CacheOrError&& result) mutable
{

We have used the opposite naming so far:
ExceptionOr<>
ResourceErrorOr<>

Also usually templated so this could be ErrorOr<Cache> maybe?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:153
> +	       callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:159
> +	   WebCore::CacheQueryOptions options;

using namespace WebCore; ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:164
> +	       if (!matchingRecords.size()) {

matchingRecords.isEmpty()

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:180
> +	   writeCacheRecords(cacheIdentifier, WTFMove(recordIdentifiers),
[cacheIdentifier, callback = WTFMove(callback)](RecordIdentifiersOrError&&
result) mutable {

ErrorOr<RecordIdentifier> ?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:190
> +	       callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:197
> +	   if (!recordsToRemove.size()) {

recordsToRemove.isEmpty()

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:209
> +		   callback(UnexpectedType<Error> { result.error() });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:215
> +		   callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:227
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:233
> +    // FIXME: Implement reading

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:239
> +    // FIXME: Implement reading

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:242
> +	   callback(UnexpectedType<Error> { Error::Internal });

makeUnexpected()?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:250
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:251
> +    callback(WTFMove(recordsIdentifiers));

What's the point of passing the records identifiers to the callback if they are
passed in by the client anyway?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:256
> +    // FIXME: Implement writing

Missing period at the end.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:257
> +    callback(WTFMove(recordsIdentifiers));

What's the point of passing the records identifiers to the callback if they are
passed in by the client anyway?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:260
> +CacheStorageEngine::Cache* CacheStorageEngine::cache(uint64_t
cacheIdentifier)

We often write:
auto CacheStorageEngine::cache(uint64_t cacheIdentifier) -> Cache*

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:57
> +    CacheStorageEngine() = default;

Why is this needed?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:59
> +    using CacheIdentifierOrError = Expected<uint64_t, Error>;

See earlier comment about using ErrorOr<> for consistency with the rest of the
code base.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:62
> +    using CacheInfosOrError =
Expected<Vector<WebCore::CacheStorageConnection::CacheInfo>, Error>;

ditto

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:65
> +    using RecordsOrError = Expected<Vector<Record>, Error>;

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:68
> +    using RecordIdentifiersOrError = Expected<Vector<uint64_t>, Error>;

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:73
> +    void open(const String& /* origin */, const String& /* cacheName */,
CacheIdentifierCallback&&);

Why the /* */? You don't need them. Since the parameter names are useful, just
have them. The style checker only complains about redundant parameter names.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:74
> +    void remove(uint64_t /* cacheIdentifier */, CacheIdentifierCallback&&);

ditto.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:84
> +    void writeCaches(CompletionCallback&&);

ToDisk?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:85
> +    void readCaches(CompletionCallback&&);

FromDisk?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:94
> +    using CacheOrError = Expected<std::reference_wrapper<Cache>, Error>;

ErrorOr<>?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.h:109
> +    uint64_t m_nextCacheIdentifier { 0 };

Any reason this cannot be a static in the cpp? Is extCacheIdentifier going to
be used from several threads for e.g.?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.cpp:1
> +

Blank line?

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:63
> +    uint64_t m_identifier { 0 };

Doesn't seem like this needs a default value?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:406
> +	       expected = WTF::UnexpectedType<ErrorType> { WTFMove(error) };

makeUnexpected()?

> Source/WebKit/Platform/IPC/ArgumentCoders.h:407
> +	   }

Seems like you should return true here? Otherwise you go on to try and read a
value :(

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:180
> +    options.cacheName = cacheName;

WTFMove(cacheName)?

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:200
> +    if (!decoder.decode(type))

I am fuzzy on the details but don't we usually use encodeEnum() / decodeEnum()
for enum types?

> Source/WebKit/Shared/WebCoreArgumentCoders.h:792
> +template<> struct EnumTraits<WebCore::CacheStorageConnection::Error> {

Not familiar with this part of the code but do we really need those if we use
encodeEnum() / decodeEnum()? IDB's KeyType uses encodeEnum() / decodeEnum() and
I don't see such EnumTraits.

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

Blank line?

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.cpp:93
> +    CacheStorageConnection::openCompleted(requestIdentifier,
result.hasValue() ? result.value() : 0, !result.hasValue() ? Error::Internal :
Error::None);

It is really unfortunate we are passing both a value and an error here (and
below). I already commented about this in the previous patch.

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

/* */ are not needed.

> Source/WebKit/WebProcess/Cache/WebCacheStorageConnection.h:60
> +    void doRemove(uint64_t requestIdentifier, uint64_t /* cacheIdentifier
*/) final;

/* */ are not needed.