[webkit-reviews] review granted: [Bug 195870] Cut down on use of StringBuffer, possibly leading toward removing it entirely : [Attachment 364995] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 195870: Cut down on use of StringBuffer, possibly leading toward removing
it entirely
https://bugs.webkit.org/show_bug.cgi?id=195870

Attachment 364995: Patch

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




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 364995
  --> https://bugs.webkit.org/attachment.cgi?id=364995
Patch

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

> Source/WebCore/dom/Document.cpp:1509
> +static String canonicalizedTitle(Document& document, const String& title)

Just a thought for the future, take a StringView.

> Source/WebCore/dom/Document.cpp:1534
> +    return builder == title ? title : builder.toString();

Nice.

> Source/WebCore/platform/Length.cpp:89
>  UniqueArray<Length> newCoordsArray(const String& string, int& len)

OK as-is. You didn't touch this. Just a thought for the future, fix the
argument names.

> Source/WebCore/platform/Length.cpp:92
> +    UChar* spacified;

OK as-is. I get the name. Each time I see it though I keep thinking
"specified". Haha.

> Source/WebCore/platform/Length.cpp:94
>      for (unsigned i = 0; i < length; i++) {

OK as-is. Just a thought, StringView.codeUnit() this loop.

> Source/WebCore/platform/Length.cpp:109
>      auto r = makeUniqueArray<Length>(len);
>  
>      int i = 0;
>      unsigned pos = 0;
>      size_t pos2;

Ok as-is. This code could use more love, maybe even better variable names :D

> Source/WebCore/platform/text/TextCodecUTF16.cpp:73
> +    size_t numCodeUnits = numBytes / 2;

OK as-is. 2 is not memorable in my opinion, but I can figure out why.
sizeof(UChar) or something compile-time computable?

> Source/WebCore/rendering/RenderText.cpp:158
> +    for (unsigned i = 1; i < length + 1; i++)

OK as-is. ++i just for the laughs or the compiler goes dumb :D

> Source/WebCore/rendering/RenderText.cpp:177
> +    return result == string ? string : result.toString();

Nice.