[Webkit-unassigned] [Bug 27165] Connections-per-host should be tracked closer to the ResourceHandle level, not the Cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27165


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70122|review?                     |review-
               Flag|                            |




--- Comment #23 from Adam Barth <abarth at webkit.org>  2010-10-07 11:34:08 PST ---
(From update of attachment 70122)
View in context: https://bugs.webkit.org/attachment.cgi?id=70122&action=review

This direction is great.  Mostly minor aesthetic comments and a question.

> WebCore/loader/MainResourceLoader.cpp:553
> +    openConnectionLimiter()->addRequestWithoutSchedule(this);

openConnectionLimiter()->addRequest(DoNotSchedule) ?

> WebCore/loader/OpenConnectionLimiter.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

You probably need the copyright lines from Loader.h/cpp in these files.

> WebCore/loader/OpenConnectionLimiter.h:47
> +class OpenConnectionLimiter : public Noncopyable {

Maybe OpenConnectionLimiter -> ResourceLoadScheduler ?

> WebCore/loader/OpenConnectionLimiter.h:49
> +    friend OpenConnectionLimiter* openConnectionLimiter();

Interesting.  Yeah, I think that's the pattern we use here.  Other code bases would make this a static function, but this seems fine.

> WebCore/loader/OpenConnectionLimiter.h:64
> +    ~OpenConnectionLimiter();

Add a blank line below here.

> WebCore/loader/OpenConnectionLimiter.h:70
> +    class Host : public RefCounted<Host> {

Host is a confusing name.  Maybe HostInformation?  This class is just data-only, right?

> WebCore/loader/OpenConnectionLimiter.h:81
> +        void servePendingRequests(Priority minimumPriority = VeryLow);

I see, this isn't model-like.  Can we move this into the parent class?  That way the parent class is the controller and this class is the model.

Also, is there a big benefit to having this be an inner class?  It seems like it would be easier if it had it's own header and file.

> WebCore/loader/OpenConnectionLimiter.h:82
> +        void end(ResourceLoader*, OpenConnectionLimiter::Priority);

OpenConnectionLimiter:: isn't needed here.

> WebCore/loader/ResourceLoader.h:133
> +        friend class OpenConnectionLimiter;

sadface

> WebCore/loader/OpenConnectionLimiter.cpp:75
> +    static OpenConnectionLimiter* openConnectionLimiter = new OpenConnectionLimiter();

DECLARE_STATIC_LOCAL

> WebCore/loader/OpenConnectionLimiter.cpp:100
> +        m_hosts.checkConsistency();
> +        String hostName = url.host();
> +        host = m_hosts.get(hostName);
> +        if (!host) {
> +            host = Host::create(hostName, maxRequestsInFlightPerHost);
> +            m_hosts.add(hostName, host);
> +        }

This seems like it should be in a helper function.

> WebCore/loader/OpenConnectionLimiter.cpp:128
> +    if (url.protocolInHTTPFamily()) {
> +        m_hosts.checkConsistency();
> +        host = m_hosts.get(url.host());
> +    } else 
> +        host = m_nonHTTPProtocolHost;

Another instance of this url => Host lookup.

> WebCore/loader/OpenConnectionLimiter.cpp:145
> +    RefPtr<Host> oldHost;
> +    if (oldURL.protocolInHTTPFamily()) {
> +        m_hosts.checkConsistency();
> +        oldHost = m_hosts.get(oldURL.host());
> +    } else 
> +        oldHost = m_nonHTTPProtocolHost;

:)

> WebCore/loader/OpenConnectionLimiter.cpp:156
> +        m_hosts.checkConsistency();
> +        String hostName = newURL.host();
> +        newHost = m_hosts.get(hostName);
> +        if (!newHost) {
> +            newHost = Host::create(hostName, maxRequestsInFlightPerHost);
> +            m_hosts.add(hostName, newHost);
> +        }

Here's the same thing but with the "create" bit set.

> WebCore/loader/OpenConnectionLimiter.cpp:182
> +    RefPtr<Host> host;
> +    KURL url(ParsedURLString, resourceLoader->request().url());
> +    if (url.protocolInHTTPFamily()) {
> +        m_hosts.checkConsistency();
> +        String hostName = url.host();
> +        host = m_hosts.get(hostName);
> +        if (!host) {
> +            host = Host::create(hostName, maxRequestsInFlightPerHost);
> +            m_hosts.add(hostName, host);
> +        }
> +    } else 
> +        host = m_nonHTTPProtocolHost;

:)

> WebCore/loader/OpenConnectionLimiter.cpp:207
> +            host->servePendingRequests(minimumPriority);

Yeah, we can probably move this controller logic from Host to here.

> WebCore/loader/OpenConnectionLimiter.cpp:299
> +void OpenConnectionLimiter::Host::end(ResourceLoader* resourceLoader, OpenConnectionLimiter::Priority priority)

end -> remove ?

> LayoutTests/http/tests/xmlhttprequest/logout.html:32
> -    xhr.abort();
> +    xhr.onload = function() {
> +        document.getElementById("logout").innerHTML = isAuthenticated() ? "FAIL" : "PASS";
> +
> +        if (window.layoutTestController)
> +            layoutTestController.notifyDone();
> +    }

Is our new behavior more consistent or less consistent with other browsers?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.