| Summary: | Switch from Vector<UChar> to StringBuilder in dom/ | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||
| Component: | WebCore Misc. | Assignee: | Nikolas Zimmermann <zimmermann> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | kling | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Bug Depends on: | 58420 | ||||||
| Bug Blocks: | |||||||
| Attachments: |
|
||||||
|
Description
Nikolas Zimmermann
2011-04-05 07:22:14 PDT
Created attachment 88225 [details]
Patch
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? 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.
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.
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. |