[webkit-reviews] review granted: [Bug 178491] Create a Fetch event when ServiceWorker has to handle a fetch : [Attachment 324317] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 19 19:01:53 PDT 2017
Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 178491: Create a Fetch event when ServiceWorker has to handle a fetch
https://bugs.webkit.org/show_bug.cgi?id=178491
Attachment 324317: Patch
https://bugs.webkit.org/attachment.cgi?id=324317&action=review
--- Comment #15 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 324317
--> https://bugs.webkit.org/attachment.cgi?id=324317
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=324317&action=review
r=me with comments.
> Source/WebCore/platform/network/ResourceResponseBase.h:70
> + WEBCORE_EXPORT CrossThreadData crossThreadData() const;
No longer needed.
> Source/WebCore/platform/network/ResourceResponseBase.h:71
> + WEBCORE_EXPORT static ResourceResponse
fromCrossThreadData(CrossThreadData&&);
No longer needed.
> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:27
> +#include "ServiceWorkerFetch.h"
We usually put this inside the #if ENABLE().
> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:64
> +void processResponse(Ref<Client>&& client, FetchResponse* response)
Why the declaration at the top? I think we usually put the static functions at
the top of the file to avoid needing such declarations.
> Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp:110
> +}
// namespace ServiceWorkerFetch
> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:96
> +void
ServiceWorkerThread::dispatchFetchTask(Ref<ServiceWorkerFetch::Client>&&
client, ResourceRequest&& request, FetchOptions&& options)
Why dispatchFetchTask and not dispatchFetchEvent ?
> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:27
> +#include "WebServiceWorkerFetchTaskClient.h"
This usually goes inside the #if ENABLE()
> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:41
> + ~WebServiceWorkerFetchTaskClient();
I think we should add a blank line before.
> LayoutTests/http/tests/workers/service/resources/basic-fetch.js:6
> +function log(msg)
We have this in 2 tests now (basic-fetch.js and basic-register.js). Could you
please move this function to sw-test-pre.js to avoid code duplication?
More information about the webkit-reviews
mailing list