Bug 132509 – [iOS][WK2] Prefetch DNS hostnames on tap highlight

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS][WK2] Prefetch DNS hostnames on tap highlight

[Benjamin Poulain]

Created attachment 230731 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 230731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230731&action=review

> Source/WebCore/dom/Document.cpp:4893
> +void Document::prefetchDNSFrom(const Node& node)
> +{
> +    if (node.isLink() && isElement(node)) {
> +        const Element& linkTypeElement = toElement(node);
> +
> +        AtomicString urlString;
> +        if (linkTypeElement.hasTagName(SVGNames::aTag))
> +            urlString = linkTypeElement.getAttribute(XLinkNames::hrefAttr);
> +        else
> +            urlString = linkTypeElement.getAttribute(HTMLNames::hrefAttr);
> +        if (!urlString.isEmpty()) {
> +            URL absoluteURL = linkTypeElement.document().completeURL(stripLeadingAndTrailingHTMLSpaces(urlString));
> +            prefetchDNS(absoluteURL.host());
> +        }
> +    }
> +}

It would be nice if we could just make this a member function of Element instead.
And I guess we should share this code with HTMLAnchorElement::parseAttribute.

[Benjamin Poulain]

Created attachment 230790 [details]
Patch

[Benjamin Poulain]

Created attachment 230794 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 230794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230794&action=review

> Source/WebCore/dom/Element.cpp:1168
> +void Element::prefetchDNSFrom(const Element& element)

It is unfortunate that this implementation is so different from what we have in Chrome::mouseDidMoveOverElement(). Can you make it similar? The situation is essentially the same, so having different solutions will be confusing.

> Source/WebCore/dom/Element.h:568
> +    static void prefetchDNSFrom(const Element&);

I don't think that we normally make Nodes or Elements const, so we?

[Alexey Proskuryakov]

Comment on attachment 230794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230794&action=review

> Source/WebCore/dom/Element.cpp:1177
> +    if (element.hasTagName(SVGNames::aTag))
> +        urlString = element.getAttribute(XLinkNames::hrefAttr);
> +    else
> +        urlString = element.getAttribute(HTMLNames::hrefAttr);

It is a layering violation to make core DOM code know about HTML and SVG like here.

[Benjamin Poulain]

(In reply to comment #6)
> (From update of attachment 230794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230794&action=review
> 
> > Source/WebCore/dom/Element.cpp:1177
> > +    if (element.hasTagName(SVGNames::aTag))
> > +        urlString = element.getAttribute(XLinkNames::hrefAttr);
> > +    else
> > +        urlString = element.getAttribute(HTMLNames::hrefAttr);
> 
> It is a layering violation to make core DOM code know about HTML and SVG like here.

Not sure to follow you. Element knows about SVG attributes in detail already.

Do you prefer if this is moved back to document?

[Alexey Proskuryakov]

Prefetch on mouse move code path uses HitTestResult, which already has the url precomputed. So I would prefer this new code to not exist at all, and instead to use the same idiom as the existing solution.

[Benjamin Poulain]

Created attachment 230862 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 230862 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230862&action=review

> Source/WebCore/dom/Element.cpp:1179
> +    if (linkAttribute.isEmpty())
> +        return document().completeURL(stripLeadingAndTrailingHTMLSpaces(linkAttribute));
> +    return URL();

Looks like normal and early return statements are switched here.

> Source/WebCore/platform/network/DNS.h:34
> -void prefetchDNS(const String& hostname);
> +class URL;
> +
> +void prefetchDNS(const URL& absoluteURL);

I think that the original interface was superior, even though we had a bit of duplicated code. The new interface is misleading - this function logically takes a hostname, not a URL.

[Benjamin Poulain]

Created attachment 230869 [details]
Patch

[Benjamin Poulain]

Comment on attachment 230869 [details]
Patch

Clearing flags on attachment: 230869

Committed r168339: <http://trac.webkit.org/changeset/168339>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.