With different ranges, TextIterator may visit different set of nodes, which makes token matching hard, so it's better to make the range of paragraph close to the actual paragraph as much as possible.
Created attachment 397226 [details] Patch
API test failure should be caused by: https://bugs.webkit.org/show_bug.cgi?id=210871.
<rdar://problem/60646283>
Comment on attachment 397226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397226&action=review I’ll review again if there’s a version where the iOS API tests pass. > Source/WebCore/editing/TextManipulationController.cpp:296 > + if (content.node && !content.node->isTextNode() && currentEndOfCurrentParagraph.equals(endOfCurrentParagraph)) Instead of "!content.node->isTextNode()", could write "!is<Text>(*content.node)". > Source/WebCore/editing/TextManipulationController.cpp:312 > + if (content.node && is<Text>(content.node) && startOfCurrentLine < offsetOfNextNewLine) { No need to add the null check here; is<Text> includes the null check. > Source/WebCore/editing/TextManipulationController.cpp:323 > + if (content.node && content.node->hasTagName(brTag)) Could consider: is<HTMLBRElement>(content.node) instead. Might require including an additional header, though. > Source/WebCore/editing/TextManipulationController.cpp:331 > + if (remainingText.length() && !remainingText.toString().stripWhiteSpace().isEmpty()) { No need to check remainingText.length() here. The stripWhiteSpace function is likely the wrong one to use in a few ways: 1) It strips all Unicode whitespace characters. We probably want to strip only HTML whitespace characters, or maybe even strip spaces based on the prevailing whitespace mode. That would be a function more like stripLeadingAndTrailingHTMLSpaces. 2) Checking if a StringView contains only HTML spaces does not require the costly operation of allocating a string. A good way to write this would be a function named containsOnlyHTMLSpace, and then containsOnlyHTMLSpace(remainingText). We could possibly implement this as !remainingText.contains(isNotHTMLSpace) without adding anything. > Source/WebCore/editing/TextManipulationController.cpp:333 > + if (startOfCurrentLine && content.node && is<Text>(content.node)) No need for the null check of content.node, because is<Text> includes that. > Source/WebCore/editing/TextManipulationController.cpp:539 > + if (content.isTextContent && content.text.toString().stripWhiteSpace().isEmpty()) { Same comment about stripWhitespace. > Source/WebCore/editing/TextManipulationController.cpp:541 > + if (content.node && content.node->hasTagName(brTag)) Same comment about is<HTMLBRElement>.
Created attachment 397291 [details] Patch
Comment on attachment 397291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397291&action=review > Source/WebCore/editing/TextManipulationController.cpp:250 > +static bool containsNonHTMLSpace(const StringView& text) StringView is always fine, don’t need const StringView&, it just makes it less efficient. That’s unlike other things that are costly to copy, where const& could be an important optimization. I suggest containsOnlyHTMLSpaces rather than containsNonHTMLSpace.
(In reply to Darin Adler from comment #6) > Comment on attachment 397291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=397291&action=review > > > Source/WebCore/editing/TextManipulationController.cpp:250 > > +static bool containsNonHTMLSpace(const StringView& text) > > StringView is always fine, don’t need const StringView&, it just makes it > less efficient. That’s unlike other things that are costly to copy, where > const& could be an important optimization. > > I suggest containsOnlyHTMLSpaces rather than containsNonHTMLSpace. Okay. Will update the naming after the api-ios bot runs.
(In reply to Sihui Liu from comment #7) > (In reply to Darin Adler from comment #6) > > I suggest containsOnlyHTMLSpaces rather than containsNonHTMLSpace. > > Okay. Will update the naming after the api-ios bot runs. Note, not just naming, but also reverses the boolean sense.
Created attachment 397304 [details] Patch
(In reply to Darin Adler from comment #8) > (In reply to Sihui Liu from comment #7) > > (In reply to Darin Adler from comment #6) > > > I suggest containsOnlyHTMLSpaces rather than containsNonHTMLSpace. > > > > Okay. Will update the naming after the api-ios bot runs. > > Note, not just naming, but also reverses the boolean sense. Right! Updated it.
Comment on attachment 397304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397304&action=review > Source/WebCore/ChangeLog:12 > + start of a paragraph can be set as the first visible position of document, while postion of first token Nit - postion => position
Created attachment 397352 [details] Patch for landing
Comment on attachment 397352 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=397352&action=review > Source/WebCore/editing/TextManipulationController.cpp:258 > +static bool containsOnlyHTMLSpaces(StringView text) > +{ > + for (unsigned index = 0; index < text.length(); ++index) { > + if (isNotHTMLSpace(text[index])) > + return false; > + } > + > + return true; > +} We eventually will want to move this to another file so it can be shared, but it’s fine to land it exactly like this right now.
Committed r260578: <https://trac.webkit.org/changeset/260578> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397352 [details].
Nice!