Currently the libsoup backend returns the response body all at once at the end. This fixes it to return it incrementally.
Created attachment 20434 [details] return data to the loader incrementally
It's a pretty good patch, thanks Dan. In gotHeaders Maybe there might be a problem on didReceiveResponse, this will tell webcore about a new response of the same job for each redirect. Same thing in gotChunk, this will give webcore further data with didReceiveData on redirects. I think a SOUP_STATUS_IS_REDIRECTION check must be added to both.
Created attachment 20443 [details] incremental patch Check for redirection.
Created attachment 20444 [details] 02-limit connections Using 20 and 2 seems a good thing for my slow connection, also seems more responsive.
Created attachment 20446 [details] revised patch checking SOUP_STATUS_IS_REDIRECTION() isn't quite right though, because in the cases where it *doesn't* redirect on a 3xx response, we'd end up with no body at the end... Error messages should generally be short anyway, so I've just changed it so that it provides the data incrementally for 2xx responses, and all at once at the end still for non-2xx responses.
Hi, well i think the previous one was still better. This way you will lose many pages from being rendered, such as 404 pages, server errors, ecc.. I'd suggest to check only for a "Location" in the header and for transport error, what do you think about?
Created attachment 20449 [details] 01-check for redirects Is right to use "Location"? It seems libsoup is case-sensitive for headers.
(In reply to comment #6) > Hi, well i think the previous one was still better. This way you will lose many > pages from being rendered, such as 404 pages, server errors, ecc.. It doesn't lose them; 2xx responses are returned incrementally, but non-2xx responses are returned all at once from finishedCallback() just like they were before this patch. So a 301 redirecting to a 200 would look like: gotHeadersCallback [msg->status_code == 301] return gotChunkCallback return restartedCallback gotHeadersCallback [msg->status_code == 200] didReceiveResponse() gotChunkCallback didReceiveData() [more gotChunkCallbacks...] finishedCallback didFinishLoading() but if libsoup didn't follow the redirect (eg, because it was in response to a PUT, or was to an ftp: URL, etc), then instead of "restarted", you'd get "finishedCallback", which would see that the final status was non-2xx, and so it would call didReceiveResponse(), didReceiveData(), and didFinishLoading() all at once to make up for the fact that it hadn't done that before.
Re: 02-limit connections: maybe that should be a separate bug? (Or else we should change the summary here to "fix loading speed problems in libsoup backend".) It definitely looks like Firefox uses 6 for its equivalent of SOUP_SESSION_MAX_CONNS_PER_HOST.
(In reply to comment #8) > It doesn't lose them; 2xx responses are returned incrementally, but non-2xx > responses are returned all at once from finishedCallback() just like they were > before this patch. Yes I'm sorry, I definitively missed the finishedCallback part. But why shouldn't 404, not found, and other errors be loaded incrementally too? Do you think there's a speedup the soup-side?
(In reply to comment #10) > But why > shouldn't 404, not found, and other errors be loaded incrementally too? Incremental loading is mostly important for images and large web pages. Most 404s are tiny, and users would never be able to notice the difference between loading them incrementally and not. And anyway, trying to exhaustively distinguish between status codes that might cause libsoup to requeue the message, and status codes that definitely won't seems like a bad idea.
(In reply to comment #11) > Incremental loading is mostly important for images and large web pages. Most > 404s are tiny, and users would never be able to notice the difference between > loading them incrementally and not. And anyway, trying to exhaustively > distinguish between status codes that might cause libsoup to requeue the > message, and status codes that definitely won't seems like a bad idea. > For instance you mean it's not safe to check for "Location"?
Yeah, just checking 3xx and Location header isn't sufficient. At the moment, the rule is that if there's a Location header, AND it contains a valid http: or https: URL, AND either (a) the status is 302 or 303, or (b) the status is 301 or 307 AND the method is GET, HEAD, OPTIONS, or PROPFIND, then the message gets automatically redirected. Once we deal with authentication, then that will add another set of cases in which libsoup requeues the message automatically (when there's a 401 or 407, and it already has a valid password cached).
Comment on attachment 20449 [details] 01-check for redirects Ouch, you're patch is better then.
Comment on attachment 20444 [details] 02-limit connections Open another bug.
Comment on attachment 20446 [details] revised patch r=me Code is fine. (ChangeLog entry should use eight spaces though for next time) Thanks
Landed in r31852.
Hm... I realized this weekend while trying to figure out the APIs that I had confused ResourceLoader::didReceiveData(const char*, int, long long lengthReceived, bool allAtOnce) and ResourceHandleClient::didReceiveData(ResourceHandle*, const char*, int, int lengthReceived) here: + client->didReceiveData(handle, chunk->data, chunk->length, false); ie, the last argument should actually be an int, not a bool. I couldn't figure out what that argument is actually supposed to mean, since AFAICT it currently ends up going unused in WebKitGtk. The old code passed "0" there but I don't know if that's right or wrong.
Dan, was the change that was landed incorrect? If so, I think a new bug should be used to track that. It's not clear to me why this was reopened.
Comment on attachment 20446 [details] revised patch Clearing review flag as this was landed already.
(In reply to comment #19) > Dan, was the change that was landed incorrect? If so, I think a new bug should > be used to track that. It's not clear to me why this was reopened. ok, i'll reclose it. (the patch is a teeny bit incorrect; it uses "false" where it should have used "0")