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.
Created attachment 309178 [details] Patch and unit tests
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().
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.
(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.
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.