[webkit-reviews] review granted: [Bug 176845] Add Cache API support of records persistency : [Attachment 320793] 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 176845: Add Cache API support of records persistency
https://bugs.webkit.org/show_bug.cgi?id=176845

Attachment 320793: Patch

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




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

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

r=me other than these comments.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:157
> +	       RunLoop::main().dispatch([traversalResult =
WTFMove(traversalResult)]() mutable {

An isolatedCopy would protect this from future changes accidentally introducing
multiply-reffed strings going across threads here.  The key is questionable
already.  I think until we have thread safe refcounted strings we need to
isolatedCopy things when going across threads at places like this.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:218
> +class ReadRecordTaskCounter : public
ThreadSafeRefCounted<ReadRecordTaskCounter> {

This doesn't need to be ThreadSafeRefCounted.  RefCounted is fine because all
operations of this class are on the same thread.  Add assertions.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:319
> +class AsynchronousPutTaskCounter : public
RefCounted<AsynchronousPutTaskCounter> {

This is a strange abstraction.	I think it would be better to reorganize the
code that uses it.