[webkit-reviews] review denied: [Bug 20690] Support for DNS prefetch : [Attachment 23220] Proposed patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Collin Jackson
<collinj at webkit.org>'s request for review:
Bug 20690: Support for DNS prefetch
https://bugs.webkit.org/show_bug.cgi?id=20690

Attachment 23220: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=23220&action=edit

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
Looks generally good except for the code changes to
Document::visitedLinkHash(). That code needs better place though so r-.

Could this be tested with HTTP layout test (at least for code coverage)?

+	 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.
 
+#if ENABLE(DNS_PREFETCH)
+#include "FrameLoaderClient.h"
+#endif

No need to make this conditional.

 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?

+void Document::initDNSPrefetchEnabled()
+{

Maybe just call it initDNSPrefetch()?

+    m_haveExplicitlyDisabledDNSPrefetch = false;
+    m_isDNSPrefetchEnabled = (securityOrigin()->protocol() == "http");

No need for parenthesis here.

+    // 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.

+void Document::setDNSPrefetchControl(const String& dnsPrefetchControl)
+{

This is not really a setter, how about parseDNSPrefetchControlHeader() or
something?

+    if (equalIgnoringCase(dnsPrefetchControl, "on") && 
+	 !m_haveExplicitlyDisabledDNSPrefetch) {

can be single line

+	 m_isDNSPrefetchEnabled = true;
+	 return;
+    }
+
+    m_isDNSPrefetchEnabled = false;
+    m_haveExplicitlyDisabledDNSPrefetch = true;
+}
+#endif
+

+    void setDNSPrefetchControl(const WebCore::String&);

No need for WebCore:: prefix.

@@ -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.