Bug 195870 - Cut down on use of StringBuffer, possibly leading toward removing it entirely
Summary: Cut down on use of StringBuffer, possibly leading toward removing it entirely
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-17 18:42 PDT by Darin Adler
Modified: 2019-03-18 17:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.90 KB, patch)
2019-03-17 19:04 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-03-17 18:42:40 PDT
Cut down on use of StringBuffer, possibly leading toward removing it entirely
Comment 1 Darin Adler 2019-03-17 19:04:48 PDT
Created attachment 364995 [details]
Patch
Comment 2 Daniel Bates 2019-03-18 14:20:50 PDT
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.
Comment 3 Darin Adler 2019-03-18 17:00:27 PDT
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.
Comment 4 WebKit Commit Bot 2019-03-18 17:27:16 PDT
Comment on attachment 364995 [details]
Patch

Clearing flags on attachment: 364995

Committed r243118: <https://trac.webkit.org/changeset/243118>
Comment 5 WebKit Commit Bot 2019-03-18 17:27:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-03-18 17:28:19 PDT
<rdar://problem/49001348>