Bug 171731

Summary: Add StringView.stripWhiteSpace()
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Web Template FrameworkAssignee: Daniel Bates <dbates>
Status: RESOLVED WONTFIX    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, darin, mkwst, sam
Priority: P2    
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=175757
Attachments:
Description Flags
Patch and unit tests none

Daniel Bates
Reported 2017-05-05 09:31:35 PDT
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.
Attachments
Patch and unit tests (19.85 KB, patch)
2017-05-05 09:51 PDT, Daniel Bates 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2017-05-05 09:51:44 PDT
Created attachment 309178 [details] Patch and unit tests
Daniel Bates
Comment 2 2017-05-05 09:54:49 PDT
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 3 2017-05-05 16:30:41 PDT
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
Comment 4 2017-05-26 17:14:55 PDT
(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
Comment 5 2017-08-21 08:50:50 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.