Summary: | Create a NetworkSession for each SessionID | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, cdumez | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 150834 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-11-03 14:23:26 PST
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. |