Created attachment 145792 [details] [review]
soup_session_get_address_for_uri

This patch includes the function soup_session_get_address_for_uri, which implements the functionality explained in bug description.

Comment on attachment 145792 [details] [review]
soup_session_get_address_for_uri

>+	SoupSessionHost *host = g_hash_table_lookup (priv->hosts, uri);

you need to hold priv->host_lock here, or else it's possible that priv->hosts will get modified by another thread while you're accessing it.

overall, the method seems a little bit... odd. It seems like it would be cleaner to have "soup_session_preresolve_address()" or something. Except that we'd need to have sync and async versions... I dunno.

OK, so, my problem with get_address_for_uri() is that really, it has no use *other* than DNS pre-resolving, so it's a little weird to have a "get address" API rather than the "prefetch DNS" API.

Also, if the user has an http proxy, then there's no point in resolving the host address ahead of time (since we're just going to pass it as a string to the proxy), but it *would* be useful to resolve the proxy address ahead of time (especially if they have PAC/WPAD and so the proxy might be different per-request-URI).

So what we should have is soup_session_prepare_for_uri() (alternate name suggestions welcome), which tells SoupSession that you're probably about to give it a request for that URI, and so it should... prepare for it. Initially this can just be hostname pre-resolving, but later we can add proxy pre-resolving too. (This would require reorganizing SoupSession some though.) It should call soup_address_resolve_async() using the session's async_context, and just document the fact that if you're using SoupSessionSync then either you need to set a useful async_context, or else you just shouldn't call this API. (It doesn't make any sense to have a blocking version of the call, since the whole point of the call is to avoid blocking later.)

One additional nit with the previous patch: it needs to actually create a SoupSessionHost for the hostname if one doesn't already exist, because otherwise it would only have an effect if you had already queued a message to that host, at which point you're probably already resolving the hostname anyway. So anyway, this probably means changing get_host_for_message() to just be a wrapper around a new get_host_for_uri(), and then using get_host_for_uri() from _prepare_for_uri() too.

Also, one more minor problem; if you call soup_address_resolve_async() twice on a given SoupAddress, then it will actually resolve the hostname twice. So that means if you start pre-resolving the address, and then actually queue the message for it before the address resolves, then it will actually send out a second DNS request and wait for *that one* to come back before it sends the message. (Though admittedly, the second request will probably come back faster than the first one did anyway, because the intermediate DNS servers should have cached the answer by that point.) With a little bit of rewriting, you could make SoupAddress notice when it was being called a second time when there was already one GResolver call pending, and it could just keep a list of callbacks to call when that completed.

Created attachment 146046 [details] [review]
soup-session-prepare-for-uri

This patch solves most of the problems that were described in comment 3.

This patch defines soup_session_prepare_for_uri. Both SoupSessionAsync and SoupSessionSync have their own definition of prepare_for_uri; however, the one from SoupSessionSync do nothing.

soup_session_get_address_for_uri is now defined in soup-session-private.h, so only SoupSessionSync and SoupSessionAsync can use that (SoupSessionAsync uses it to obtain the address to prefetch). This function does now create a SoupSessionHost if it doesn't exist.

Also, SoupSessionAddress was modified so now if soup_session_address_resolve_async is called more than once while the resolution is being done it only does the resolution once and then all the callbacks are being called.

The only problem is that soup_session_prepare_for_uri does not yet handle proxies, but i think that it can be added later.

Created attachment 147183 [details] [review]
[SoupAddress] Remove some cruft

Created attachment 147184 [details] [review]
[SoupAddress] make _resolve_async idempotent, document/fix thread-safety

Document that _resolve_async() can be called multiple times, but only
from the same async_context, and _resolve_sync() can be called
multiple times from different threads. Note that _get_name, etc, may
misbehave in the presence of multiple threads.

Change _resolve_async() so that multiple attempts to resolve the same
address will result in only a single GResolver call, and they will all
complete at the same time.

Created attachment 147185 [details] [review]
Add soup_session_prepare_for_uri

Lets the session prepare for a possible upcoming request. (Eg,
WebKitGTK will call this when the user hovers over a link.) Currently
just does DNS resolution. Will eventually do proxy resolution as well.

Based on a patch from José Millán Soto

does this work? i think it's ready to go

Created attachment 148129 [details] [review]
New version of soup_session_prepare_for_uri, this times it checks whether there is a proxy or not

Patch based on the previous one sent by Dan.
This one is basically the same idea of the one Dan sent, but if checks is there is a proxy configured, and if so, it does nothing (to prevent prefetching a host which may not be used)

I think that this is ok enought for going as a first version of the function.

Attachment 147185 [details] pushed as f8ce30a - Add soup_session_prepare_for_uri

Just checking whether or not a proxyresolver is present isn't enough,
since that doesn't necessarily mean a given host will use a proxy. So I
committed my original patch for now.

(Note that this is committed to master, not the gnome-2-28 branch.)

> Just checking whether or not a proxyresolver is present isn't enough,
> since that doesn't necessarily mean a given host will use a proxy. So I
> committed my original patch for now.

My idea was not to check whether a host will use a proxy, it was to ensure not to resolv a DNS address if there is the posibility of the address using proxy.

Right, but in epiphany, there will *always* be a SoupProxyResolver attached to the session, whether or not the user has a proxy configured. (Because adding a proxyresolver to the session is the only way to even find out if a proxy is configured.) So your patch would have made epiphany never pre-resolve hostnames. Whereas without that patch, we do some gratuitous extra DNS resolution sometimes, but that's not too big a deal.