Summary: | TreeWalker.previousSibling() / nextSibling() does not behave according to the specification | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | DOM | Assignee: | 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
Chris Dumez
2015-09-22 23:37:27 PDT
Created attachment 261826 [details]
Patch
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 <> Created attachment 261833 [details]
Patch
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 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 on attachment 261833 [details] Patch Clearing flags on attachment: 261833 Committed r190187: <http://trac.webkit.org/changeset/190187> All reviewed patches have been landed. Closing bug. |