[Webkit-unassigned] [Bug 105324] Use ElementTraversal in LiveNodeListBase

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=105324


Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #180186|review?                     |review+
               Flag|                            |




--- Comment #10 from Ryosuke Niwa <rniwa at webkit.org>  2012-12-19 14:01:06 PST ---
(From update of attachment 180186)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.