[webkit-reviews] review denied: [Bug 41630] [Soup] DNS prefetching spams resolver, shoots self in the foot : [Attachment 132029] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 41630: [Soup] DNS prefetching spams resolver, shoots self in the foot
https://bugs.webkit.org/show_bug.cgi?id=41630

Attachment 132029: Patch
https://bugs.webkit.org/attachment.cgi?id=132029&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132029&action=review


It's great that you're trying to share this code. Hook up with some people on
the Windows and Mac ports to see how you can add the appropriate files to the
build (see below).

> Source/WebCore/platform/network/DNS.h:38
> +class DNSResolveQueue : public TimerBase {

Ideally DNSResolveQueue should have it's own header and source file. Perhaps
ping someone from the Mac port about adding the new files to the Xcode project.


> Source/WebCore/platform/network/DNS.h:65
> +    void add(const String& hostname)
> +    {
> +	 // If there are no names queued, and few enough are in flight, resolve
immediately (the mouse may be over a link).
> +	 if (!m_names.size()) {
> +	   if (proxyIsEnabledInSystemPreferences())
> +	     return;

A method of this size should be in source file instead of a header.

> Source/WebCore/platform/network/DNS.h:90
> +    virtual bool proxyIsEnabledInSystemPreferences() = 0;
> +    virtual void resolve(const String&) = 0;

You actually don't need to make this an abstract class. While that's useful to
do if need to instantiate different types of DNSResolveQueues at runtime, that
isn't the case. There is only ever one networking backend at a time in WebKit.
Instead what you can do here is to use the pattern that GraphicsContext and the
like do where there's a 

bool platformProxyIsEnabledInSystemPreferences();
void platformResolve(const String& address);

And those implementations are in the platform-specific directory in a different
file. I think this is the preferred method in WebCore.

> Source/WebCore/platform/network/DNS.h:94
> +    void fired()
> +    {
> +	 if (proxyIsEnabledInSystemPreferences()) {

Ditto.

> Source/WebCore/platform/network/cf/DNSCFNet.cpp:55
> +const int DNSResolveQueue::namesToResolveImmediately = 4;
> +const double DNSResolveQueue::coalesceDelayInSeconds = 1.0;
> +// CFHost doesn't currently throttle for us, see <rdar://8105550>.
> +const int DNSResolveQueue::maxSimultaneousRequests = 8;
> +const int DNSResolveQueue::maxRequestsToQueue = 64;
> +const double DNSResolveQueue::retryResolvingInSeconds = 0.1;

These don't differ between ports, so I prefer to keep them as static variables
here. There are all kinds of tricky issues with static members that you
probably don't even want to get near if you don't have to. 
http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.14