[webkit-reviews] review granted: [Bug 27165] Connections-per-host should be tracked closer to the ResourceHandle level, not the Cache : [Attachment 68132] Address comments + don't decrement in releaseResources() if m_request.isNull()

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 27165: Connections-per-host should be tracked closer to the ResourceHandle
level, not the Cache
https://bugs.webkit.org/show_bug.cgi?id=27165

Attachment 68132: Address comments + don't decrement in releaseResources() if
m_request.isNull()
https://bugs.webkit.org/attachment.cgi?id=68132&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=68132&action=review

Looks great.  Thanks.  This is subtle stuff, so it seems likely we've screwed
at least one thing up.

> WebCore/loader/ResourceLoader.cpp:133
> +    // For http(s) hosts, we should always enforce the connection limit.
> +    // For non-http(s) hosts, we should only enforce the limit if the
document isn't done parsing and we don't know all stylesheets yet.
> +    bool shouldLimitRequests = clientRequest.url().protocolInHTTPFamily() ||
(m_frame->document() && (m_frame->document()->parsing() ||
!m_frame->document()->haveStylesheetsLoaded()));

m_frame->document() is always non-NULL.

Did you verify that this piece of complexity is still needed?

> WebCore/loader/OpenConnectionLimiter.h:45
> +};

Missing newline after this line.

> WebCore/loader/OpenConnectionLimiter.cpp:70
> +    requestsPerHostMap().set(host, requestsPerHostMap().get(host) + 1);

There's now operator[] on this object that returns a non-const reference?

requestsPerHostMap()[host]++ ?