[webkit-reviews] review granted: [Bug 239356] Leverage StringView in more places to avoid some String allocations : [Attachment 457653] 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 239356: Leverage StringView in more places to avoid some String allocations
https://bugs.webkit.org/show_bug.cgi?id=239356

Attachment 457653: Patch

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




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

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

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:94
> +	       result.append(std::pair { lineNumber, line.toString() });

I guess we do need the explicit std::pair; I would omit it if we could.

> Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:167
> +		   contentType = StringView(header).substring(contentTypeBegin
+ contentTypePrefixLength, contentTypeEnd - contentTypeBegin -
contentTypePrefixLength).stripLeadingAndTrailingMatchedCharacters(isHTTPSpace).
toString();

Is there a very-common case where the header string happens to have nothing
that need to be trimmed, no "\r\n", no spaces? I ask because that is the case
will become *less* efficient, now copying the String.

I wish switching to StringView didn’t make the
stripLeadingAndTrailingHTTPSpaces() function call *so* much longer
stripLeadingAndTrailingMatchedCharacters(isHTTPSpace). On reflection, seems
unnecessarily to have "matched characters" in that function name.

>
Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCRtpTransceiverBackend.cpp
:94
> +    rtcCodec.name = StringView(codec.mimeType).substring(6).utf8().data();

Since we are setting a std::string here it would be better if CString knew how
to convert to std::string without writing .data(), which makes the code look
unnecessarily dangerous, although it’s actually correct and fine. Kind of
annoying that we need both CString and std::string in WebKit. Longer term, I
wonder how many unique great things CString has; maybe we can drop it and
switch to std::string. It uses our faster malloc, but otherwise seems to have
no significantly different features from std::string.

> Source/WebCore/html/HTMLMapElement.cpp:103
>	   m_name = mapName;

WTFMove(mapName)

> Source/WebCore/page/Location.cpp:202
> +    if (!hash.isEmpty() && hash[0] == '#')

We have startsWith(UChar) function for cases like this.

> Source/WebCore/page/UserContentURLPattern.h:37
> +    UserContentURLPattern(StringView pattern)

Should we add explicit?

> Source/WebCore/page/UserContentURLPattern.h:38
>      : m_matchSubdomains(false)

Normally we indent.

> Source/WebCore/platform/network/HTTPParsers.cpp:681
>      // FIXME: The rest of this should use StringView.

Does this comment still make sense?