Summary: | Cut down on use of StringBuffer, possibly leading toward removing it entirely | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||
Component: | WebKit Misc. | Assignee: | Darin Adler <darin> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, dbates, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Darin Adler
2019-03-17 18:42:40 PDT
Created attachment 364995 [details]
Patch
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 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 on attachment 364995 [details] Patch Clearing flags on attachment: 364995 Committed r243118: <https://trac.webkit.org/changeset/243118> All reviewed patches have been landed. Closing bug. |