Bug 57843 – Switch from Vector<UChar> to StringBuilder in dom/

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nikolas Zimmermann]

As discussed in bug 56099, we should deploy StringBuilder usage in WebCore where possible.
This bug addresses Source/WebCore/dom.

[Nikolas Zimmermann]

Created attachment 88225 [details]
Patch

[Andreas Kling]

Comment on attachment 88225 [details]
Patch

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

> Source/WebCore/dom/DatasetDOMStringMap.cpp:124
> +            builder.append(toASCIILower(characters[i]));

Why characters[i] and not character? And why do you need the local regardless?

[Nikolas Zimmermann]

Comment on attachment 88225 [details]
Patch

Sorry Andreas, I should have removed r? before. As discussed with Maciej, he prefers to optimize operator+ instead of having to change all callsites that append just 2 strings. I'll look into that.

[Nikolas Zimmermann]

Comment on attachment 88225 [details]
Patch

Marking patch for review again, now that 58420 is fixed. This patch is unrelated to the String operator+ optimization, and should go in as well.
Maciej, can you have another look please?
Vector<UChar> -> StringBuilder should be fine.

[Darin Adler]

Comment on attachment 88225 [details]
Patch

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

>> Source/WebCore/dom/DatasetDOMStringMap.cpp:124
>> +            builder.append(toASCIILower(characters[i]));
> 
> Why characters[i] and not character? And why do you need the local regardless?

I agree with both comments. There is little benefit to using the local, but if we do use it we should use it in all three places.

[Nikolas Zimmermann]

Thanks Darin, landed in r86553. Adressed Darin/Andreas comment in r86555 regarding the local variable.