<rdar://problem/61066103>
Created attachment 396986 [details] Patch
Comment on attachment 396986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396986&action=review > Source/WebCore/editing/TextManipulationController.cpp:481 > + if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element))) What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too? > Source/WebCore/editing/TextManipulationController.cpp:488 > + if (i) Sad that we have to avoid range-based for just for this, but alas
Comment on attachment 396986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396986&action=review >> Source/WebCore/editing/TextManipulationController.cpp:481 >> + if (replacementTokens.size() > 1 && (!item.element || !canPerformTextManipulationByReplacingEntireTextContent(*item.element))) > > What's all the null checking? Didn't we just do this with `element` above? Why not just use that, too? Good catch! Changed this to just use `element` and avoid the redundant null check. >> Source/WebCore/editing/TextManipulationController.cpp:488 >> + if (i) > > Sad that we have to avoid range-based for just for this, but alas :P
Comment on attachment 396986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396986&action=review >>> Source/WebCore/editing/TextManipulationController.cpp:488 >>> + if (i) >> >> Sad that we have to avoid range-based for just for this, but alas > > :P I think we should add a StringBuilder feature for appending things with separators. This comes up a *lot*. > Source/WebCore/editing/TextManipulationController.cpp:489 > + newValue.appendLiteral(" "); Appending a space is more efficiently done with ' '.
(In reply to Darin Adler from comment #4) > Comment on attachment 396986 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396986&action=review > > >>> Source/WebCore/editing/TextManipulationController.cpp:488 > >>> + if (i) > >> > >> Sad that we have to avoid range-based for just for this, but alas > > > > :P > > I think we should add a StringBuilder feature for appending things with > separators. This comes up a *lot*. +1 to this. I also looked for a helper somewhere in WTF to join Vector<String> with a separator, but couldn’t find any. > > > Source/WebCore/editing/TextManipulationController.cpp:489 > > + newValue.appendLiteral(" "); > > Appending a space is more efficiently done with ' '. Good point — changed to newValue.append(' ');
Created attachment 397006 [details] Patch for landing
Committed r260393: <https://trac.webkit.org/changeset/260393> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397006 [details].