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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org>  2012-12-18 11:49:46 PST ---
(From update of attachment 179980)
View in context: https://bugs.webkit.org/attachment.cgi?id=179980&action=review

> Source/WebCore/ChangeLog:9
> +        

Leading whitespace.

> Source/WebCore/ChangeLog:11
> +        

Ditto.

> Source/WebCore/ChangeLog:13
> +        

Ditto.

> Source/WebCore/ChangeLog:21
> +        

Ditto for the rest of the change log entry.

> Source/WebCore/ChangeLog:27
> +            Root is always ContainerNode if the list has anything in it. Traversal functions are slightly more

Root is always ContainerNode. period. We only instantiate LiveNodeListBase on Document or Element.

> Source/WebCore/dom/LiveNodeList.cpp:59
> +    if (!rootNode->isContainerNode())
> +        return 0;

I don't think this can ever occur. m_ownerNode is always element or document, and root node can only be m_ownerNode or its ancestor.

> Source/WebCore/dom/TagNodeList.h:45
> -            return adoptRef(new TagNodeList(rootNode, starAtom, localName));
> +            return adoptRef(new TagNodeList(rootNode, TagNodeListType, starAtom, localName));

You can just pass in type here.

> Source/WebCore/html/HTMLCollection.cpp:382
> +    if (type() == HTMLTagNodeListType)
> +        return firstMatchingElement(static_cast<const HTMLTagNodeList*>(this), root);
> +    if (type() == ClassNodeListType)
> +        return firstMatchingElement(static_cast<const ClassNodeList*>(this), root);

So the reason you're cherry-picking these is because they're most frequently used?
Why don't we just special-case ChildNodeListType and then handle the rest in isAcceptableElement instead? There isn't really a point in differentiating NodeList and HTMLCollection here.

> Source/WebCore/html/HTMLCollection.cpp:470
> +    ContainerNode* root = rootContainerNode();
> +    if (!root) {
> +        setLengthCache(0);
> +        return 0;
> +    }

It's really strange that rootContainerNode could ever return 0. Do you recall when that happened?
We probably need to fix that bug first.

> Source/WebCore/html/HTMLCollection.cpp:522
> +        currentItem = traverseForwardToOffset(offset, currentItem, currentOffset, root);
> +    else
> +        currentItem = static_cast<const HTMLCollection*>(this)->traverseForwardToOffset(offset, toElement(currentItem), currentOffset, offsetInArray, root);

Note that we know currentOffset != offset at this point.

> Source/WebCore/html/HTMLCollection.cpp:584
> +        offsetInArray = m_cachedElementsArrayOffset;
> +        while (currentOffset != offset && (currentElement = virtualItemAfter(offsetInArray, currentElement)))
> +            ++currentOffset;

This doesn't seem right virtualItemAfter needs to update m_cachedElementsArrayOffset. You probably need to enable microdata in order to test this change.

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