[webkit-reviews] review requested: [Bug 20690] Support for DNS prefetch : [Attachment 23445] Updated patch with comments from Antti

[bugzilla-daemon at webkit.org]

Collin Jackson <collinj at webkit.org> has asked  for review:
Bug 20690: Support for DNS prefetch
https://bugs.webkit.org/show_bug.cgi?id=20690

Attachment 23445: Updated patch with comments from Antti
https://bugs.webkit.org/attachment.cgi?id=23445&action=edit

------- Additional Comments from Collin Jackson <collinj at webkit.org>
(In reply to comment #2)
> Could this be tested with HTTP layout test (at least for code coverage)?

Good idea. I've added one. 

> +	   If ENABLE(DNS_PREFETCH) is true, notify the FrameLoaderClient 
> +	   about hyperlinks and <link rel="dns-prefetch"> tags that are
> +	   encountered so that it can speculatively resolve hostnames.
> 
> I would prefer not to have a feature define for this. Having it enabled on
> platforms that don't support it looks harmless and avoiding configuration
> proliferation is a good thing.

Ok. I've removed the feature define.

> +#if ENABLE(DNS_PREFETCH)
> +#include "FrameLoaderClient.h"
> +#endif
> 
> No need to make this conditional.

Ok. This include isn't used in Document.cpp any more, so I just removed it.

>  void Document::updateFocusAppearanceSoon()
> @@ -4188,6 +4202,18 @@ unsigned Document::visitedLinkHash(const
> 
>      bool hasColonSlashSlash = containsColonSlashSlash(characters, length);
> 
> +#if ENABLE(DNS_PREFETCH)
> +    String prefetchHost;
> +    if (hasColonSlashSlash) {
> +	   KURL parsedURL(attributeURL, m_baseURL);
> +	   prefetchHost = parsedURL.host();
> +    } else
> +	   prefetchHost = m_baseURL.host();
> +
> +    if (frame() && (m_isDNSPrefetchEnabled || prefetchHost ==
> m_securityOrigin->host()))
> +	   frame()->loader()->client()->prefetchDNS(prefetchHost);
> +#endif
> 
> This function is for calculating a hash and this kind of code is
inappropriate
> here. Please find the right place to put this code. Would simply putting it
to
> HTMLAnchorElement::parseMappedAttribute() work?

Ok, I moved it.

> +void Document::initDNSPrefetchEnabled()
> +{
> 
> Maybe just call it initDNSPrefetch()?

Ok. I renamed it to initDNSPrefetch().

> +    m_haveExplicitlyDisabledDNSPrefetch = false;
> +    m_isDNSPrefetchEnabled = (securityOrigin()->protocol() == "http");
> 
> No need for parenthesis here.

Fixed.

> +    // Inherit DNS prefetch opt-out from parent frame    
> +    if (Document* parent = parentDocument())
> +	   if (!parent->isDNSPrefetchEnabled())
> +	       m_isDNSPrefetchEnabled = false;
> 
> Outer if needs curly brackets according to the coding style.

Fixed.

> +void Document::setDNSPrefetchControl(const String& dnsPrefetchControl)
> +{
> 
> This is not really a setter, how about parseDNSPrefetchControlHeader() or
> something?

Ok. I've renamed it to parseDNSPrefetchControlHeader().

> +    if (equalIgnoringCase(dnsPrefetchControl, "on") && 
> +	   !m_haveExplicitlyDisabledDNSPrefetch) {
> 
> can be single line

Fixed.

> +	   m_isDNSPrefetchEnabled = true;
> +	   return;
> +    }
> +
> +    m_isDNSPrefetchEnabled = false;
> +    m_haveExplicitlyDisabledDNSPrefetch = true;
> +}
> +#endif
> +
> 
> +    void setDNSPrefetchControl(const WebCore::String&);
> 
> No need for WebCore:: prefix.

Fixed.

> @@ -111,6 +111,7 @@ protected:
>      bool m_alternate;
>      bool m_isStyleSheet;
>      bool m_isIcon;
> +    bool m_isDnsPrefetch;
>      bool m_createdByParser;
>  };
> 
> m_isDNSPrefetch to match coding style.

Fixed.