Bug 149493

Summary: TreeWalker.previousSibling() / nextSibling() does not behave according to the specification
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, darin, esprehn+autocc, kangil.han, koivisto, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2015-09-22 23:37:27 PDT
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.
Comment 1 Chris Dumez 2015-09-23 09:54:42 PDT
Created attachment 261826 [details]
Patch
Comment 2 Darin Adler 2015-09-23 10:35:54 PDT
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 <>
Comment 3 Chris Dumez 2015-09-23 11:38:55 PDT
Created attachment 261833 [details]
Patch
Comment 4 Darin Adler 2015-09-23 13:23:30 PDT
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.
Comment 5 Chris Dumez 2015-09-23 13:50:29 PDT
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.
Comment 6 WebKit Commit Bot 2015-09-23 14:41:10 PDT
Comment on attachment 261833 [details]
Patch

Clearing flags on attachment: 261833

Committed r190187: <http://trac.webkit.org/changeset/190187>
Comment 7 WebKit Commit Bot 2015-09-23 14:41:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2015-09-28 11:38:56 PDT
<rdar://problem/22882468>