Bug 213156 – Text manipulation does not observe newly displayed element inside previously observed content

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

<rdar://problems/63734491>

[Sihui Liu]

Created attachment 401804 [details]
Patch

[Darin Adler]

Comment on attachment 401804 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401804&action=review

> Source/WebCore/editing/TextManipulationController.cpp:774
> -        updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions);
> +        updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions, false);

This goes against WebKit coding style. We frown on functions with boolean arguments, where call sites pass a constant, since there’s no way to tell what "false" means. We prefer either separately named functions that make the difference clear, or enum classes for boolean values so you can see their meaning at the call site.

> Source/WebCore/editing/TextManipulationController.cpp:781
> -        insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *node });
> +        insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *node, false });

I think the same applies here, even though this is aggregate initialization rather than function calling. The enum class approach helps make these things easier to understand at the call site. The "false" is quite mysterious.

[Sihui Liu]

(In reply to Darin Adler from comment #3)
> Comment on attachment 401804 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401804&action=review
> 
> > Source/WebCore/editing/TextManipulationController.cpp:774
> > -        updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions);
> > +        updateInsertions(lastTopDownPath, topDownPath, nullptr, reusedOriginalNodes, insertions, false);
> 
> This goes against WebKit coding style. We frown on functions with boolean
> arguments, where call sites pass a constant, since there’s no way to tell
> what "false" means. We prefer either separately named functions that make
> the difference clear, or enum classes for boolean values so you can see
> their meaning at the call site.
> 
> > Source/WebCore/editing/TextManipulationController.cpp:781
> > -        insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *node });
> > +        insertions.append(NodeInsertion { lastTopDownPath.size() ? lastTopDownPath.last().second.ptr() : nullptr, *node, false });
> 
> I think the same applies here, even though this is aggregate initialization
> rather than function calling. The enum class approach helps make these
> things easier to understand at the call site. The "false" is quite
> mysterious.

Thanks for the review. I will use enum class IsNodeManipulated.

[Sihui Liu]

Created attachment 401909 [details]
Patch for landing

[EWS]

Committed r263044: <https://trac.webkit.org/changeset/263044>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401909 [details].