Bug 190073 - Simplify StyledMarkupAccumulator::traverseNodesForSerialization
Summary: Simplify StyledMarkupAccumulator::traverseNodesForSerialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 157443
  Show dependency treegraph
 
Reported: 2018-09-28 01:16 PDT by Ryosuke Niwa
Modified: 2018-09-28 13:04 PDT (History)
5 users (show)

See Also:


Attachments
WIP (6.18 KB, patch)
2018-09-28 01:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Cleanup (7.45 KB, patch)
2018-09-28 01:36 PDT, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-09-28 01:16:26 PDT
We need to simplify the traverse code in StyledMarkupAccumulator::traverseNodesForSerialization.
Comment 1 Ryosuke Niwa 2018-09-28 01:18:33 PDT
Created attachment 351064 [details]
WIP
Comment 2 Ryosuke Niwa 2018-09-28 01:36:35 PDT
Created attachment 351066 [details]
Cleanup
Comment 3 Ryosuke Niwa 2018-09-28 01:41:26 PDT
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 4 Antti Koivisto 2018-09-28 07:09:32 PDT
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.
Comment 5 Ryosuke Niwa 2018-09-28 12:52:48 PDT
(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.
Comment 6 Ryosuke Niwa 2018-09-28 13:03:47 PDT
Committed r236609: <https://trac.webkit.org/changeset/236609>
Comment 7 Radar WebKit Bug Importer 2018-09-28 13:04:37 PDT
<rdar://problem/44872428>