[webkit-reviews] review canceled: [Bug 200862] Use makeString and multi-argument StringBuilder::append instead of less efficient multiple appends : [Attachment 376639] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has canceled Darin Adler <darin at apple.com>'s
request for review:
Bug 200862: Use makeString and multi-argument StringBuilder::append instead of
less efficient multiple appends
https://bugs.webkit.org/show_bug.cgi?id=200862

Attachment 376639: Patch

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




--- Comment #18 from Darin Adler <darin at apple.com> ---
Comment on attachment 376639
  --> https://bugs.webkit.org/attachment.cgi?id=376639
Patch

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

I’ll upload a new patch after I fix the bug in StringBuilder::append.

>> Source/WebCore/css/CSSFontFaceRule.cpp:53
>>	StringBuilder result;
> 
> This is unused and can be removed.

Done.

>> Source/WebCore/css/CSSFontFaceSrcValue.cpp:77
>> +	result.append(m_resource, ')');
> 
> It might read a little nicer to put literals ("local(" or "url(") as a
ternary expression for the first parameter.

Done, and then converted the function to makeString instead of StringBuilder.

>> Source/WebCore/css/CSSGradientValue.cpp:941
>> +		    result.append(", ", stop.m_color->cssText(), ')');
> 
> If you wanted to, you could collapse all the appends here doing,
result.append("color-stop(", FormattedNumber::fixedPrecision(position), ", ",
stop.m_color->cssText(), ')').

Yes, happy to do that. I think there’s quite a bit more to collapse elsewhere
too; I was working on doing more and more of this and it got a little "out of
hand" and so I stopped myself and wanted to land what I had.

Done.