[webkit-reviews] review granted: [Bug 130384] Micro-optimize element descendant iterator. : [Attachment 227028] Even better patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 130384: Micro-optimize element descendant iterator.
https://bugs.webkit.org/show_bug.cgi?id=130384

Attachment 227028: Even better patch
https://bugs.webkit.org/attachment.cgi?id=227028&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=227028&action=review


> Source/WebCore/dom/ElementDescendantIterator.h:51
> +    Vector<Element*, 16, UnsafeVectorOverflow> m_stack;

Please call these something more informative like m_ancestorSiblingStack

> Source/WebCore/dom/ElementDescendantIterator.h:192
> +    Element* firstChild = Traversal<Element>::firstChild(m_current);
> +    Element* nextSibling = Traversal<Element>::nextSibling(m_current);

Please just use ElementTraversal directly.

> Source/WebCore/dom/ElementDescendantIterator.h:196
> +    if (firstChild) {
> +	   if (nextSibling)
> +	       m_stack.append(nextSibling);

Clever!

> Source/WebCore/dom/SelectorQuery.cpp:267
> -    for (auto& element :
descendantsOfType<Element>(const_cast<ContainerNode&>(rootNode))) {
> +    for (auto& element :
elementDescendants(const_cast<ContainerNode&>(rootNode))) {

This is bit confusing. It is ok as intermediate step but we should really add
all the missing functionality here and start returning
ElementDescendantIterator from descendantsOfType<Element>. The typed iterator
can then be implemented in terms of ElementDescendantIterator.