[webkit-reviews] review denied: [Bug 175599] [Cache API] Implement Worker connection to the Cache storage engine : [Attachment 318197] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 175599: [Cache API] Implement Worker connection to the Cache storage engine
https://bugs.webkit.org/show_bug.cgi?id=175599

Attachment 318197: Patch

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




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

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

> Source/WebCore/Modules/cache/CacheQueryOptions.h:43
> +    CacheQueryOptions options;

return {
    ignoreSearch,
    ignoreMethod,
    ignoreVary,
    cacheName.isolatedCopy()
};

It is about the same amount of lines and avoids some unnecessary refcounting
churn for the String.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:137
> +    iterator->value(cacheIdentifier, error);

It is risky to call a callback while you're holding on to a HashMap iterator.
The callback may do things that modify the m_openAndRemoveCachePendingRequests
HashMap and would invalidate your iterator, causing a crash on the next line.
Please fix.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:149
> +    iterator->value(WTFMove(caches));

It is risky to call a callback while you're holding on to a HashMap iterator.
The callback may do things that modify the m_retrieveCachesPendingRequests
HashMap and would invalidate your iterator, causing a crash on the next line.
Please fix.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:161
> +    iterator->value(WTFMove(records));

It is risky to call a callback while you're holding on to a HashMap iterator.
The callback may do things that modify the m_retrieveRecordsPendingRequests
HashMap and would invalidate your iterator, causing a crash on the next line.
Please fix.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:173
> +    iterator->value(WTFMove(records), error);

ditto

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:174
> +    m_batchDeleteAndPutPendingRequests.remove(iterator);

Method is called remove, member is called delete, please choose one and stick
to it.

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:185
> +    iterator->value(WTFMove(records), error);

ditto.

> Source/WebCore/Modules/cache/CacheStorageConnection.h:107
> +    uint64_t m_openAndRemoveCachePendingRequestsCounter { 0 };

Why do we need for counters?

Can't we just have one to generate identifiers for all kinds of requests?

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:141
> +		   isolatedCaches.uncheckedAppend(CacheInfo { cache.identifier,
cache.name.isolatedCopy() });

May be nice (i.e. more reusable) to have this as an isolatedCopy() method on
the CacheInfo struct instead.

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:197
> +    for (const auto& record : records)

This logic is duplicated, may be nice to move it to a utility function.

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:206
> +	   for (auto& recordData : recordsData)

This logic is duplicated, may be nice to move it to a utility function.

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:36
> +class WorkerCacheStorageConnection : public CacheStorageConnection {

final?

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.h:56
> +    RefPtr<CacheStorageConnection> m_mainThreadConnection;

What makes sure this gets destroyed on the main thread?

> Source/WebCore/loader/FetchOptions.h:66
> +    FetchOptions options = *this;

I don't like this pattern because if somebody adds a new data member that is
not safely copyable cross-thread, this code will still build but crash. I'd
rather it fails building when something adds a new data member and forgets to
update this isolatedCopy() method.
I.e. I'd rather we use { } initializer with all members explicitly initialized.