[webkit-reviews] review granted: [Bug 239426] Leverage StringView in more places : [Attachment 457767] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 239426: Leverage StringView in more places
https://bugs.webkit.org/show_bug.cgi?id=239426

Attachment 457767: Patch

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




--- Comment #6 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 457767
  --> https://bugs.webkit.org/attachment.cgi?id=457767
Patch

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:148
> +    stringValue =
StringView(stringValue).stripWhiteSpace().convertToASCIILowercase();

Going through the convertToASCIILowercase() and seeing how many can be
converted to either explicit equalIgnoringASCIICase() or case-ignore
SortedArrayMap would be a good investment.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:234
> +		   purposeStringValue =
StringView(purposeStringValue).stripWhiteSpace().convertToASCIILowercase();
>		   Vector<String> keywords =
purposeStringValue.splitAllowingEmptyEntries(" ");

This could totally be done without any allocation using SortedArrayMap and
StringView's functor based split.

> Source/WebCore/editing/Editor.cpp:3224
> +    String transposed = makeString(text[1], text[0]);

Unrelated to this change, but it seems like this is totally not going to work
with all graphemes. I bet transposing (control-t) two emoji doesn't work. Yep.

> Source/WebCore/platform/network/CacheValidation.cpp:301
> +		   double maxAge = directives[i].second.toDouble(ok);

Another good project for the future is replacing these out parameter based
conversion functions with std:;optional returning ones.

> Source/WebCore/platform/network/MIMEHeader.cpp:125
> +    auto encoding = text.stripWhiteSpace();

Another good SortedArrayMap use case for the future.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:464
> +	   if (!executeCommandSlow(makeString("DROP TABLE ", table)))

Because string concatenation and databases make me very uncomfortable, can this
/ should this use prepareStatement + '?' + bindText()? (not relevant to this
change, but a question none-the-less).

> Source/WebCore/platform/xr/PlatformXR.h:120
> +    auto feature = string.stripWhiteSpace().convertToASCIILowercase();

This would be more efficient / idiomatic if we switch to use a case insensitive
SortedArrayMap (for the future, not relevant to the kind of changes you are
making).