Bug 257084 – Remove no-argument simplifyWhiteSpace()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Anne van Kesteren]

Making callers responsible for the type of whitespace they want simplified as the current default is wrong.

Now I have a patch for this as this seemed like a nice incremental step toward improving the whitespace situation, but it's not compiling as replacing existing callers with simplifyWhitespace(isSpaceOrNewline) is apparently not identical:

In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource88.cpp:4:
accessibility/AccessibilityNodeObject.cpp:583:76: error: cannot initialize a parameter of type 'WTF::CodeUnitMatchFunction' (aka 'bool (*)(char16_t)') with an lvalue of type 'bool (UChar32)' (aka 'bool (int)'): type mismatch at 1st parameter ('UChar' (aka 'char16_t') vs 'UChar32' (aka 'int'))
        String string = stringValue().stripWhiteSpace().simplifyWhiteSpace(isSpaceOrNewline);
In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource88.cpp:1:
In file included from /Users/annevk/Dev/OpenSource/Source/WebCore/WebCorePrefix.h:168:
In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/AtomString.h:25:
/Users/annevk/Dev/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/WTFString.h:199:90: note: passing argument to parameter here
    WTF_EXPORT_PRIVATE String WARN_UNUSED_RETURN simplifyWhiteSpace(CodeUnitMatchFunction) const;
In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource88.cpp:4:
accessibility/AccessibilityNodeObject.cpp:2448:90: error: cannot initialize a parameter of type 'WTF::CodeUnitMatchFunction' (aka 'bool (*)(char16_t)') with an lvalue of type 'bool (UChar32)' (aka 'bool (int)'): type mismatch at 1st parameter ('UChar' (aka 'char16_t') vs 'UChar32' (aka 'int'))
        text = (element ? element->innerText() : node->textContent()).simplifyWhiteSpace(isSpaceOrNewline);
In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource88.cpp:1:
In file included from /Users/annevk/Dev/OpenSource/Source/WebCore/WebCorePrefix.h:168:
In file included from /Users/annevk/Dev/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/AtomString.h:25:
/Users/annevk/Dev/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/text/WTFString.h:199:90: note: passing argument to parameter here
    WTF_EXPORT_PRIVATE String WARN_UNUSED_RETURN simplifyWhiteSpace(CodeUnitMatchFunction) const;
2 errors generated.

I suppose I could make isSpaceOrNewline templated, but that feels like a step in the opposite direction.

Darin, any recommendations?

[Anne van Kesteren]

I tried to make isSpaceOrNewline into a template (modeled after isASCIIWhitespace), but that still doesn't jive with CodeUnitMatchFunction. My knowledge of this space is not sufficient to tackle this problem.

I even tried going from UChar32 to UChar for these Unicode whitespace predicates as no White_Space code point goes outside of it, but that gave me

Undefined symbols for architecture arm64e:
  "WTF::StringImpl::simplifyWhiteSpace(bool (*)(char16_t))", referenced from:
      WebCore::newCoordsArray(WTF::String const&, int&) in UnifiedSource273.o
ld: symbol(s) not found for architecture arm64e
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[Darin Adler]

That last error looks like a missing WTF_EXPORT_PRIVATE.

I’m not sure why we have simplifyWhitespace. It seems rarely useful to turn runs of spaces into a single one. Functions that split on spaces can be made to work with runs easily. It might be easier to remove the callers one by one rather than fixing this.

How many callers do we have? I wonder what useful they are doing.

[Anne van Kesteren]

There's not many callers of simplifyWhiteSpace() so that might be doable. Already identified some dead code.

I'm kinda worried I will run into similar compiler issues when removing stripWhiteSpace(), but I'll investigate.

