Bug 175747 – [Cache API] Add support for CacheStorage.match

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

[Cache API] Add support for CacheStorage.match

[youenn fablet]

Created attachment 318567 [details]
Patch

[Build Bot]

Attachment 318567 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 318567 [details]
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.

[youenn fablet]

Created attachment 318667 [details]
Patch for landing

[Build Bot]

Attachment 318667 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:84:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Thanks for the review.

(In reply to Chris Dumez from comment #3)
> Comment on attachment 318567 [details]
> 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"

OK

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

OK


> > 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.

OK

> > 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.

info and options are captured in Cache::doMatch lambdas.
 
> > Source/WebCore/Modules/cache/CacheStorage.cpp:68
> > +static inline Vector<Ref<Cache>> copyCaches(Vector<Ref<Cache>>& caches)
> 
> Looks like the parameter should be const.

OK

> > 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.

I like having lambdas as end parameters.
I added a wrapper function.

[WebKit Commit Bot]

Comment on attachment 318667 [details]
Patch for landing

Clearing flags on attachment: 318667

Committed r220984: <http://trac.webkit.org/changeset/220984>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34000173>