[webkit-reviews] review granted: [Bug 175747] [Cache API] Add support for CacheStorage.match : [Attachment 318567] 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 175747: [Cache API] Add support for CacheStorage.match
https://bugs.webkit.org/show_bug.cgi?id=175747

Attachment 318567: Patch

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




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

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

r=me with comments.

> Source/WebCore/ChangeLog:11
> +	   Make Cache::match uses Cache::doMatch as well.

"use"

> Source/WebCore/Modules/cache/Cache.cpp:79
> +	   if (!records.size()) {

records.isEmpty()

> Source/WebCore/Modules/cache/CacheStorage.cpp:57
> +    auto infoCopy = info;

Local variable not needed, just capture info in the lambda. By default, it
captures by value and copies.

> Source/WebCore/Modules/cache/CacheStorage.cpp:58
> +    auto optionsCopy = options;

Local variable not needed, just capture options in the lambda. By default, it
captures by value and copies.

> Source/WebCore/Modules/cache/CacheStorage.cpp:59
> +    caches[index]->doMatch(WTFMove(info), WTFMove(options), [caches =
WTFMove(caches), info = WTFMove(infoCopy), options = WTFMove(optionsCopy),
completionHandler = WTFMove(completionHandler), index](FetchResponse* value)
mutable {

No WTFMove() for info & options.

> Source/WebCore/Modules/cache/CacheStorage.cpp:64
> +	   doSequentialMatch(++index, WTFMove(caches), WTFMove(info),
WTFMove(options), WTFMove(completionHandler));

No 100% clear why we need to copy info / options for every index... I assume
you have a good reason since it is more expensive.

> Source/WebCore/Modules/cache/CacheStorage.cpp:68
> +static inline Vector<Ref<Cache>> copyCaches(Vector<Ref<Cache>>& caches)

Looks like the parameter should be const.

> Source/WebCore/Modules/cache/CacheStorage.cpp:89
> +	   doSequentialMatch(0, copyCaches(m_caches), WTFMove(info),
WTFMove(options), [protectedThis = makeRef(*this), this, promise =
WTFMove(promise)](FetchResponse* result) mutable {

It does not look good to require the call site to pass 0 as index here. I think
we should abstract this better by either:
- Introduce a wrapper function for the initial call
or
- Make the index parameter last and use 0 as default parameter value.