Bug 195870 – Cut down on use of StringBuffer, possibly leading toward removing it entirely

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Cut down on use of StringBuffer, possibly leading toward removing it entirely

[Darin Adler]

Created attachment 364995 [details]
Patch

[Daniel Bates]

Comment on attachment 364995 [details]
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.

[Darin Adler]

Comment on attachment 364995 [details]
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.

I did consider that. I kept it as a String because of that last line of code below (the one you commented "Nice" on!).

>> 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.

I did that and then undid it.

>> 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.

I did consider making this better as well but decided to leave it alone.

>> Source/WebCore/platform/Length.cpp:94
>>      for (unsigned i = 0; i < length; i++) {
> 
> OK as-is. Just a thought, StringView.codeUnit() this loop.

Because "I" is used to index into both the input string and the "spacified" string I could only do that if I was changing more.

>> Source/WebCore/platform/Length.cpp:109
>>      size_t pos2;
> 
> Ok as-is. This code could use more love, maybe even better variable names :D

Yes, what it really needs is a completely different implementation. No reason we have to copy things just to skip over all the illegal characters.

>> 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?

Yes, could say sizeof(UChar). I’d like to use the commit queue, so I’ll come back to do it later.

>> 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

Sure, I’d do that, but I’d like to use the commit queue so I won’t for now at least.

[WebKit Commit Bot]

Comment on attachment 364995 [details]
Patch

Clearing flags on attachment: 364995

Committed r243118: <https://trac.webkit.org/changeset/243118>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49001348>