Bug 175677 - [Cache API] Add support for Cache.add/addAll
Summary: [Cache API] Add support for Cache.add/addAll
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-17 10:39 PDT by youenn fablet
Modified: 2017-08-21 19:04 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.23 KB, patch)
2017-08-17 10:45 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.97 KB, patch)
2017-08-18 18:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.89 KB, patch)
2017-08-18 22:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (32.87 KB, patch)
2017-08-21 17:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (32.87 KB, patch)
2017-08-21 18:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-08-17 10:39:27 PDT
[Cache API] Add support for Cache.add/addAll
Comment 1 youenn fablet 2017-08-17 10:45:57 PDT
Created attachment 318380 [details]
Patch
Comment 2 youenn fablet 2017-08-17 11:23:41 PDT
EndOfLoad callback part of the patch is the same as the one uploaded in bug 175658 patch
Comment 3 youenn fablet 2017-08-17 11:43:36 PDT
Not sure why other EWS bots are not kicking on...
Comment 4 Alex Christensen 2017-08-17 15:44:22 PDT
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.
Comment 5 youenn fablet 2017-08-18 18:35:19 PDT
Created attachment 318569 [details]
Patch
Comment 6 youenn fablet 2017-08-18 22:35:04 PDT
Created attachment 318583 [details]
Patch
Comment 7 Sam Weinig 2017-08-19 12:51:56 PDT
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).
Comment 8 youenn fablet 2017-08-19 13:10:36 PDT
(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...
Comment 9 Sam Weinig 2017-08-19 16:07:43 PDT
(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.
Comment 10 Sam Weinig 2017-08-20 17:03:00 PDT
I added new StringView API to make the whitespace stripping cheaper in https://bugs.webkit.org/show_bug.cgi?id=175757.
Comment 11 Alex Christensen 2017-08-21 14:35:11 PDT
Comment on attachment 318583 [details]
Patch

r=me
Adopt Sam's StringView API.
Comment 12 Chris Dumez 2017-08-21 15:12:18 PDT
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?
Comment 13 youenn fablet 2017-08-21 17:25:15 PDT
Created attachment 318706 [details]
Patch for landing
Comment 14 youenn fablet 2017-08-21 17:27:01 PDT
(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.
Comment 15 WebKit Commit Bot 2017-08-21 18:16:44 PDT
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
Comment 16 youenn fablet 2017-08-21 18:22:23 PDT
Created attachment 318715 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2017-08-21 19:03:27 PDT
Comment on attachment 318715 [details]
Patch for landing

Clearing flags on attachment: 318715

Committed r220998: <http://trac.webkit.org/changeset/220998>
Comment 18 WebKit Commit Bot 2017-08-21 19:03:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2017-08-21 19:04:02 PDT
<rdar://problem/34005117>