[Webkit-unassigned] [Bug 26001] Change callers of String::adopt() to String::createUninitialized()

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 25 13:33:05 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=26001


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #30642|review?                     |review-
               Flag|                            |




------- Comment #3 from darin at apple.com  2009-05-25 13:33 PDT -------
(From update of attachment 30642)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list