...
<rdar://problems/63734491>
Created attachment 401804 [details] Patch
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.
(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.
Created attachment 401909 [details] Patch for landing
Committed r263044: <https://trac.webkit.org/changeset/263044> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401909 [details].