Created attachment 351064 [details] WIP

Created attachment 351066 [details] Cleanup

Comment on attachment 351066 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=351066&action=review > Source/WebCore/editing/markup.cpp:557 > for (Node* n = startNode; n != pastEnd; n = next) { Note I intentionally kept this single letter variable name so that the patch diff looks sane. I can rename it in a separate patch if someone feels strongly about it.

Comment on attachment 351066 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=351066&action=review > Source/WebCore/editing/markup.cpp:525 > + auto enterNode = [&] (Node& node) { ... > Source/WebCore/editing/markup.cpp:542 > + auto exitedNode = [&] (Node& node) { Names enterNode/exitNode or enteredNode/exitedNode would pair better > Source/WebCore/editing/markup.cpp:556 > + Node* next; It is non-obvious that 'next' will always get initialized or is not used. Please null it or refactor code so it is otherwise obvious. > Source/WebCore/editing/markup.cpp:574 > + Vector<Node*, 8> exitedAncestors; > + next = nullptr; > + if (auto* firstChild = n->firstChild()) > + next = firstChild; > + else if (auto* nextSibling = n->nextSibling()) > + next = nextSibling; > + else { > + for (auto* ancestor = n->parentNode(); ancestor; ancestor = ancestor->parentNode()) { > + exitedAncestors.append(ancestor); > + if (auto* nextSibling = ancestor->nextSibling()) { > + next = nextSibling; > + break; > + } > + } > + } Wouldn't any of the existing code we have for basic tree traversal work (NodeTraversal or iterators)? > Source/WebCore/editing/markup.cpp:594 > + for (auto* ancestor : exitedAncestors) { > + if (depth || next != pastEnd) > + exitedNode(*ancestor); > } You should break out of the loop if the test fails. > Source/WebCore/editing/markup.cpp:600 > + if (depth) { > + for (auto* ancestor = (pastEnd ? pastEnd : lastNode)->parentNode(); ancestor && depth; ancestor = ancestor->parentNode()) > + exitedNode(*ancestor); > + } if (depth) could be removed.

(In reply to Antti Koivisto from comment #4) > Comment on attachment 351066 [details] > Cleanup > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351066&action=review > > > Source/WebCore/editing/markup.cpp:525 > > + auto enterNode = [&] (Node& node) { > > ... > > > Source/WebCore/editing/markup.cpp:542 > > + auto exitedNode = [&] (Node& node) { > > Names enterNode/exitNode or enteredNode/exitedNode would pair better Will use enterNode/exitNode since it's a bit odd for a function which determines whether to enter a node or not to be called "enteredNode". > > Source/WebCore/editing/markup.cpp:556 > > + Node* next; > > It is non-obvious that 'next' will always get initialized or is not used. > Please null it or refactor code so it is otherwise obvious. Sure, will initialize. > > Source/WebCore/editing/markup.cpp:574 > > + Vector<Node*, 8> exitedAncestors; > > + next = nullptr; > > + if (auto* firstChild = n->firstChild()) > > + next = firstChild; > > + else if (auto* nextSibling = n->nextSibling()) > > + next = nextSibling; > > + else { > > + for (auto* ancestor = n->parentNode(); ancestor; ancestor = ancestor->parentNode()) { > > + exitedAncestors.append(ancestor); > > + if (auto* nextSibling = ancestor->nextSibling()) { > > + next = nextSibling; > > + break; > > + } > > + } > > + } > > Wouldn't any of the existing code we have for basic tree traversal work > (NodeTraversal or iterators)? I don't think there is any that collect ancestors. > > Source/WebCore/editing/markup.cpp:594 > > + for (auto* ancestor : exitedAncestors) { > > + if (depth || next != pastEnd) > > + exitedNode(*ancestor); > > } > > You should break out of the loop if the test fails. Will do. > > Source/WebCore/editing/markup.cpp:600 > > + if (depth) { > > + for (auto* ancestor = (pastEnd ? pastEnd : lastNode)->parentNode(); ancestor && depth; ancestor = ancestor->parentNode()) > > + exitedNode(*ancestor); > > + } > > if (depth) could be removed. Sure. Will do.

Committed r236609: <https://trac.webkit.org/changeset/236609>

<rdar://problem/44872428>