Add StringView.stripWhiteSpace()

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[Daniel Bates]

We should add StringView.stripWhiteSpace() as the analog to String.stripWhiteSpace() to avoid unnecessary allocation and copying of characters at call sites that need a string without leading and trailing whitespace temporarily.

[Daniel Bates]

Created attachment 309178 [details]
Patch and unit tests

[Daniel Bates]

Comment on attachment 309178 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=309178&action=review

> Source/WTF/wtf/text/StringCommon.h:54
> +    inline bool operator()(UChar ch) const

I am open to suggestion on the name of this parameter. Otherwise, will rename the parameter to c before landing for consistency with the name of the parameter to isSpaceOrNewline().

[Darin Adler]

Comment on attachment 309178 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=309178&action=review

> Source/WTF/ChangeLog:3
> +        Add StringView.stripWhiteSpace()

I don’t think we should do this. We almost never actually want stripWhiteSpace.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1924
> -    return node && node->nodeType() == Node::TEXT_NODE && node->nodeValue().stripWhiteSpace().length() == 0;
> +    return node && node->nodeType() == Node::TEXT_NODE && StringView(node->nodeValue()).stripWhiteSpace().isEmpty();

I think this should be stripping HTML spaces, not all whitespace.

[Daniel Bates]

(In reply to Darin Adler from comment #3)
> Comment on attachment 309178 [details]
> Patch and unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309178&action=review
> 
> > Source/WTF/ChangeLog:3
> > +        Add StringView.stripWhiteSpace()
> 
> I don’t think we should do this. We almost never actually want
> stripWhiteSpace.

I take it we should be using stripLeadingAndTrailingHTMLSpaces() whenever we want to strip whitespace from a string regardless of whether the string was extracted from markup.

> 
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:1924
> > -    return node && node->nodeType() == Node::TEXT_NODE && node->nodeValue().stripWhiteSpace().length() == 0;
> > +    return node && node->nodeType() == Node::TEXT_NODE && StringView(node->nodeValue()).stripWhiteSpace().isEmpty();
> 
> I think this should be stripping HTML spaces, not all whitespace.

OK.

[Darin Adler]

If this is the bug we were referring to where I expressed an objection, it’s an objection to having functions that make use of the concept of Unicode whitespace. I suspect this concept is only very rarely relevant in WebKit, where as the concept of an HTML space is likely relevant in *many* places.