[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