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.
Created attachment 238131 [details] Patch
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.
We normally use “traverse” to refer to tree traversal, not just walking the linear child list.
(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.
(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).
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?
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().
Another proposal would be findChildAt(index). I still prefer traverseToChildAt(index) though.
Created attachment 238230 [details] Patch
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.
Comment on attachment 238230 [details] Patch Clearing flags on attachment: 238230 Committed r173684: <http://trac.webkit.org/changeset/173684>
All reviewed patches have been landed. Closing bug.