[Cache API] Adding generic support for CacheStorage and Cache methods
Created attachment 317873 [details] Patch
Attachment 317873 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:44: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:51: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:64: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:85: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:306: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:57: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317878 [details] Patch
Attachment 317878 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:44: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:51: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:64: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:85: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:306: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:57: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 51 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317890 [details] Patch
Attachment 317890 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:44: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:51: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:64: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:85: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:306: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:57: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 51 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317894 [details] Patch
Attachment 317894 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:44: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:51: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:64: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:85: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:306: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:57: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317894&action=review > Source/WebCore/ChangeLog:19 > + Adding a CacheStorageProvider abstraction that creates a CacheStorageConnection. > + The CacheStorageProvider is accessed from the page for Document calls. > + The CacheStorageConnection is responsible to implement the read/write operations on the cache database. > + At the moment, it does nothing. > + > + Implementing CacheStorage APIs and Cache APIs based on the CacheStorageConnection except for Cache add and addAll which will be implemented later on. > + > + Adding support for CacheStorage.match function as a JS built-in as this involves promises and allows a clearer implementation. > + Adding various accessors and constructors for Fetch constructs as needed by the Cache API implementation. > + This is a big change. Can you add more detailed change information in the change file/function list below? > Source/WebCore/Modules/cache/Cache.cpp:52 > + auto* origin = m_context.securityOrigin(); > + return origin ? origin->toString() : emptyString(); In what situations does a context not have an origin? > Source/WebCore/Modules/cache/Cache.cpp:131 > + if (!protocolIsInHTTPFamily(request->url()) || request->method() != "GET") { > + promise.reject(Exception { TypeError, ASCIILiteral("Request is not HTTP/HTTPS or method is not GET") }); > + return; > + } Seems like this should be split up, so the error is specific to the issue. > Source/WebCore/Modules/cache/Cache.cpp:146 > + String varyValue = response->headers().internalHeaders().get(WebCore::HTTPHeaderName::Vary); This can be auto. > Source/WebCore/Modules/cache/Cache.cpp:148 > + Vector<String> varyingHeaderNames; > + varyValue.split(',', varyingHeaderNames); Split returning a Vector is a pretty allocation heavy operation. Seems like the functor variant would be more appropriate here. > Source/WebCore/Modules/cache/Cache.cpp:161 > + promise.reject(Exception { TypeError, ASCIILiteral("Caching a loading Response is not yet supported") }); NotSupportedError? > Source/WebCore/Modules/cache/Cache.cpp:166 > + promise.reject(Exception { TypeError, ASCIILiteral("Caching a Response with data stored in a ReadableStream is not yet supported") }); NotSupportedError? > Source/WebCore/Modules/cache/Cache.cpp:312 > +void Cache::batchPutOperation(const FetchRequest& request, const FetchResponse& response, Function<void(CacheStorageConnection::Error)>&& callback) Can the function take an ExceptionOr<void> instead of an Error enum? > Source/WebCore/Modules/cache/Cache.idl:37 > + [NewObject, PrivateIdentifier, PublicIdentifier] Promise<sequence<FetchResponse>> matchAll(optional RequestInfo request, optional CacheQueryOptions options); I have been making a concerted effort to remove js-builtin from the bindings, as they go agains the point of the bindings generator, to be generated. The changelog indicates that this is necessary to make a cleaner implementation. Can you go into more detail on that? > Source/WebCore/Modules/cache/CacheStorage.h:58 > + ScriptExecutionContext& m_context; Why is this reference safe? What is keeping the ScriptExecutionContext alive while the CacheStorage is alive? > Source/WebCore/Modules/cache/CacheStorage.idl:36 > + [NewObject, JSBuiltin] Promise<any> match(RequestInfo request, optional CacheQueryOptions options); Can we avoid the builtin? > Source/WebCore/Modules/fetch/FetchRequest.h:60 > + struct InternalRequest { > + ResourceRequest request; > + FetchOptions options; > + String referrer; > + }; Can we just put FetchOptions and referrer into the ResourceRequest, and get rid of InternalRequest?
Comment on attachment 317894 [details] Patch Thanks for the feedback. More inline. View in context: https://bugs.webkit.org/attachment.cgi?id=317894&action=review >> Source/WebCore/ChangeLog:19 >> + > > This is a big change. Can you add more detailed change information in the change file/function list below? Right, I will do this. Reason of this big change is to validate CacheStorageConnection, but maybe I should have split it further. >> Source/WebCore/Modules/cache/Cache.cpp:52 >> + return origin ? origin->toString() : emptyString(); > > In what situations does a context not have an origin? Workers do that check when returning their origins but I am not sure why. This might be old code that should be updated. I can put a FIXME. >> Source/WebCore/Modules/cache/Cache.cpp:131 >> + } > > Seems like this should be split up, so the error is specific to the issue. OK >> Source/WebCore/Modules/cache/Cache.cpp:146 >> + String varyValue = response->headers().internalHeaders().get(WebCore::HTTPHeaderName::Vary); > > This can be auto. OK >> Source/WebCore/Modules/cache/Cache.cpp:148 >> + varyValue.split(',', varyingHeaderNames); > > Split returning a Vector is a pretty allocation heavy operation. Seems like the functor variant would be more appropriate here. OK >> Source/WebCore/Modules/cache/Cache.cpp:161 >> + promise.reject(Exception { TypeError, ASCIILiteral("Caching a loading Response is not yet supported") }); > > NotSupportedError? OK >> Source/WebCore/Modules/cache/Cache.cpp:166 >> + promise.reject(Exception { TypeError, ASCIILiteral("Caching a Response with data stored in a ReadableStream is not yet supported") }); > > NotSupportedError? OK >> Source/WebCore/Modules/cache/Cache.cpp:312 >> +void Cache::batchPutOperation(const FetchRequest& request, const FetchResponse& response, Function<void(CacheStorageConnection::Error)>&& callback) > > Can the function take an ExceptionOr<void> instead of an Error enum? An exception error is an enum plus a string, which would need additional handling for workers or IPC. At now, there is no need for custom strings that cannot be deduced from the error value. >> Source/WebCore/Modules/cache/Cache.idl:37 >> + [NewObject, PrivateIdentifier, PublicIdentifier] Promise<sequence<FetchResponse>> matchAll(optional RequestInfo request, optional CacheQueryOptions options); > > I have been making a concerted effort to remove js-builtin from the bindings, as they go agains the point of the bindings generator, to be generated. > > The changelog indicates that this is necessary to make a cleaner implementation. Can you go into more detail on that? The CacheStorage match spec implementation is based on promises and on Cache.@matchAll. It iterates through the caches objects and call matchAll on each cache as soon as the previous matchAll promise is fulfilled. This is probably doable in C++ but is more tedious and would be less close to the spec. FWIW, I also plan to implement Cache.addAll as a JS builtin. The principle in that case is to start several fetch calls at the same time and do a Promise.all on all fetch promises. Once done, write to the cache atomically. Again, this is probably doable in C++ but the js builtin implementation will be easier, clearer and closer to the spec. >> Source/WebCore/Modules/cache/CacheStorage.h:58 >> + ScriptExecutionContext& m_context; > > Why is this reference safe? What is keeping the ScriptExecutionContext alive while the CacheStorage is alive? Right, CacheStorage should be a ContextDestructionObserver or an ActiveDOMObject. >> Source/WebCore/Modules/cache/CacheStorage.idl:36 >> + [NewObject, JSBuiltin] Promise<any> match(RequestInfo request, optional CacheQueryOptions options); > > Can we avoid the builtin? As said above, we can probably avoid it but it will decrease the readability and ease of authoring. If that is a blocker, let's split CacheStorage.match implementation from this patch. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:41 > + return Exception { TypeError, ASCIILiteral("Not implemented") }; Should be NotSupportedError then. >> Source/WebCore/Modules/fetch/FetchRequest.h:60 >> + }; > > Can we just put FetchOptions and referrer into the ResourceRequest, and get rid of InternalRequest? We need an abstraction like this one for cache API and for the fetch event. Referrer as is would be a bit odd in the ResourceRequest since it is not always the same as the request http header field. There are specific values "no-referrer" and "client". Maybe this referrer should be an enumeration in ResourceRequest. Adding fetch options into ResourceRequest would be a bigger change. For instance, CachedResource blocks modifications of FetchOptions but not of its resource request. FetchOptions is the base class of ResourceLoaderOptions/ThreadableLoaderOptions. Somehow I feel that fetch options are immutable for a given fetch while the resource request may mutate. Probably we can get rid of InternalRequest and inline request and options as regular FetchRequest members.
Created attachment 317934 [details] Patch
Created attachment 317937 [details] Patch
Attachment 317937 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:52: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:65: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:80: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:109: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:320: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:58: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317946 [details] win fix try
Attachment 317946 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:52: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:65: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:80: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:109: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:320: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.h:58: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317972 [details] win fix try again
Attachment 317972 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:52: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:65: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:80: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:109: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/Cache.cpp:273: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:46: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 6 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.
I changed a bit the patch compared to the one reviewed by Sam yesterday: - Adding an identifier to Cache. Passing an origin+name to the CacheStorageConnection is not sufficient (and not very efficient either). There is for instance the case where a page adds a Cache, removes it and adds another one with the same name. Both Caches should still work although the one that is removed will not persist once the web page is gone. - Moving batchPutOperation to take a list of records, as it will be needed for addAll implementation. I also updated the implementation according Sam comments. One related change is making Cache/CacheStorage active DOM objects.
(In reply to youenn fablet from comment #10) > >> Source/WebCore/Modules/cache/Cache.cpp:312 > >> +void Cache::batchPutOperation(const FetchRequest& request, const FetchResponse& response, Function<void(CacheStorageConnection::Error)>&& callback) > > > > Can the function take an ExceptionOr<void> instead of an Error enum? > > An exception error is an enum plus a string, which would need additional > handling for workers or IPC. > At now, there is no need for custom strings that cannot be deduced from the > error value. Seems a bit unfortunate to have an alternate Exception mechanism here. > >> Source/WebCore/Modules/cache/Cache.idl:37 > >> + [NewObject, PrivateIdentifier, PublicIdentifier] Promise<sequence<FetchResponse>> matchAll(optional RequestInfo request, optional CacheQueryOptions options); > > > > I have been making a concerted effort to remove js-builtin from the bindings, as they go agains the point of the bindings generator, to be generated. > > > > The changelog indicates that this is necessary to make a cleaner implementation. Can you go into more detail on that? > > The CacheStorage match spec implementation is based on promises and on > Cache.@matchAll. > It iterates through the caches objects and call matchAll on each cache as > soon as the previous matchAll promise is fulfilled. > This is probably doable in C++ but is more tedious and would be less close > to the spec. > > FWIW, I also plan to implement Cache.addAll as a JS builtin. > The principle in that case is to start several fetch calls at the same time > and do a Promise.all on all fetch promises. Once done, write to the cache > atomically. > Again, this is probably doable in C++ but the js builtin implementation will > be easier, clearer and closer to the spec. > ... > > >> Source/WebCore/Modules/cache/CacheStorage.idl:36 > >> + [NewObject, JSBuiltin] Promise<any> match(RequestInfo request, optional CacheQueryOptions options); > > > > Can we avoid the builtin? > > As said above, we can probably avoid it but it will decrease the readability > and ease of authoring. > If that is a blocker, let's split CacheStorage.match implementation from > this patch. I'd really like to see us evolve the C++ promise support rather than implement this as builtins. I actually find the opposite, that the builtins decrease readability, because you have to know how to jump between the JS code and the C++ code. Perhaps splitting that out into a separate change will be good.
(In reply to Sam Weinig from comment #19) > I'd really like to see us evolve the C++ promise support rather than > implement this as builtins. I actually find the opposite, that the builtins > decrease readability, because you have to know how to jump between the JS > code and the C++ code. I could not agree more. Promises are going to show up in more and more web APIs going forward, so its worth the investment.
(In reply to Brady Eidson from comment #20) > (In reply to Sam Weinig from comment #19) > > I'd really like to see us evolve the C++ promise support rather than > > implement this as builtins. I actually find the opposite, that the builtins > > decrease readability, because you have to know how to jump between the JS > > code and the C++ code. > > I could not agree more. > > Promises are going to show up in more and more web APIs going forward, so > its worth the investment. +1. I was very happy to see Sam's patches removing some of the JS builtins code. I think the new C++ code looked much better.
Created attachment 318017 [details] Patch
Created attachment 318022 [details] Patch
Attachment 318022 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:70: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:85: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:115: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:46: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:285: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318026 [details] Patch
Attachment 318026 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:92: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:122: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:46: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:285: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 46 files If any of these errors are false positives, please file a bug against check-webkit-style.
> > Source/WebCore/Modules/cache/Cache.cpp:312 > > +void Cache::batchPutOperation(const FetchRequest& request, const FetchResponse& response, Function<void(CacheStorageConnection::Error)>&& callback) > > Can the function take an ExceptionOr<void> instead of an Error enum? Rethinking about it, while CacheStorageConnection should use an Error enum, it would be better for this callback to be a Function<void(ExceptionOr<T>&&)&&. Especially if DOMPromiseDeferred has a method that gets an ExceptionOr<T>&& and call reject/resolve based on it. I'll fix that later if possible. > I'd really like to see us evolve the C++ promise support rather than > implement this as builtins. I actually find the opposite, that the builtins > decrease readability, because you have to know how to jump between the JS > code and the C++ code. Sure, we could add some more C++ code. I am not sure that this is worth it though for CacheStorage.match. Jumping between JS and C++ code is not very complicated, the only gotcha is the private id uses. > Perhaps splitting that out into a separate change will be good. Done. I'll try to check the C++ way for match and see the differences. For addAll, I may actually go to C++ since the fetch promise resolves at response time while it might need a callback to the end of the load.
Comment on attachment 318026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318026&action=review > Source/WebCore/Modules/cache/Cache.cpp:79 > + refreshRequestToResponseMap([this, promise = WTFMove(promise)]() { I think refreshRequestToResponseMap is confusingly named. It is really hard for me to understand what it does. > Source/WebCore/Modules/cache/Cache.cpp:90 > + if (records.size()) { !records.isEmpty() > Source/WebCore/Modules/cache/Cache.cpp:139 > + varyValue.split(',', false, [&](const StringView& view) { We usually pass StringView type by value. > Source/WebCore/Modules/cache/Cache.cpp:140 > + if (!hasVaryHeaderStar && view == "*") Looks like you are forgetting to update varyingHeaderNames Vector in your functor? > Source/WebCore/Modules/cache/Cache.cpp:171 > + batchPutOperation(*request, response.get(), [promise = WTFMove(promise)](CacheStorageConnection::Error error) mutable { Is "batch" the right naming here? Are you really batching or merely queueing? > Source/WebCore/Modules/cache/Cache.cpp:287 > + callback(!!records.size(), error); !records.isEmpty() > Source/WebCore/Modules/cache/Cache.cpp:326 > + Vector<CacheStorageRecord> newMap; confusing naming. > Source/WebCore/Modules/cache/Cache.cpp:358 > + return !m_requestToResponseMap.size() && !hasPendingActivity(); m_requestToResponseMap.isEmpty() > Source/WebCore/Modules/cache/Cache.h:46 > + enum class MatchType { All, OnlyFirst }; Could be private if comment below is applied. > Source/WebCore/Modules/cache/Cache.h:47 > + void match(RequestInfo&&, CacheQueryOptions&&, Ref<DeferredPromise>&&); Why isn't this const? > Source/WebCore/Modules/cache/Cache.h:48 > + void matchAll(std::optional<RequestInfo>&&, CacheQueryOptions&&, Ref<DeferredPromise>&&, MatchType = MatchType::All); Having a method named matchAll() which takes a MatchType parameter which can be OnlyFirst is confusing. My suggestion would be to have a private method that takes the MatchType parameter and have it called from match() / matchAll() using the right matchType. We could call this private method something like matchForType(). Also, why isn't this const? > Source/WebCore/Modules/cache/Cache.h:54 > + void keys(std::optional<RequestInfo>&&, CacheQueryOptions&&, KeysPromise&&); Why isn't this const? > Source/WebCore/Modules/cache/Cache.h:67 > + void refreshRequestToResponseMap(WTF::Function<void()>&&); This method name is confusing. > Source/WebCore/Modules/cache/Cache.h:73 > + void updateRequestToResponseMap(Vector<CacheStorageConnection::Record>&&); This method name is confusing. > Source/WebCore/Modules/cache/Cache.h:79 > + Vector<CacheStorageRecord> m_requestToResponseMap; Why is this called map? The variable naming is confusing given its Vector type. Could it something like m_cacheStorageRecords? > Source/WebCore/Modules/cache/CacheQueryOptions.h:33 > +public: Why this change? Looked better before. > Source/WebCore/Modules/cache/CacheStorage.cpp:61 > +void CacheStorage::refreshCacheMap(WTF::Function<void()>&& callback) refreshCacheMapThen? Otherwise, it is really unclear what the lambda is for. > Source/WebCore/Modules/cache/CacheStorage.cpp:68 > + m_connection->refreshCacheMap(origin, [this, callback = WTFMove(callback)](Vector<CacheStorageConnection::CacheInfo>&& cachesInfo) { refreshCacheMapThen? > Source/WebCore/Modules/cache/CacheStorage.cpp:101 > + return; So in this case we do not reject the promise? > Source/WebCore/Modules/cache/CacheStorage.cpp:130 > + return; So in this case the promise is not resolved? > Source/WebCore/Modules/cache/CacheStorage.cpp:170 > + return !m_cacheMap.size() && !hasPendingActivity(); m_cacheMap.isEmpty() > Source/WebCore/Modules/cache/CacheStorage.h:58 > + void refreshCacheMap(WTF::Function<void()>&&); This name really does not make it clear what the lambda parameter is for. > Source/WebCore/Modules/cache/CacheStorage.h:61 > + Vector<Ref<Cache>> m_cacheMap; Confusing naming given this is a Vector. > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:41 > + return Exception { TypeError, ASCIILiteral("Not implemented") }; NotSupportedError? > Source/WebCore/Modules/cache/CacheStorageConnection.h:41 > + enum class Error { Maybe it's just a personal preference but I think it's look better if we used std::optional<Error> rather than having a None value in the enum. > Source/WebCore/Modules/cache/CacheStorageConnection.h:67 > + static Ref<CacheStorageConnection> create() { return adoptRef(*new CacheStorageConnection()); } We tend to have this one come first in our classes, for readability. Also, you should make the constructor protected to force call sites to use your factory. protected: CacheStorageConnection() = default; > Source/WebCore/Modules/cache/CacheStorageConnection.h:71 > + using CacheMapCallback = WTF::Function<void(Vector<CacheInfo>&&)>; Confusing naming given the type. > Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37 > + WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { }; Should not be on one line. > Source/WebCore/Modules/fetch/FetchRequest.cpp:77 > + if (!init.window.isUndefinedOrNull() && !init.window.isEmpty()) Can't we just use jsNull() at the call site? Or default initialize window to jsNull() in the init struct? > Source/WebCore/Modules/fetch/FetchRequest.h:56 > + struct InternalRequest { It is confusing to have an "Internal" request exposed in the public API. > Source/WebCore/Modules/fetch/FetchRequest.h:91 > + const URL& internalRequestURL() const { return m_internalRequest.request.url(); } The relationship between this getter and the url() one is unclear. Can we call the other one urlString() and call this one url() ? > Source/WebCore/page/CacheStorageProvider.h:36 > + static Ref<CacheStorageProvider> create() { return adoptRef(*new CacheStorageProvider); } You also need to make the default constructor protected / private. > Source/WebKit/WebProcess/Cache/WebCacheStorageProvider.h:34 > + static Ref<WebCacheStorageProvider> create() { return adoptRef(*new WebCacheStorageProvider); } You also need to make the default constructor private.
Comment on attachment 318026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318026&action=review > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:70 > + if (varyingHeaderNames.contains("*")) We could do this check in the loop below to avoid iterating over the vector twice. > Source/WebCore/Modules/cache/DOMWindowCaches.cpp:-62 > - if (!m_caches && frame()) We normally have "if (isCurrentlyDisplayedInFrame()) return nullptr;" in DOMWindowProperty getters to make sure the Window is still the one displayed in the frame (frame is reused in case of navigation). You can then drop the frame() and frame()->document() checks below. > Source/WebCore/Modules/fetch/FetchResponse.h:96 > + const ResourceResponse& response() const { return m_response; } Given this: ResourceRequest FetchRequest::resourceRequest() const; I would call this one resourceResponse() for consistency. > Source/WebKit/WebProcess/Cache/WebCacheStorageProvider.h:2 > + * Copyright (C) 2016 Apple Inc. All rights reserved. 2017?
Created attachment 318090 [details] Patch
Attachment 318090 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:92: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:121: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:49: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:289: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 62 files If any of these errors are false positives, please file a bug against check-webkit-style.
Thanks for the review. (In reply to Chris Dumez from comment #28) > Comment on attachment 318026 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318026&action=review > > > Source/WebCore/Modules/cache/Cache.cpp:79 > > + refreshRequestToResponseMap([this, promise = WTFMove(promise)]() { > > I think refreshRequestToResponseMap is confusingly named. It is really hard > for me to understand what it does. RequestToResponseMap is spec jargon. I changed to to Records so the method is renamed retrieveRecords. > > > Source/WebCore/Modules/cache/Cache.cpp:90 > > + if (records.size()) { > > !records.isEmpty() OK > > Source/WebCore/Modules/cache/Cache.cpp:139 > > + varyValue.split(',', false, [&](const StringView& view) { > > We usually pass StringView type by value. OK > > Source/WebCore/Modules/cache/Cache.cpp:140 > > + if (!hasVaryHeaderStar && view == "*") > > Looks like you are forgetting to update varyingHeaderNames Vector in your > functor? varyingHeaderNames is a left over from a previous patch. I removed it. > > Source/WebCore/Modules/cache/Cache.cpp:171 > > + batchPutOperation(*request, response.get(), [promise = WTFMove(promise)](CacheStorageConnection::Error error) mutable { > > Is "batch" the right naming here? Are you really batching or merely queueing? batch operation is reusing spec terminology. When implementing addAll, batchPutOperation will take a vector of records. > > Source/WebCore/Modules/cache/Cache.cpp:287 > > + callback(!!records.size(), error); > > !records.isEmpty() OK > > Source/WebCore/Modules/cache/Cache.cpp:326 > > + Vector<CacheStorageRecord> newMap; > > confusing naming. Changed to records. > > Source/WebCore/Modules/cache/Cache.cpp:358 > > + return !m_requestToResponseMap.size() && !hasPendingActivity(); > > m_requestToResponseMap.isEmpty() > > > Source/WebCore/Modules/cache/Cache.h:46 > > + enum class MatchType { All, OnlyFirst }; > > Could be private if comment below is applied. OK > > Source/WebCore/Modules/cache/Cache.h:47 > > + void match(RequestInfo&&, CacheQueryOptions&&, Ref<DeferredPromise>&&); > > Why isn't this const? match is not really const here since we are retrieving the list of records. We could try to make it const by making the records field mutable. Not sure this is worth it. > > Source/WebCore/Modules/cache/Cache.h:48 > > + void matchAll(std::optional<RequestInfo>&&, CacheQueryOptions&&, Ref<DeferredPromise>&&, MatchType = MatchType::All); > > Having a method named matchAll() which takes a MatchType parameter which can > be OnlyFirst is confusing. My suggestion would be to have a private method > that takes the MatchType parameter and have it called from match() / > matchAll() using the right matchType. We could call this private method > something like matchForType(). Changed. > Also, why isn't this const? See above. > > Source/WebCore/Modules/cache/Cache.h:54 > > + void keys(std::optional<RequestInfo>&&, CacheQueryOptions&&, KeysPromise&&); > > Why isn't this const? Ditto. > > Source/WebCore/Modules/cache/Cache.h:67 > > + void refreshRequestToResponseMap(WTF::Function<void()>&&); > > This method name is confusing. > > > Source/WebCore/Modules/cache/Cache.h:73 > > + void updateRequestToResponseMap(Vector<CacheStorageConnection::Record>&&); > > This method name is confusing. > > > Source/WebCore/Modules/cache/Cache.h:79 > > + Vector<CacheStorageRecord> m_requestToResponseMap; > > Why is this called map? The variable naming is confusing given its Vector > type. Could it something like m_cacheStorageRecords? It is a map, sorted by insertion order, which ends up being implemented as a vector in WebKit. I changed it to m_records. > > Source/WebCore/Modules/cache/CacheStorage.cpp:61 > > +void CacheStorage::refreshCacheMap(WTF::Function<void()>&& callback) > > refreshCacheMapThen? Otherwise, it is really unclear what the lambda is for. Used retrieveCaches. > > Source/WebCore/Modules/cache/CacheStorage.cpp:68 > > + m_connection->refreshCacheMap(origin, [this, callback = WTFMove(callback)](Vector<CacheStorageConnection::CacheInfo>&& cachesInfo) { > > refreshCacheMapThen? > > > Source/WebCore/Modules/cache/CacheStorage.cpp:101 > > + return; > > So in this case we do not reject the promise? Changed it to an assertion. > > Source/WebCore/Modules/cache/CacheStorage.cpp:130 > > + return; > > So in this case the promise is not resolved? Ditto, the callback should not be called if we en up with a null string. > > Source/WebCore/Modules/cache/CacheStorage.cpp:170 > > + return !m_cacheMap.size() && !hasPendingActivity(); > > m_cacheMap.isEmpty() Removed m_cacheMap.isEmpty() since each cache will know whether to suspend or not by itself. > > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:41 > > + return Exception { TypeError, ASCIILiteral("Not implemented") }; > > NotSupportedError? OK > > Source/WebCore/Modules/cache/CacheStorageConnection.h:41 > > + enum class Error { > > Maybe it's just a personal preference but I think it's look better if we > used std::optional<Error> rather than having a None value in the enum. > > > Source/WebCore/Modules/cache/CacheStorageConnection.h:67 > > + static Ref<CacheStorageConnection> create() { return adoptRef(*new CacheStorageConnection()); } > > We tend to have this one come first in our classes, for readability. Changed it. > Also, you should make the constructor protected to force call sites to use > your factory. OK > protected: > CacheStorageConnection() = default; > > > Source/WebCore/Modules/cache/CacheStorageConnection.h:71 > > + using CacheMapCallback = WTF::Function<void(Vector<CacheInfo>&&)>; > > Confusing naming given the type. > > > Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37 > > + WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { }; > > Should not be on one line. > > > Source/WebCore/Modules/fetch/FetchRequest.cpp:77 > > + if (!init.window.isUndefinedOrNull() && !init.window.isEmpty()) > > Can't we just use jsNull() at the call site? Or default initialize window to > jsNull() in the init struct? We could, but it seems unnecessary to include JSC headers just for that. > > Source/WebCore/Modules/fetch/FetchRequest.h:56 > > + struct InternalRequest { > > It is confusing to have an "Internal" request exposed in the public API. Removed the getter. We should remove this struct at some point. > > Source/WebCore/Modules/fetch/FetchRequest.h:91 > > + const URL& internalRequestURL() const { return m_internalRequest.request.url(); } > > The relationship between this getter and the url() one is unclear. Can we > call the other one urlString() and call this one url() ? Changed it. > > Source/WebCore/page/CacheStorageProvider.h:36 > > + static Ref<CacheStorageProvider> create() { return adoptRef(*new CacheStorageProvider); } > > You also need to make the default constructor protected / private. OK > > Source/WebKit/WebProcess/Cache/WebCacheStorageProvider.h:34 > > + static Ref<WebCacheStorageProvider> create() { return adoptRef(*new WebCacheStorageProvider); } > > You also need to make the default constructor private. OK (In reply to Chris Dumez from comment #29) > Comment on attachment 318026 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318026&action=review > > > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:70 > > + if (varyingHeaderNames.contains("*")) > > We could do this check in the loop below to avoid iterating over the vector > twice. OK > > Source/WebCore/Modules/cache/DOMWindowCaches.cpp:-62 > > - if (!m_caches && frame()) > > We normally have "if (isCurrentlyDisplayedInFrame()) return nullptr;" in > DOMWindowProperty getters to make sure the Window is still the one displayed > in the frame (frame is reused in case of navigation). > > You can then drop the frame() and frame()->document() checks below. OK > > Source/WebCore/Modules/fetch/FetchResponse.h:96 > > + const ResourceResponse& response() const { return m_response; } > > Given this: > ResourceRequest FetchRequest::resourceRequest() const; > > I would call this one resourceResponse() for consistency. OK > > Source/WebKit/WebProcess/Cache/WebCacheStorageProvider.h:2 > > + * Copyright (C) 2016 Apple Inc. All rights reserved. > > 2017? OK
Created attachment 318092 [details] Patch
Created attachment 318093 [details] Patch
Attachment 318093 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:92: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:121: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:49: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:289: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 318113 [details] Patch
Attachment 318113 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:92: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:121: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:49: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:289: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 318113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318113&action=review r=me with comments > Source/WebCore/Modules/cache/Cache.cpp:143 > + varyValue.split(',', false, [&](StringView view) { Vary header usually looks like so: Vary: accept-encoding, accept-language Because there is often a space after the comma, the code is is not sufficient. I believe you should strip surrounding spaces before doing comparison. > Source/WebCore/Modules/cache/Cache.cpp:236 > + queryCache(request.releaseNonNull(), WTFMove(options), [promise = WTFMove(promise)](const Vector<CacheStorageRecord>& records) mutable { const auto& records would like work. > Source/WebCore/Modules/cache/Cache.cpp:302 > + cachedResponse.setHTTPHeaderField(header.key, header.value); This merges existing headers and new ones. > Source/WebCore/Modules/cache/Cache.cpp:305 > + cachedRequest.setHTTPHeaderFields(request.headers().internalHeaders()); This replaces existing headers with new ones. Which behavior is right? > Source/WebCore/Modules/cache/Cache.cpp:362 > + return !m_records.size() && !hasPendingActivity(); m_records.isEmpty() > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:72 > + if (name == "*") Vary header usually looks like so: Vary: accept-encoding, accept-language Because there is often a space after the comma, the code is is not sufficient. I believe you should strip surrounding spaces before doing comparison. See stripLeadingAndTrailingHTTPSpaces() in HTTPParsers.h. > Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37 > + WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { }; Should not be on one line.
Thanks for the review. (In reply to Chris Dumez from comment #38) > Comment on attachment 318113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318113&action=review > > r=me with comments > > > Source/WebCore/Modules/cache/Cache.cpp:143 > > + varyValue.split(',', false, [&](StringView view) { > > Vary header usually looks like so: > Vary: accept-encoding, accept-language > > Because there is often a space after the comma, the code is is not > sufficient. I believe you should strip surrounding spaces before doing > comparison. Right, I'll do the easy way and add a FIXME since I guess StringView should be able to strip whitespaces as well. > > Source/WebCore/Modules/cache/Cache.cpp:236 > > + queryCache(request.releaseNonNull(), WTFMove(options), [promise = WTFMove(promise)](const Vector<CacheStorageRecord>& records) mutable { > > const auto& records would like work. > > > Source/WebCore/Modules/cache/Cache.cpp:302 > > + cachedResponse.setHTTPHeaderField(header.key, header.value); > > This merges existing headers and new ones. This does not matter in practice. There is a FIXME though to set all http header fields like done for ResourceRequest. > > > Source/WebCore/Modules/cache/Cache.cpp:305 > > + cachedRequest.setHTTPHeaderFields(request.headers().internalHeaders()); > > This replaces existing headers with new ones. > > Which behavior is right? In this particular context, they should be no difference in behavior. This particular implementation is better though. Hence fix above. > > Source/WebCore/Modules/cache/Cache.cpp:362 > > + return !m_records.size() && !hasPendingActivity(); > > m_records.isEmpty() OK > > Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37 > > + WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { }; > > Should not be on one line. I like the one-line style. It also seems used in many different headers in WebKit. Should we update the style document to accept this?
Comment on attachment 318113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318113&action=review >>> Source/WebCore/Modules/cache/Cache.cpp:143 >>> + varyValue.split(',', false, [&](StringView view) { >> >> Vary header usually looks like so: >> Vary: accept-encoding, accept-language >> >> Because there is often a space after the comma, the code is is not sufficient. I believe you should strip surrounding spaces before doing comparison. > > Right, I'll do the easy way and add a FIXME since I guess StringView should be able to strip whitespaces as well. No, I believe Darin objected to doing this in https://bugs.webkit.org/show_bug.cgi?id=171731 so don't add a FIXME. I think you'll need to use a String. >>> Source/WebCore/Modules/cache/WorkerGlobalScopeCaches.h:37 >>> + WorkerGlobalScopeCaches(WorkerGlobalScope& scope) : m_scope(scope) { }; >> >> Should not be on one line. > > I like the one-line style. > It also seems used in many different headers in WebKit. > Should we update the style document to accept this? This is not the WebKit coding style. If you want to start the mail thread about allowing this, go for it (I personally do not think it looks good on one line). But in the mean time, PLEASE follow the WebKit coding style. The fact that it is used elsewhere is not evidence that it is OK to do so.
Created attachment 318151 [details] Patch
Attachment 318151 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:57: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:72: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:92: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorage.cpp:121: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/Modules/cache/CacheStorageConnection.h:49: The parameter name "request" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/Modules/cache/Cache.cpp:289: More than one command on the same line [whitespace/newline] [4] Total errors found: 6 in 61 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 318151 [details] Patch Clearing flags on attachment: 318151 Committed r220758: <http://trac.webkit.org/changeset/220758>
All reviewed patches have been landed. Closing bug.
<rdar://problem/33903467>