Bug 175677 – [Cache API] Add support for Cache.add/addAll

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

[Cache API] Add support for Cache.add/addAll

[youenn fablet]

Created attachment 318380 [details]
Patch

[youenn fablet]

EndOfLoad callback part of the patch is the same as the one uploaded in bug 175658 patch

[youenn fablet]

Not sure why other EWS bots are not kicking on...

[Alex Christensen]

Comment on attachment 318380 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchResponse.h:98
> +    Variant<std::nullptr_t, RefPtr<FormData>, RefPtr<SharedBuffer>> consumeBody();

This is not implemented, and if it were we could use Ref instead of RefPtr.

> Source/WebCore/Modules/fetch/FetchResponse.h:107
> +    static void startFetching(ScriptExecutionContext&, FetchRequest&, std::optional<FetchPromise>&&, std::optional<EndOfLoad>&&);

My comment in https://bugs.webkit.org/show_bug.cgi?id=175658 applies here, too.

[youenn fablet]

Created attachment 318569 [details]
Patch

[youenn fablet]

Created attachment 318583 [details]
Patch

[Sam Weinig]

Comment on attachment 318583 [details]
Patch

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

> Source/WebCore/Modules/cache/Cache.cpp:124
> +        if (!hasStar && stripLeadingAndTrailingHTTPSpaces(view.toStringWithoutCopying()) == "*")

I wonder if, rather than allocating these two strings per loop, we could add a strip function to StringView that would do this without allocation, something like:

template<bool isMatchedCharacter(UChar), typename CharacterType>
inline StringView stripLeadingAndTrailingMatchedCharacters(StringView base, const CharacterType* characters, size_t length)
{
    if (!length)
        return base;

    unsigned start = 0;
    unsigned end = length - 1;
    
    // Skip mached characters from start.
    while (start <= end && predicate(characters[start]))
        ++start;
    
    // Only the matched characters.
    if (start > end)
        return StringView::empty();

    // Skip mached characters from end.
    while (end && predicate(characters[end]))
        --end;

    if (!start && end == length - 1)
        return base;

    StringView result(characters + start, end + 1 - start);
    result.setUnderlyingString(base);
    return result;
}

template<bool isMatchedCharacter(UChar)>
StringView StringView::stripLeadingAndTrailingMatchedCharacters()
{
    if (is8Bit())
        return WTF::stripLeadingAndTrailingMatchedCharacters<isMatchedCharacter, LChar>(*this, characters8(), m_length);
    return WTF::stripLeadingAndTrailingMatchedCharacters<isMatchedCharacter, UChar>(*this, characters16(), m_length);
}


And then a stripLeadingAndTrailingHTTPSpaces overload like:

inline StringView stripLeadingAndTrailingHTTPSpaces(StringView stringView)
{
    return stringView.stripLeadingAndTrailingMatchedCharacters<isHTTPSpace>();
}

(Untest code).

[youenn fablet]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 318583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318583&action=review
> 
> > Source/WebCore/Modules/cache/Cache.cpp:124
> > +        if (!hasStar && stripLeadingAndTrailingHTTPSpaces(view.toStringWithoutCopying()) == "*")
> 
> I wonder if, rather than allocating these two strings per loop, we could add
> a strip function to StringView that would do this without allocation,

I also think it seems a good idea.
I got some push back about it in a previous cache api patch and did not try to do it here.

I can move forward on this in a follow-up, touching string headers is quite compile-time consuming...

[Sam Weinig]

(In reply to youenn fablet from comment #8)
> (In reply to Sam Weinig from comment #7)
> > Comment on attachment 318583 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318583&action=review
> > 
> > > Source/WebCore/Modules/cache/Cache.cpp:124
> > > +        if (!hasStar && stripLeadingAndTrailingHTTPSpaces(view.toStringWithoutCopying()) == "*")
> > 
> > I wonder if, rather than allocating these two strings per loop, we could add
> > a strip function to StringView that would do this without allocation,
> 
> I also think it seems a good idea.
> I got some push back about it in a previous cache api patch and did not try
> to do it here.
> 

Interesting. What was the objection?

> I can move forward on this in a follow-up, touching string headers is quite
> compile-time consuming...

Yeah, no need to do it in this change.

[Sam Weinig]

I added new StringView API to make the whitespace stripping cheaper in https://bugs.webkit.org/show_bug.cgi?id=175757.

[Alex Christensen]

Comment on attachment 318583 [details]
Patch

r=me
Adopt Sam's StringView API.

[Chris Dumez]

Comment on attachment 318583 [details]
Patch

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

> Source/WebCore/Modules/cache/Cache.cpp:115
> +    infos.append(WTFMove(info));

Vector has a constructor that takes an initializer list and would be me more efficient.

> Source/WebCore/Modules/cache/Cache.cpp:139
> +        if (auto callback = WTFMove(m_callback))

This WTFMove() does not seem useful.

> Source/WebCore/Modules/cache/Cache.cpp:424
> +CacheStorageConnection::Record toConnectionRecord(const FetchRequest& request, FetchResponse& response, CacheStorageConnection::ResponseBody&& responseBody)

Why isn't this static anymore? Seems wrong.

> Source/WebCore/Modules/cache/Cache.h:62
> +    ExceptionOr<Ref<FetchRequest>> requestFromInfo(RequestInfo&&, bool ignoreMethod);

Can this be static?

[youenn fablet]

Created attachment 318706 [details]
Patch for landing

[youenn fablet]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 318583 [details]
> Patch
> 
> r=me
> Adopt Sam's StringView API.

Sure, will do in a separate patch.

(In reply to Chris Dumez from comment #12)
> Comment on attachment 318583 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318583&action=review
> 
> > Source/WebCore/Modules/cache/Cache.cpp:115
> > +    infos.append(WTFMove(info));
> 
> Vector has a constructor that takes an initializer list and would be me more
> efficient.

OK
 
> > Source/WebCore/Modules/cache/Cache.cpp:139
> > +        if (auto callback = WTFMove(m_callback))
> 
> This WTFMove() does not seem useful.

OK

> > Source/WebCore/Modules/cache/Cache.cpp:424
> > +CacheStorageConnection::Record toConnectionRecord(const FetchRequest& request, FetchResponse& response, CacheStorageConnection::ResponseBody&& responseBody)
> 
> Why isn't this static anymore? Seems wrong.

It still is, there is a declaration.

> > Source/WebCore/Modules/cache/Cache.h:62
> > +    ExceptionOr<Ref<FetchRequest>> requestFromInfo(RequestInfo&&, bool ignoreMethod);
> 
> Can this be static?

It could but would require passing another parameter.

[WebKit Commit Bot]

Comment on attachment 318706 [details]
Patch for landing

Rejecting attachment 318706 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 318706, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/imported/w3c/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4357058

[youenn fablet]

Created attachment 318715 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 318715 [details]
Patch for landing

Clearing flags on attachment: 318715

Committed r220998: <http://trac.webkit.org/changeset/220998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34005117>