[webkit-reviews] review granted: [Bug 213156] Text manipulation does not observe newly displayed element inside previously observed content : [Attachment 401804] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 213156: Text manipulation does not observe newly displayed element inside
previously observed content
https://bugs.webkit.org/show_bug.cgi?id=213156

Attachment 401804: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 401804
  --> https://bugs.webkit.org/attachment.cgi?id=401804
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.