[webkit-reviews] review denied: [Bug 31415] [v8] do not copy data twice when converting short v8 string into WebCore::String : [Attachment 43080] Addressing Vitaly's comments

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied anton muhin
<antonm at chromium.org>'s request for review:
Bug 31415: [v8] do not copy data twice when converting short v8 string into
WebCore::String
https://bugs.webkit.org/show_bug.cgi?id=31415

Attachment 43080: Addressing Vitaly's comments
https://bugs.webkit.org/attachment.cgi?id=43080&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
> Index: WebCore/ChangeLog

> +	   Do not use WebCore::String::String(const UChar*, int length) to
convert
> +	   short v8 strings.
> +
> +	   Plus added string traits.

It is good to add the reason for the change to the ChangeLog as well as what
was done.
Ideally, perf optimizations would come with some measurement to show the actual
win.

> +    static String convertFromV8(v8::Handle<v8::String> v8String, int length)
{
> +	   ASSERT(v8String->Length() == length);
> +	   // NOTE: as of now, String(const UChar*, int) performs
String::createUninitialized
> +	   // anyway, so no need to optimize like we do for AtomicString below.

> +	   UChar* buffer;
> +	   String result = String::createUninitialized(length, buffer);
> +	   v8String->Write(reinterpret_cast<uint16_t*>(buffer), 0, length);
> +	   return result;

It is unfortunate that this introduces almost a duplicate of the code... Could
you consider having the StringTraits to have some simple bool flag, like
"useInlineBuffer" and have a single copy of the code, something like this:
if (StringTraits<StringType>::useInlineBuffer() && length <= inlineBufferSize)
... use inline buffer ...
} else {
... use createUninitialized ...
}
or some other way to keep the code in one place?