[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