[webkit-reviews] review granted: [Bug 190073] Simplify StyledMarkupAccumulator::traverseNodesForSerialization : [Attachment 351066] Cleanup

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 190073: Simplify StyledMarkupAccumulator::traverseNodesForSerialization
https://bugs.webkit.org/show_bug.cgi?id=190073

Attachment 351066: Cleanup

https://bugs.webkit.org/attachment.cgi?id=351066&action=review




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 351066
  --> https://bugs.webkit.org/attachment.cgi?id=351066
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.