[webkit-reviews] review granted: [Bug 239412] Drop String::truncate() and use String::left() instead : [Attachment 457737] 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 239412: Drop String::truncate() and use String::left() instead
https://bugs.webkit.org/show_bug.cgi?id=239412

Attachment 457737: Patch

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




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

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

> Source/WebCore/dom/FragmentDirectiveParser.cpp:105
> +	       firstToken = firstToken.left(tokens.first().length() - 2);
>	       
>	       if (auto prefix =
WTF::URLParser::formURLDecode(tokens.takeFirst()))

It’s unnecessary to truncate in place. Instead it can be more like this:

    auto takenFirstToken = tokens.takeFirst();
    if (auto prefix =
URLParser::formURLDecode(StringView(takenFirstToken).left(firstToken.length() -
2)))

Notice that this means we don’t have to call String::left, which allocates
memory!

> Source/WebCore/html/HTMLImageElement.cpp:216
>	       String type = typeAttribute.string();
> -	       type.truncate(type.find(';'));
> -	       type = stripLeadingAndTrailingHTMLSpaces(type);
> +	       type =
stripLeadingAndTrailingHTMLSpaces(type.left(type.find(';')));
>	       if (!type.isEmpty() &&
!MIMETypeRegistry::isSupportedImageVideoOrSVGMIMEType(type))
>		   continue;

This allocates an intermediate string after finding the ";", but before
stripping the spaces, so it can allocate two strings. We can do this without
double allocation.

    StringView typeView = typeAttribute;
    typeView =
typeView.left(typeView.find(';)).stripLeadingAndTrailingMatchedCharacters(isHTM
LSpace<UChar>);
    if (!typeView.isEmpty()) {
	auto typeString = typeView == typeAttribute ? typeAttribute.string() :
typeView.toString();
	if (!MIMETypeRegistry::isSupportedImageVideoOrSVGMIMEType(typeString))
	    continue;
    }

But unfortunately it’s longer code. Not sure how you feel about this.

> Source/WebCore/html/TextFieldInputType.cpp:627
> +    eventText = eventText.left(textLength).replace("\r\n", "
").replace('\r', ' ').replace('\n', ' ');

New code is no worse, but so inefficient to make so many buffers! This
operation could be written to use a maximum of one buffer by working on a copy
of the string in place, but instead way this code works, we can have up to 3
intermediate String objects plus the final String object passed into limitText,
which might create one more. A worst case of 5 malloc/free pairs instead of 1.
Probably doesn’t matter, though.

> Source/WebCore/loader/FrameLoader.cpp:760
> +	       headerContentLanguage =
stripLeadingAndTrailingHTMLSpaces(headerContentLanguage.left(headerContentLangu
age.find(','))); // notFound == -1 == don't truncate.

This can allocate two strings instead of 1. This is the exact same operation as
HTMLImageElement::bestFitSourceFromPictureElement, but with "," instead of ";",
with the same opportunity for a better algorithm.

>
Source/WebCore/platform/graphics/avfoundation/objc/AVAssetTrackUtilities.mm:49
> +	       auto codecStringView =
StringView(codecString).left(codecString.find('.'));
> +	       auto codecIdentifier =
FourCC::fromString(codecStringView.left(4));

How about a one liner:

    if (auto identifier =
FourCC::fromString(StringView(codecString).left(codecString.find('.')).left(4))
)

> Source/WebKitLegacy/win/WebDownloadCFNet.cpp:195
> +	   m_destination = StringView(m_bundlePath).left(m_destination.length()
- DownloadBundle::fileExtension().length()).toString();

I’m not sure I understand why isolatedCopy is needed. If it’s not, then we
should use String::left instead of StringView::left.

If it is needed, then not thrilled that it’s now hidden in the use of
StringView::toString. It good for efficiency that we don’t make an extra copy,
though, if we do need it!

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1342
> +    messageString = messageString.left(messageString.find(UChar(0)));

Could use nullCharacter from CharacterNames.h instead of UChar(0).