[webkit-reviews] review granted: [Bug 223776] Refactor NetworkSessionCocoa to prepare for per-WebPageProxy sessions : [Attachment 424319] Patch for EWS

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted  review:
Bug 223776: Refactor NetworkSessionCocoa to prepare for per-WebPageProxy
sessions
https://bugs.webkit.org/show_bug.cgi?id=223776

Attachment 424319: Patch for EWS

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




--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 424319
  --> https://bugs.webkit.org/attachment.cgi?id=424319
Patch for EWS

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:159
> +    HashMap<WebPageProxyIdentifier, RefPtr<SessionSet>>
m_perPageSessionSets;

I think the value could be made a Ref

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1298
> +    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=177394

You may as well remove this comment and the 2 lines below.  We know why we
can't do this, and we typically don't like having commented out code in WebKit.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1307
> +    SessionSet* sessionSet = webPageProxyID ?
m_perPageSessionSets.get(webPageProxyID) : nullptr;

If webPageProxyID can be 0 then it should probably be an
Optional<WebPageProxyIdentifier>
If you're protecting against strange things happening to the HashTable, you
should probably use decltype(m_perPageSessionSets.get)::isValidKey

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1319
> +    return
sessionSetForPage(pageID).initializeEphemeralStatelessSessionIfNeeded(isNavigat
ingToAppBoundDomain, *this);

You call pageID webPageProxyID everywhere else in this patch.  Why not here?