[webkit-reviews] review denied: [Bug 67079] Replace usages of Vector<UChar> with existing StringBuilder : [Attachment 105476] patch v2

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 67079: Replace usages of Vector<UChar> with existing StringBuilder
https://bugs.webkit.org/show_bug.cgi?id=67079

Attachment 105476: patch v2
https://bugs.webkit.org/attachment.cgi?id=105476&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105476&action=review


This patch looks good, would be great to be able to remove the String::adopt
method, do you know how far this takes us in that direction?
r- for a couple of small issues, but generally looks fine to me.

> Source/WebCore/platform/text/TextStream.cpp:87
>      size_t stringLength = strlen(string);

I think this function should just be
{ m_text.append(string); return *this; }

(If the StringBuilder needs to grow the buffer it should be checking for
overflow itself).

> Source/WebCore/storage/IDBLevelDBCoding.cpp:275
> +	   result.append(static_cast<UChar>((hi << 8) | lo));

Errrrk! – this looks like a(n existing) bug!
'hi' is an unsigned char, but is left shifted by 8.
You can probably fix this like this:

result.append((static_cast<UChar>(hi) << 8) | lo);

> Source/WebCore/xml/parser/CharacterReferenceParserInlineMethods.h:47
> +	  
source.prepend(SegmentedString(String(consumedCharacters.characters(),
consumedCharacters.length())));

To get the current contents of a StringBuilder as a String you could call
consumedCharacters.toString()
(or maybe consumedCharacters.toStringPreserveCapacity() )
instead of "String(consumedCharacters.characters(),
consumedCharacters.length())".