Bug 149493 – TreeWalker.previousSibling() / nextSibling() does not behave according to the specification

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

TreeWalker.previousSibling()  / nextSibling() does not behave according to the specification:
- https://dom.spec.whatwg.org/#dom-treewalker-nextsibling
- https://dom.spec.whatwg.org/#dom-treewalker-previoussibling
- https://dom.spec.whatwg.org/#concept-traverse-siblings

This is causing several checks in imported/w3c/web-platform-tests/dom/traversal/TreeWalker.html to fail. Firefox passes all the checks.

[Chris Dumez]

Created attachment 261826 [details]
Patch

[Darin Adler]

Comment on attachment 261826 [details]
Patch

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

> Source/WebCore/dom/TreeWalker.cpp:134
> +template <TreeWalker::SiblingTraversalType type>
> +Node* TreeWalker::traverseSiblings()

I would write this:

template<TreeWalker::SiblingTraversalType type> Node* TreeWalker::traverseSiblings()

The desire to have it all on one line is probably just my personal quirk. I do like it better, though.

The lack of space before the <> is more important to me and while I don’t know about everyone on the WebKit project I know that at least Anders strongly prefers it too.

> Source/WebCore/dom/TreeWalker.cpp:145
> +                m_current = sibling.release();

In modern code this should be WTF::move(sibling) rather than sibling.release().

> Source/WebCore/dom/TreeWalker.h:59
> +        template <SiblingTraversalType> Node* traverseSiblings();

template<> with no space rather than template <>

[Chris Dumez]

Created attachment 261833 [details]
Patch

[Darin Adler]

Comment on attachment 261833 [details]
Patch

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

> Source/WebCore/dom/TreeWalker.cpp:150
> +            sibling = isNext ? sibling->firstChild() : sibling->lastChild();
> +            if (acceptNodeResult == NodeFilter::FILTER_REJECT || !sibling)
> +                sibling = isNext ? node->nextSibling() : node->previousSibling();

I just noticed something redundant here. If acceptNodeResult is FILTER_REJECT, then there’s no reason to execute that first line of code setting sibling, because it gets overwritten by the second line of code. Maybe this is better?

    if (acceptNodeResult == NodeFilter::FILTER_REJECT)
        sibling = nullptr;
    else
        sibling = isNext ? sibling->firstChild() : sibling->lastChild();
    if (!sibling)
        sibling = isNext ? node->nextSibling() : node->previousSibling();

Not really sure what’s best for efficiency and what’s best for clarity. We can definitely land it your way, but I just noticed this strangeness.

[Chris Dumez]

Comment on attachment 261833 [details]
Patch

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

>> Source/WebCore/dom/TreeWalker.cpp:150
>> +                sibling = isNext ? node->nextSibling() : node->previousSibling();
> 
> I just noticed something redundant here. If acceptNodeResult is FILTER_REJECT, then there’s no reason to execute that first line of code setting sibling, because it gets overwritten by the second line of code. Maybe this is better?
> 
>     if (acceptNodeResult == NodeFilter::FILTER_REJECT)
>         sibling = nullptr;
>     else
>         sibling = isNext ? sibling->firstChild() : sibling->lastChild();
>     if (!sibling)
>         sibling = isNext ? node->nextSibling() : node->previousSibling();
> 
> Not really sure what’s best for efficiency and what’s best for clarity. We can definitely land it your way, but I just noticed this strangeness.

You are right that the first sibling assignment is useless if acceptNodeResult == NodeFilter::FILTER_REJECT, however, it is should be cheap. The current code also has the benefit of being a bit more concise and matching the text of the DOM spec.
I have a slight preference for keeping it this way.

[WebKit Commit Bot]

Comment on attachment 261833 [details]
Patch

Clearing flags on attachment: 261833

Committed r190187: <http://trac.webkit.org/changeset/190187>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/22882468>