[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