Bug 150857

Created attachment 264726 [details] Patch

Created attachment 264728 [details] Patch

Created attachment 264731 [details] Patch

Comment on attachment 264728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264728&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:195 > + RELEASE_ASSERT(networkSession); Why do we need to assert here at all? Particularly, why a RELEASE_ASSERT? > Source/WebKit2/NetworkProcess/NetworkSession.h:95 > + explicit NetworkDataTask(NetworkSession&, NetworkSessionTaskClient&, RetainPtr<NSURLSessionDataTask>); The argument to this function should just be NSURLSessionDataTask *, not a RetainPtr. If you were trying to save retain count churn, maybe RetainPtr&&. > Source/WebKit2/NetworkProcess/NetworkSession.h:115 > + NetworkSession(Type, WebCore::SessionID); > + ~NetworkSession(); A little non-standard in style to declare the constructor and destructor after declaring other member functions. > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:177 > +static std::unique_ptr<NetworkSession>& defaultNetworkSession() > +{ > + ASSERT(isMainThread()); > + static NeverDestroyed<std::unique_ptr<NetworkSession>> session; > + return session; > +} No need for this helper function. > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:183 > + if (!defaultNetworkSession()) > + defaultNetworkSession() = std::make_unique<NetworkSession>(NetworkSession::Type::Normal, WebCore::SessionID::defaultSessionID()); > + return *defaultNetworkSession(); No helper function or make_unique needed: ASSERT(isMainThread()); static NeverDestroyed<NetworkSession> session(NetworkSession::Type::Normal, WebCore::SessionID::defaultSessionID()); return session; > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:202 > + RELEASE_ASSERT(dataTask); Why do we need to assert here at all? Particularly, why a RELEASE_ASSERT?

Created attachment 264738 [details] Patch

Created attachment 264739 [details] Patch

Comment on attachment 264739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264739&action=review > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:195 > + for (auto* dataTask : m_dataTaskMap.values()) > + dataTask->cancel(); I'm not sure if this is correct. I had an ASSERT(m_dataTaskMap.isEmpty()) before, but I wasn't sure that will always be true. Now I think it might.

Created attachment 264753 [details] Patch

Comment on attachment 264753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264753&action=review > Source/WebKit2/NetworkProcess/NetworkSession.h:103 > + friend class NetworkDataTask; Why friend? > Source/WebKit2/Shared/SessionTracker.cpp:85 > + return map.get(); I think you can just return map here.

(In reply to comment #9) > Comment on attachment 264753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264753&action=review > > > Source/WebKit2/NetworkProcess/NetworkSession.h:103 > > + friend class NetworkDataTask; > > Why friend? A NetworkDataTask adds itself to m_dataTaskMap in its constructor and removes itself in the destructor. > > > Source/WebKit2/Shared/SessionTracker.cpp:85 > > + return map.get(); > > I think you can just return map here. Yep, and a few other places in this file.