shouldParseTelephoneNumbersInNode() does not need to check for Comment nodes

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

shouldParseTelephoneNumbersInNode() does not need to check for Comment nodes. We already know the input is a ContainerNode and a Comment is not a ContainerNode.

[Chris Dumez]

Created attachment 258179 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258179 [details]
Patch

Clearing flags on attachment: 258179

Committed r187893: <http://trac.webkit.org/changeset/187893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 258179 [details]
Patch

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

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352
> +    for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) {

Modern way to write this is:

    for (auto& container : lineage(node)) {

[Darin Adler]

Comment on attachment 258179 [details]
Patch

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352
>> +    for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) {
> 
> Modern way to write this is:
> 
>     for (auto& container : lineage(node)) {

Well, not exactly, I think it’s:

    for (auto& container : lineageOfType<Element>(node)) {
    }

The reason I say Element here instead of ContainerNode is that only an element can have isLink set to true, even though the function is in Node. Links are all HTMLAnchorElement, HTMLLinkElement, or SVGAElement. The rest if the checks in disallowTelephoneNumberParsing are all clearly only relevant for Element, not other kinds of ContainerNode.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 258179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258179&action=review
> 
> >> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2352
> >> +    for (const ContainerNode* ancestor = &node; ancestor; ancestor = ancestor->parentNode()) {
> > 
> > Modern way to write this is:
> > 
> >     for (auto& container : lineage(node)) {
> 
> Well, not exactly, I think it’s:
> 
>     for (auto& container : lineageOfType<Element>(node)) {
>     }
> 
> The reason I say Element here instead of ContainerNode is that only an
> element can have isLink set to true, even though the function is in Node.
> Links are all HTMLAnchorElement, HTMLLinkElement, or SVGAElement. The rest
> if the checks in disallowTelephoneNumberParsing are all clearly only
> relevant for Element, not other kinds of ContainerNode.

Funny that you mention it, in an earlier version of my patch I had:
static inline bool shouldParseTelephoneNumbersInNode(const ContainerNode& node)
{
    const Element* element = is<Element>(node) ? &downcast<Element>(node) : node.parentElement();
    if (!element)
        return true;

    for (auto& ancestor : lineageOfType<Element>(*element)) {
        if (disallowTelephoneNumberParsing(ancestor))
            return false;
    }
    return true;
}

However, as you can see it is not as nice as one would think because lineageOfType() expects a non-null element. The code ended being longer and a bit more complicated so I kept the old-style for-loop in this case. However, if you prefer this (or have a better way to write this), I can upload a patch to make this change.

Also note that there is no performance win to traverse Elements only in this case as all the checks in disallowTelephoneNumberParsing() are efficient on Node (i.e. isLink() / hasTagName() are not any faster on an Element than on a Node) so the implicit isElement() checks are redundant.