Bug 136825 – Rename Node::childNode(index) to traverseToChildAt(index) for clarity

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Rename Node::childNode(index) to traverseToChildAt(index) to make it clearer that the method is actually traversing the children and thus is potentially expensive.
We should also avoid calling this method whenever there is a more efficient alternative.

[Chris Dumez]

Created attachment 238131 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238131&action=review

I like it!

> Source/WebCore/WebCore.exp.in:-1629
> -__ZNK7WebCore13ContainerNode9childNodeEj

Check UIKit before landing just in case.

> Source/WebCore/dom/ContainerNode.cpp:945
> +    for (; child && index > 0; --index)

I am surprised you had to manually write the descending loop, clang is usually very good at that.

A random thought: if this code is hot enough, you should dump the average value of "index" in a typical use. If it is large enough, it could be worth doing some manual unrolling.

> Source/WebCore/inspector/DOMPatchSupport.cpp:381
> +    for (size_t i = 0; node && i < newMap.size(); ++i, node = node->nextSibling()) {

size_t -> unsigned.

Personally I would use a while() here instead of for() because of the return in the loop and the non-obvious iteration steps.

[Darin Adler]

We normally use “traverse” to refer to tree traversal, not just walking the linear child list.

[Chris Dumez]

(In reply to comment #3)
> We normally use “traverse” to refer to tree traversal, not just walking the linear child list.

Do you have another naming suggestion? I thought iterating over the direct children still counted as a tree traversal, but if there is a better name for it, I am happy to make the update.

[Chris Dumez]

(In reply to comment #3)
> We normally use “traverse” to refer to tree traversal, not just walking the linear child list.

Since you're using the term "walking", do you think walkToChildAt() would be a better alternative?
I don't feel strongly either way ("traversing" a linked list sounded OK to me).

[Chris Dumez]

Comment on attachment 238131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238131&action=review

>> Source/WebCore/dom/ContainerNode.cpp:945
>> +    for (; child && index > 0; --index)
> 
> I am surprised you had to manually write the descending loop, clang is usually very good at that.
> 
> A random thought: if this code is hot enough, you should dump the average value of "index" in a typical use. If it is large enough, it could be worth doing some manual unrolling.

I did not check the generated assembly to see if there was a difference. Since I was updated this function for coding style, I figured I would refactor it a bit. The purpose was not really to optimize.

>> Source/WebCore/inspector/DOMPatchSupport.cpp:381
>> +    for (size_t i = 0; node && i < newMap.size(); ++i, node = node->nextSibling()) {
> 
> size_t -> unsigned.
> 
> Personally I would use a while() here instead of for() because of the return in the loop and the non-obvious iteration steps.

newMap.size() returns a size_t so wouldn't be using unsigned for |i| be risky?

[Chris Dumez]

Comment on attachment 238131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238131&action=review

>> Source/WebCore/WebCore.exp.in:-1629
>> -__ZNK7WebCore13ContainerNode9childNodeEj
> 
> Check UIKit before landing just in case.

I have just checked, UIKit does not use ContainerNode::childNode().

[Chris Dumez]

Another proposal would be findChildAt(index). I still prefer traverseToChildAt(index) though.

[Chris Dumez]

Created attachment 238230 [details]
Patch

[Darin Adler]

Comment on attachment 238230 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238230&action=review

> Source/WebCore/dom/ContainerNode.cpp:945
> +    for (; child && index > 0; --index)

It’d be pretty standard WebKit style to just say "child && index" instead of "child && index > 0".

> Source/WebCore/dom/Position.cpp:249
> -        return m_anchorNode->childNode(m_offset - 1); // -1 converts to childNode((unsigned)-1) and returns null.
> +        return m_offset ? m_anchorNode->traverseToChildAt(m_offset - 1) : nullptr;

Wow, that 0 case could have been really slow before!

> Source/WebCore/inspector/DOMPatchSupport.cpp:381
> +    for (unsigned i = 0; node && i < newMap.size(); ++i, node = node->nextSibling()) {

A little strange to use unsigned here now when it was size_t before.

[WebKit Commit Bot]

Comment on attachment 238230 [details]
Patch

Clearing flags on attachment: 238230

Committed r173684: <http://trac.webkit.org/changeset/173684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.