Comment on attachment 251023[details]
more thread safety
View in context: https://bugs.webkit.org/attachment.cgi?id=251023&action=review
r=me
> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:122
> auto filter = std::make_unique<ContentsFilter>();
Maybe rename this one to recordFilter now that we have another filter?
> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:124
> size_t size = 0;
Ditto, I would rename this to recordsSize for clarity as it doesn't account for bodies.
> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:189
> bool Storage::mayContain(const Key& key) const
Maybe we can add a mayContainBodyBlob() now? For consistency. Then rename this one to mayContainRecord().
I know it is only used in one place right now but I still like it.
> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:421
> +void Storage::dispatchReadOperation(ReadOperation& read)
Not a big fan of 'read' variable name. Maybe something like readOperation would be more readable here.
> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:435
> + bool shouldGetBodyBlob = !m_bodyBlobFilter || m_bodyBlobFilter->mayContain(read.key.hash());
I would use a utility function like we do for the records filter already.

(In reply to comment #4)
> Not sure it is best but maybe we could use 2 booleans instead (e.g.
> hasReadRecord, hasReadBody). Might be a little more readable than expecting
> this integer to be 2. Your call.
That's what the first patch had and it is actually difficult to make thread safe.