[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





--- Comment #33 from Nate Chapin <japhet at chromium.org>  2010-10-18 15:31:15 PST ---
Sorry for the delay....a few comments/questions on the less-trivial requests

(In reply to comment #31)
> (From update of attachment 70314 [details])
> +ResourceLoadScheduler::Priority ResourceLoadScheduler::determinePriority(const ResourceRequest& request) const
> +{
> +    switch (request.targetType()) {
> +    case ResourceRequest::TargetIsMainFrame:
> 
> Ugh, this is horrible. ResourceRequest::m_targetType is misguided code - this member variable isn't guaranteed to be accurate. The reason is that Mac WebKit API sends every request to embedder as an NSURLRequest, and the embedder is free to do anything with it. After the call returns, ResourceRequest is re-created from NSURLRequest, so anything ResourceRequest::doUpdateResourceRequest() doesn't know about is lost.
> 
> It's not good design to have cross-platform code with such strong Mac ties, but that's what we have now (plus misguided code from developers who chose to ignore this limitation after having been told about it). We should at least have comments in ResourceRequestBase.h - or better, come up with a strategy for dealing with this. See also: bug 44722 comment 49.
> 
> In any case, request.targetType() shouldn't be used in ResourceLoadScheduler now.
>
> 
> -    m_handle = ResourceHandle::create(m_frame->loader()->networkingContext(), clientRequest, this, m_defersLoading, m_shouldContentSniff);
> 
> +    resourceLoadScheduler()->add(this);
> 
> So, ResourceLoader has a circular relationship with ResourceLoadScheduler - you create a ResourceLoader, it registers with ResourceLoadScheduler, and then the scheduler tells the loader to start. And other times, you just start a load and register it with the scheduler after the fact . I didn't expect ResourceLoader to know about scheduling, but perhaps I didn't think about it long enough. Can this be streamlined?

Removing knowledge of the initial scheduling from ResourceLoader would simplify things and make it easier to keep ResourceLoadScheduler from having to know about how priority is calculated, so I think that part makes sense.  However, I don't see a good way to keep ResourceLoader from knowing about the end-of-life accounting (unless we come up with a better time to call ResourceLoadScheduler->remove() than the ResourceLoader destructor).

Does it seem reasonable to allow ResourceLoader to know about the cross-origin redirect and remove cases, but not know about how the initial scheduling occurs?

> 
> +    if (!redirectResponse.isNull())
> +        resourceLoadScheduler()->changeHost(this, request);
> 
> Since changeHost is only called as a response to a redirect, it should probably have "redirect" in its name somehow. I've been puzzled about what this function did until I saw where it was being called.
> 
> I'm wondering how changing a host affects scheduling. Clearly, we don't get a chance to re-prioritize the request in WebCore, networking library does everything internally.
> 
> It may be worth making an experiment - create multiple long-standing connections to host 1, then make a request to host 2 that gets redirected to host 1. Will it load immediately in Safari, Chrome or other browsers, even though the concurrent connection limit is already met? This is probably not going to affect this patch, but is something that's nice to check while we're at it.
> 

Agreed on renaming changeHost().

Currently, I don't think we're handling this case correctly (namely, if we have a cross-origin redirect, we continue to account for it under the original host, not the one that it ought to be under for the redirect).  This patch, on the other hand, needs to move the request between hosts because we no longer have a host subclass directly receiving callbacks.

I'll see if I can get the experiment for you.

> 
> +ResourceLoadScheduler::HostInformation* ResourceLoadScheduler::hostForURL(const KURL& url, bool createIfNotFound)
> +{
> +    if (!url.protocolInHTTPFamily())
> +        return m_nonHTTPProtocolHost.get();
> 
> Should these all be the same (ftp: and data: have little in common)? Is that how existing code works?

It's how the existing code works.  I have no strong opinion on how it should work.

> 
> +        * http/tests/xmlhttprequest/logout.html: It is no longer safe
> +        to assume that an async xhr will have any effects synchronously.
> 
> Did this test pass in Firefox and IE in its original form? If it did, we should probably find a way to keep it working.

The data I provided before was for the original form.  I can get it for the new form of the test if you like, but I was mostly pointing out that behavior on this test isn't standard as is.

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