% git grep simplifyWhiteSpace Source            
Source/WTF/wtf/text/StringImpl.cpp:Ref<StringImpl> StringImpl::simplifyWhiteSpace()
Source/WTF/wtf/text/StringImpl.cpp:Ref<StringImpl> StringImpl::simplifyWhiteSpace(CodeUnitMatchFunction isWhiteSpace)
Source/WTF/wtf/text/StringImpl.h:    WTF_EXPORT_PRIVATE Ref<StringImpl> simplifyWhiteSpace();
Source/WTF/wtf/text/StringImpl.h:    Ref<StringImpl> simplifyWhiteSpace(CodeUnitMatchFunction);
Source/WTF/wtf/text/WTFString.cpp:String String::simplifyWhiteSpace() const
Source/WTF/wtf/text/WTFString.cpp:    return m_impl ? m_impl->simplifyWhiteSpace() : String { };
Source/WTF/wtf/text/WTFString.cpp:String String::simplifyWhiteSpace(CodeUnitMatchFunction isWhiteSpace) const
Source/WTF/wtf/text/WTFString.cpp:    return m_impl ? m_impl->simplifyWhiteSpace(isWhiteSpace) : String { };
Source/WTF/wtf/text/WTFString.h:    WTF_EXPORT_PRIVATE String WARN_UNUSED_RETURN simplifyWhiteSpace() const;
Source/WTF/wtf/text/WTFString.h:    WTF_EXPORT_PRIVATE String WARN_UNUSED_RETURN simplifyWhiteSpace(CodeUnitMatchFunction) const;
Source/WebCore/accessibility/AccessibilityNodeObject.cpp:        String string = stringValue().stripWhiteSpace().simplifyWhiteSpace();
Source/WebCore/accessibility/AccessibilityNodeObject.cpp:    return builder.toString().stripWhiteSpace().simplifyWhiteSpace(isHTMLSpaceButNotLineBreak);
Source/WebCore/accessibility/AccessibilityNodeObject.cpp:        text = (element ? element->innerText() : node->textContent()).simplifyWhiteSpace();
Source/WebCore/html/HTMLMediaElement.cpp:    auto title = String(attributeWithoutSynchronization(titleAttr)).stripWhiteSpace().simplifyWhiteSpace();
Source/WebCore/html/HTMLMediaElement.cpp:    title = document().title().stripWhiteSpace().simplifyWhiteSpace();
Source/WebCore/html/HTMLOptGroupElement.cpp:    itemText = itemText.simplifyWhiteSpace();
Source/WebCore/html/HTMLOptionElement.cpp:    return stripLeadingAndTrailingHTMLSpaces(document().displayStringModifiedByEncoding(text)).simplifyWhiteSpace(isASCIIWhitespace);
Source/WebCore/html/HTMLOptionElement.cpp:    return stripLeadingAndTrailingHTMLSpaces(collectOptionInnerText()).simplifyWhiteSpace(isASCIIWhitespace);
Source/WebCore/html/HTMLOptionElement.cpp:    return stripLeadingAndTrailingHTMLSpaces(collectOptionInnerText()).simplifyWhiteSpace(isASCIIWhitespace);
Source/WebCore/html/HTMLOptionElement.cpp:        return stripLeadingAndTrailingHTMLSpaces(collectOptionInnerText()).simplifyWhiteSpace(isASCIIWhitespace);
Source/WebCore/page/DragController.cpp:        editor.copyURL(linkURL, hitTestResult->textContent().simplifyWhiteSpace(), pasteboard);
Source/WebCore/page/DragController.cpp:        String textContentWithSimplifiedWhiteSpace = hitTestResult->textContent().simplifyWhiteSpace();
Source/WebCore/platform/Length.cpp:    RefPtr<StringImpl> str = string.impl()->simplifyWhiteSpace();
Source/WebCore/svg/SVGDescElement.cpp:    return textContent().simplifyWhiteSpace();
Source/WebCore/xml/XPathFunctions.cpp:        return s.simplifyWhiteSpace(isXMLSpace);
Source/WebCore/xml/XPathFunctions.cpp:    return s.simplifyWhiteSpace(isXMLSpace);
Source/WebCore/xml/XPathValue.cpp:            const String& str = m_data->string.simplifyWhiteSpace();

[Radar WebKit Bug Importer]

<rdar://problem/109770814>

[Anne van Kesteren]

Pull request: https://github.com/WebKit/WebKit/pull/14289

[EWS]

Committed 264480@main (cb0b8f5fb7c3): <https://commits.webkit.org/264480@main>

Reviewed commits have been landed. Closing PR #14289 and removing active labels.