[webkit-reviews] review granted: [Bug 178475] Add preliminary support for ServiceWorker Handle Fetch : [Attachment 324178] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 18 16:31:56 PDT 2017


Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 178475: Add preliminary support for ServiceWorker Handle Fetch
https://bugs.webkit.org/show_bug.cgi?id=178475

Attachment 324178: Patch

https://bugs.webkit.org/attachment.cgi?id=324178&action=review




--- Comment #16 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 324178
  --> https://bugs.webkit.org/attachment.cgi?id=324178
Patch

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

r=me with comments.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:80
> +    void scheduleLoadFromNetworkProcess(WebCore::ResourceLoader&, const
WebCore::ResourceRequest&, const WebResourceLoader::TrackingParameters&, bool
shouldClearReferrerOnHTTPSToHTTPRedirect, Seconds);

I think we should provide the name of the last parameter as it is non-obvious.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:53
> +    if (!(response.httpStatusCode() <= 300 || response.httpStatusCode() >=
400 || response.httpStatusCode() == 304 || response.httpStatusCode() == 305 ||
response.httpStatusCode() == 306)) {

Can we add a isRedirect() method to ResourceResponseBase so this is reusable?
Or a isRedirectResponse(response) function is some Webcore header?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:67
> +    if (auto callback = WTFMove(m_callback))

Why are we calling the completion callback with success here and not in
didFinish()?

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.h:64
> +    RefPtr<WebCore::ResourceLoader> m_loader;

It looks like m_loader cannot currently be null. If so, it should probably be a
Ref<> and we should get rid of all the null checks.

> Source/WebKit/WebProcess/Storage/WebServiceWorkerProvider.cpp:90
> +    auto fetch = connection.startFetch(*this, loader, loader.identifier(),
WTFMove(callback));

This is a risky pattern. It is common for networking code to fail
synchronously, in which case, you'd try to remove the object from
m_ongoingFetchTasks (and fail silently) and then add it to m_ongoingFetchTasks
even though it is already finished.

> Source/WebKit/WebProcess/WebProcess.h:443
> +    std::optional<ServiceWorkerContextManager> m_serviceWorkerManager;

Why optional and not unique_ptr?


More information about the webkit-reviews mailing list