Bug 210866 – TextManipulationController should set range of paragraph using token's positions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 397226 [details]
Patch

[Sihui Liu]

API test failure should be caused by: https://bugs.webkit.org/show_bug.cgi?id=210871.

[Sihui Liu]

<rdar://problem/60646283>

[Darin Adler]

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>.

[Sihui Liu]

Created attachment 397291 [details]
Patch

[Darin Adler]

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.

[Sihui Liu]

(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.

[Darin Adler]

(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.

[Sihui Liu]

Created attachment 397304 [details]
Patch

[Sihui Liu]

(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.

[Wenson Hsieh]

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

[Sihui Liu]

Created attachment 397352 [details]
Patch for landing

[Darin Adler]

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.

[EWS]

Committed r260578: <https://trac.webkit.org/changeset/260578>

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

[Ryosuke Niwa]

Nice!