[webkit-reviews] review granted: [Bug 175455] [Cache API] Adding generic support for CacheStorage and Cache methods : [Attachment 318113] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 175455: [Cache API] Adding generic support for CacheStorage and Cache
methods
https://bugs.webkit.org/show_bug.cgi?id=175455

Attachment 318113: Patch

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




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

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

r=me with comments

> Source/WebCore/Modules/cache/Cache.cpp:143
> +    varyValue.split(',', false, [&](StringView view) {

Vary header usually looks like so:
Vary: accept-encoding, accept-language

Because there is often a space after the comma, the code is is not sufficient.
I believe you should strip surrounding spaces before doing comparison.

> Source/WebCore/Modules/cache/Cache.cpp:236
> +    queryCache(request.releaseNonNull(), WTFMove(options), [promise =
WTFMove(promise)](const Vector<CacheStorageRecord>& records) mutable {

const auto& records would like work.

> Source/WebCore/Modules/cache/Cache.cpp:302
> +	   cachedResponse.setHTTPHeaderField(header.key, header.value);

This merges existing headers and new ones.

> Source/WebCore/Modules/cache/Cache.cpp:305
> +    cachedRequest.setHTTPHeaderFields(request.headers().internalHeaders());

This replaces existing headers with new ones.

Which behavior is right?

> Source/WebCore/Modules/cache/Cache.cpp:362
> +    return !m_records.size() && !hasPendingActivity();

m_records.isEmpty()

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:72
> +	   if (name == "*")

Vary header usually looks like so:
Vary: accept-encoding, accept-language

Because there is often a space after the comma, the code is is not sufficient.
I believe you should strip surrounding spaces before doing comparison.

See stripLeadingAndTrailingHTTPSpaces() in HTTPParsers.h.

> Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37
> +    WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { };

Should not be on one line.