[webkit-reviews] review granted: [Bug 226803] Avoid some calls to StringView::toString() / StringView::toStringWithoutCopying() : [Attachment 430936] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 226803: Avoid some calls to StringView::toString() /
StringView::toStringWithoutCopying()
https://bugs.webkit.org/show_bug.cgi?id=226803

Attachment 430936: Patch

https://bugs.webkit.org/attachment.cgi?id=430936&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 430936
  --> https://bugs.webkit.org/attachment.cgi?id=430936
Patch

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

> Source/WTF/wtf/URL.cpp:559
> +    if constexpr (std::is_same_v<StringType, StringView>)
> +	   return input.toString();
> +    else
> +	   return input;

Another way to write this is:

    return makeString(input);

We would have to make sure it’s optimized well enough for the single argument
StringView and String cases, but I think it’s better than writing the if
statement.

> Source/WTF/wtf/text/TextStream.h:71
>      WTF_EXPORT_PRIVATE TextStream& operator<<(const char*);

If we remove the const char* and String overloads rather than adding an
AtomString one, will the StringView overload just be used instead? If so, would
be nice to remove them.

Also would be good at some point to find a way to support everything that
StringConcatenate does without having to write separate functions for each.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:539
> +    auto stringView = range.consumeIncludingWhitespace().value();

Do we really need to change the variable name?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3337
> +    for (unsigned i = 0; i < gridRowNames.length(); ++i) {

This should use a loop more like this:

    for (auto character : gridRowNames.codeUnits()) {

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3566
> +    return isPredefinedCounterStyle(nameToken.id()) ? AtomString {
name.convertToASCIILowercase() } : name.toAtomString();

If this is hot enough, should consider a
StringView::convertToASCIILowercaseAtom that optimizes the case for short
strings at least, and goes directly to an AtomString rather than allocating and
destroying a StringImpl if it happens to match an existing AtomString.