[webkit-reviews] review granted: [Bug 20690] Support for DNS prefetch : [Attachment 23534] Updated patch with comments from Mark Rowe

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has granted 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 23534: Updated patch with comments from Mark Rowe
https://bugs.webkit.org/attachment.cgi?id=23534&action=edit

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
 281		 if (value.startsWith("http:") || value.startsWith("https:") ||
value.startsWith("//")) {
 282		     prefetchDNS(document()->completeURL(value).host());
 283		 }

You have an unnecessary set of braces around this if statement.

There's no need for a separate DNSMac.cpp and DNSCFNet.cpp -- both Mac and
Windows will probably use CFHost for prefetching.

The header guard on DNS.h is named DNSPrefetch_h, which doesn't match the file
name.

You'll also need to stub out prefetchDNS for other platforms in order to not
break Qt, Gtk and wx builds.  Adding to their TemporaryLinkStubs.cpp file is
probably the right approach for those ports.

Unless I'm not understanding the test, it doesn't appear to actually test
whether DNS prefetching has worked nor whether parsing the relevant headers and
attributes were parsed, only that it does not crash while parsing.  The output
from DRT could be a little clearer about this as it currently says "Browsers
MAY do a DNS prefetch on the following links:" then doesn't list any links. 
There are also coding style issues in the JavaScript, such as braces around
one-line if statements.

r=me if you address these issues.