[webkit-reviews] review granted: [Bug 136825] Rename Node::childNode(index) to traverseToChildAt(index) for clarity : [Attachment 238131] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 136825: Rename Node::childNode(index) to traverseToChildAt(index) for
clarity
https://bugs.webkit.org/show_bug.cgi?id=136825

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.