Use NetworkSession for pings when using NetworkSession
Created attachment 272779 [details] Patch
Created attachment 272786 [details] Patch
Comment on attachment 272786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272786&action=review > Source/WebKit2/NetworkProcess/PingLoad.h:71 > + virtual void willPerformHTTPRedirection(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, RedirectCompletionHandler completionHandler) > + { > + completionHandler({ }); > + delete this; > + } > + virtual void didReceiveChallenge(const WebCore::AuthenticationChallenge&, ChallengeCompletionHandler completionHandler) > + { > + completionHandler(AuthenticationChallengeDisposition::Cancel, { }); > + delete this; > + } > + virtual void didReceiveResponseNetworkSession(const WebCore::ResourceResponse&, ResponseCompletionHandler completionHandler) > + { > + completionHandler(WebCore::PolicyAction::PolicyIgnore); > + delete this; > + } > + virtual void didReceiveData(RefPtr<WebCore::SharedBuffer>&&) { ASSERT_NOT_REACHED(); } > + virtual void didCompleteWithError(const WebCore::ResourceError&) { delete this; } > + virtual void didBecomeDownload() { ASSERT_NOT_REACHED(); } > + virtual void didSendData(uint64_t totalBytesSent, uint64_t totalBytesExpectedToSend) { ASSERT_NOT_REACHED(); } > + virtual void wasBlocked() { delete this; } > + virtual void cannotShowURL() { delete this; } These should all be marked override, and not virtual.
Created attachment 272833 [details] Patch
Comment on attachment 272833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272833&action=review > Source/WebKit2/NetworkProcess/PingLoad.h:69 > + void didSendData(uint64_t totalBytesSent, uint64_t totalBytesExpectedToSend) override { ASSERT_NOT_REACHED(); } This assertion was being hit in many tests. Will commit without it if reviewed.
Comment on attachment 272833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272833&action=review > Source/WebKit2/ChangeLog:3 > + Use NetworkSession for pings when using NetworkSession The title could be reformulated with just one 'NetworkSession' > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:145 > RefPtr<NetworkingContext> context = RemoteNetworkingContext::create(loadParameters.sessionID, loadParameters.shouldClearReferrerOnHTTPSToHTTPRedirect); > > +#if USE(NETWORK_SESSION) > + // PingLoad manages its own lifetime, deleting itself when its purpose has been fulfilled. > + new PingLoad(loadParameters); > +#else > // PingHandle manages its own lifetime, deleting itself when its purpose has been fulfilled. > new PingHandle(context.get(), loadParameters.request, loadParameters.allowStoredCredentials == AllowStoredCredentials, PingHandle::UsesAsyncCallbacks::Yes); > +#endif RemoteNetworkingContext doesn't seems to be used in USE(NETWORK_SESSION) path. It should be moved inside the #else branch.
http://trac.webkit.org/changeset/197568
Comment on attachment 272833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272833&action=review > Source/WebKit2/NetworkProcess/PingLoad.h:34 > +class PingLoad : public NetworkDataTaskClient { Should mark the class final. Also, I think that it could be private inheritance rather than public. > Source/WebKit2/NetworkProcess/PingLoad.h:36 > + PingLoad(const NetworkResourceLoadParameters& parameters) Typically we’d want to mark this explicit. Also, in a class that deletes itself like this, I think it’s better to only expose a static member function to trigger this load and not return the pointer. That way the caller doesn’t have to write what looks like incorrect code. > Source/WebKit2/NetworkProcess/PingLoad.h:37 > + : m_timeoutTimer(*this, &PingLoad::timeoutTimerFired) I new code, a bit more elegant to write this with a lambda: : m_timeoutTimer(*this, [this]() { delete this; }) > Source/WebKit2/NetworkProcess/PingLoad.h:43 > + if (auto* networkSession = SessionTracker::networkSession(parameters.sessionID)) { > + m_task = NetworkDataTask::create(*networkSession, *this, parameters.request, parameters.allowStoredCredentials, parameters.contentSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect); > + m_task->resume(); > + } else > + ASSERT_NOT_REACHED(); The if does us no good. If you really want an assertion then I’d just write: auto* networkSession = SessionTracker::networkSession(parameters.sessionID); ASSERT(networkSession); ... But I’d suggest not bothering with the assertion.