[webkit-reviews] review denied: [Bug 110352] Use DOM ordering for list counts : [Attachment 191972] Patch

[bugzilla-daemon at webkit.org]

Elliott Sprehn <esprehn at chromium.org> has denied Andrei Bucur
<abucur at adobe.com>'s request for review:
Bug 110352: Use DOM ordering for list counts
https://bugs.webkit.org/show_bug.cgi?id=110352

Attachment 191972: Patch
https://bugs.webkit.org/attachment.cgi?id=191972&action=review

------- Additional Comments from Elliott Sprehn <esprehn at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191972&action=review


This should use Element* pretty much everywhere. There's no reason to be
skipping the Text nodes manually everywhere. :)

> Source/WebCore/dom/NodeTraversal.cpp:35
> +Node* nextSiblingIncludingPseudo(const Node* current)

This is more complicated than it needs to be. Just add
Node::pseudoAwareNextSibling() and Node::pseudoAwarePreviousSibling(). In fact
these methods used to exist and then I removed them when I didnt' need them.
Just add them back.
https://trac.webkit.org/changeset/136744/trunk/Source/WebCore/dom/Node.cpp

> Source/WebCore/dom/NodeTraversal.cpp:52
> +Node* previousSiblingIncludingPseudo(const Node* current)

All these methods should be in ElementTraversal and should return Element*.

> Source/WebCore/dom/NodeTraversal.cpp:69
> +Node* firstChildIncludingPseudo(const Node* current)

These methods would be much clearer if they were split by type.

if (current->isElement()) { ... return ...; } ... return;

> Source/WebCore/dom/NodeTraversal.h:82
> +// Pre-order traversal including the pseudo-elements.

ElementTraversal and return Element*. I don't know why you need to see Text
nodes in any of these traversals.

> Source/WebCore/rendering/RenderCounter.cpp:75
> +	   previous = NodeTraversal::previousSiblingIncludingPseudo(previous);

ElementTraversal removes the need for all this.

> Source/WebCore/rendering/RenderListItem.cpp:105
> +    for (Node* parent = listItemNode->parentNode(); parent; parent =
parent->parentNode()) {

parentElement(), this should all be element related.