Make ServiceWorkerClientFetch closer to WebResourceLoader

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Now that service worker registrations are in the network process, we could consider serve service worker content through the regular network process loading code path.
The first step is to make ServiceWorkerClientFetch closer to WebResourceLoader.

[youenn fablet]

Created attachment 362019 [details]
Patch

[youenn fablet]

Created attachment 362067 [details]
Patch

[Alex Christensen]

Comment on attachment 362067 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362067&action=review

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:220
> +    }, WorkerRunLoop::defaultMode());

Could we make this a default parameter?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:65
> +    void didReceiveRedirectResponse(WebCore::ResourceResponse&&);

Do we want to just inform that a redirect happened, or will we in the future have a need for the service worker to be able to reject the redirect?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:201
> +        auto formData = WTFMove(m_formData);
> +        didReceiveFormDataAndFinish(formData.releaseNonNull());

Could we just call m_formData.releaseNonNull() without WTFMove?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:86
> +    RefPtr<WebCore::SharedBuffer> m_buffer;
> +    RefPtr<FormData> m_formData;

Can these be a Variant, or could we have more than one of these?

> Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:88
> +    Optional<ResourceError> m_error;

Making this a std::unique_ptr would save memory in the common case.

[youenn fablet]

Created attachment 362140 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to Alex Christensen from comment #3)
> Comment on attachment 362067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362067&action=review
> 
> > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:220
> > +    }, WorkerRunLoop::defaultMode());
> 
> Could we make this a default parameter?

Sure, could be done as a separate patch.

> > Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:65
> > +    void didReceiveRedirectResponse(WebCore::ResourceResponse&&);
> 
> Do we want to just inform that a redirect happened, or will we in the future
> have a need for the service worker to be able to reject the redirect?

When fetch event respondWith is called, there is no way back.
A DocumentLoader may trigger another service worker fetch which will be treated as another fetch event.

> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.cpp:201
> > +        auto formData = WTFMove(m_formData);
> > +        didReceiveFormDataAndFinish(formData.releaseNonNull());
> 
> Could we just call m_formData.releaseNonNull() without WTFMove?
> 
> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:86
> > +    RefPtr<WebCore::SharedBuffer> m_buffer;
> > +    RefPtr<FormData> m_formData;
> 
> Can these be a Variant, or could we have more than one of these?
> 
> > Source/WebKit/WebProcess/Storage/WebServiceWorkerFetchTaskClient.h:88
> > +    Optional<ResourceError> m_error;
> 
> Making this a std::unique_ptr would save memory in the common case.

I updated the patch to make all of these a Variant with proper Ref<>, UniqueRef<>.

[WebKit Commit Bot]

Comment on attachment 362140 [details]
Patch for landing

Clearing flags on attachment: 362140

Committed r241603: <https://trac.webkit.org/changeset/241603>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48120172>

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 362146 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362146 [details]
Patch

Clearing flags on attachment: 362146

Committed r241609: <https://trac.webkit.org/changeset/241609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.