Summary: | [Cache API] Add support for Cache.add/addAll | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, cdumez, commit-queue, sam, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-08-17 10:39:27 PDT
Created attachment 318380 [details]
Patch
EndOfLoad callback part of the patch is the same as the one uploaded in bug 175658 patch Not sure why other EWS bots are not kicking on... 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. Created attachment 318569 [details]
Patch
Created attachment 318583 [details]
Patch
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). (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... (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. I added new StringView API to make the whitespace stripping cheaper in https://bugs.webkit.org/show_bug.cgi?id=175757. Comment on attachment 318583 [details]
Patch
r=me
Adopt Sam's StringView API.
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? Created attachment 318706 [details]
Patch for landing
(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 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 Created attachment 318715 [details]
Patch for landing
Comment on attachment 318715 [details] Patch for landing Clearing flags on attachment: 318715 Committed r220998: <http://trac.webkit.org/changeset/220998> All reviewed patches have been landed. Closing bug. |