[webkit-reviews] review denied: [Bug 26001] Change callers of String::adopt() to String::createUninitialized() : [Attachment 30642] Fx

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dave Moore <davemoore at google.com>'s
request for review:
Bug 26001: Change callers of String::adopt() to String::createUninitialized()
https://bugs.webkit.org/show_bug.cgi?id=26001

Attachment 30642: Fx
https://bugs.webkit.org/attachment.cgi?id=30642&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on!

When there's a single patch with some obviously-right parts and other
more-debatable parts, I highly recommend getting the obviously-right part
reviewed and landed separately first.

> +	   WARNING: NO TEST CASES ADDED OR CHANGED

If you don't think a patch requires new test cases, then please remove this
line before posting the patch for review.

> +    int resultLength = 0;

I think unsigned would be better than int. Maybe size_t, but I guess we just
use unsigned for String at this point.

>      for (Node* c = e->firstChild(); c; c = c->nextSibling())
>	   if (c->nodeType() == Node::TEXT_NODE || c->nodeType() ==
Node::CDATA_SECTION_NODE || c->nodeType() == Node::COMMENT_NODE)
> -	       append(text, c->nodeValue());
> +	       resultLength += c->nodeValue().length();

Brace style was not matching WebKit style guide in the old version of this
code. The for statement should have braces.

> +    String result = String::createUninitialized(resultLength, text);

I don't think result is the best name for this. It's the string that will be
used to create a stylesheet. Not a function result. Maybe combinedText or
sheetText or text.

> +	       UChar* data;
> +	       PassRefPtr<StringImpl> newImpl = 
> +		   StringImpl::createUninitialized(m_impl->length() +
str.length(), data);
> +	       memcpy(data, m_impl->characters(), m_impl->length() *
sizeof(UChar));
> +	       memcpy(data + m_impl->length(), str.characters(), str.length() *
sizeof(UChar));
> +	       m_impl = newImpl;

This should use RefPtr and release, not PassRefPtr -- we don't use PassRefPtr
for local variables. Please read http://webkit.org/coding/RefPtr.html for
details. Same comment in the many other places PassRefPtr is used.

> -    data.resize(realLength);
> -    Unicode::toLower(data.characters(), realLength, m_data, m_length,
&error);
> +	   return newImpl;
> +    newImpl = createUninitialized(realLength, data);
> +    Unicode::toLower(data, realLength, m_data, m_length, &error);

This new implementation requires a complete new buffer if the lowercase is even
slightly longer. The old one at least allowed the common case where realloc
could get the original space to work without a new allocation. I guess the new
way is probably OK since this case is so unusual, but it would be interesting
to know if we could someone retain the resize optimization.

review- because of the PassRefPtr issue.