[webkit-reviews] review denied: [Bug 23846] [GTK]Enable DNS prefetching : [Attachment 41725] DNS prefetch patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied José Millán Soto
<jmillan at igalia.com>'s request for review:
Bug 23846: [GTK]Enable DNS prefetching
https://bugs.webkit.org/show_bug.cgi?id=23846

Attachment 41725: DNS prefetch patch
https://bugs.webkit.org/attachment.cgi?id=41725&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +static bool checkPrefetched(const String& hostname)
> +{
> +    static HashMap<String, double> lastTimePrefetched;
> +    const double ttl = 60;

I would make this a #define. Any reason why you prefer to use a const variable?


> +    /*
> +    soup_session_prepare_for_uri not yet avaliable:
> +	   https://bugzilla.gnome.org/show_bug.cgi?id=598948
> +
> +    String uri = "http://"+hostname;
> +    SoupURI* soupUri = soup_uri_new(uri.utf8().data());
> +    soup_session_prepare_for_uri(ResourceHandle::defaultSession(), soupUri);

> +    soup_uri_free(soupUri);
> +    */

We don't usually keep dead code. I would prefer just mentioning the bug report
here.

> +    // This may work while soup_session_prepare_for_uri is not avaliable
> +    SoupAddress* address = soup_address_new(hostname.utf8().data(),
> +	   SOUP_ADDRESS_ANY_PORT);

No need to break this line, it is not too long by WebKit standards.

> +    soup_address_resolve_async(address, 0, 0, 0, 0);
> +    g_object_unref(address);

According to my understanding of what was said in #webkit-gtk by danw recently,
on newer libsoup versions this would have no effect. Here's the relevant
discussion:

Oct 22 14:11:12 <danw>	xan: pre-GResolver, there was a one hour cache
underneath SoupAddress, so if you did soup_address_new("google.com"), then that
might al
ready be resolved. Post-GResolver, that caching layer is gone. However,
SoupSession still caches *specific SoupAddresses*. So if you do your own
soup_address_n
ew("google.com"), then you're going to have to go to the network to resolve it.
But if you queue a request for "google.com", SoupSessino probably
Oct 22 14:11:12 <danw>	 already has an already-resolved SoupAddress to use

We may want to have this for older soup versions, but then again, it may be
better to queue a NO-OP request? r- for now, for the above. Thanks for working
on this.