Bug 210866

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!