[webkit-reviews] review granted: [Bug 105324] Use ElementTraversal in LiveNodeListBase : [Attachment 180186] patch3

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 105324: Use ElementTraversal in LiveNodeListBase
https://bugs.webkit.org/show_bug.cgi?id=105324

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180186&action=review


> Source/WebCore/dom/LiveNodeList.cpp:57
> +    Node* rootNode = this->rootNode();

Do we need "this->"?

> Source/WebCore/html/HTMLCollection.cpp:204
> +template <> inline bool isMatchingElement(const HTMLCollection*
htmlCollection, Element* element)

Nice! It might make sense to specialize this function further for common html
collections like element.children & document.all (of course in a follow up
patch).

> Source/WebCore/html/HTMLCollection.cpp:277
> +template <> inline bool isMatchingElement(const HTMLTagNodeList* nodeList,
Element* element)
> +{
> +    return nodeList->nodeMatchesInlined(element);
> +}

Would it make sense to define this specialization in TagNodeList.h instead? (No
need to do this in this patch).

> Source/WebCore/html/HTMLCollection.cpp:327
>      if (isNodeList(type()) && shouldOnlyIncludeDirectChildren()) //
ChildNodeList

Can we change this to type() == ChildNodeList? If not, I'll do that.

> Source/WebCore/html/HTMLCollection.cpp:371
> +    while (currentOffset != offset && (currentElement =
nextMatchingElement(nodeList, currentElement, root)))

Can we assert that ASSERT(currentOffset < offset); at the beginning?

> Source/WebCore/html/HTMLCollection.cpp:380
> +    ASSERT(!overridesItemAfter());

I don't think this assertion is useful given that we're already asserting
type() is ChildNodeListType since only HTMLCollection overrides itemAfter.

> Source/WebCore/html/HTMLCollection.cpp:381
> +    while (currentOffset != offset && (currentNode =
currentNode->nextSibling()))

Ditto about asserting currentOffset < offset.

> Source/WebCore/html/HTMLCollection.cpp:528
> +    if (type() == ChildNodeListType) {
> +	   currentItem = traverseChildNodeListForwardToOffset(offset,
currentItem, currentOffset);
> +    } else if (isNodeList(type()))

Nit: We don't need curly brackets here.

> Source/WebCore/html/HTMLCollection.cpp:579
> +    if (overridesItemAfter())

We can add UNLIKELY here as well although I don't think it'll matter.

> Source/WebCore/html/HTMLCollection.cpp:583
> +    if (shouldOnlyIncludeDirectChildren())
> +	   return firstMatchingChildElement(static_cast<const
HTMLCollection*>(this), root);

It'll be nice if we could merge this code with childNodes somehow.

> Source/WebCore/html/HTMLCollection.cpp:601
> +    if (overridesItemAfter()) {

UNLIKELY?