Bug 26001 – Change callers of String::adopt() to String::createUninitialized()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dave Moore]

Bug https://bugs.webkit.org/show_bug.cgi?id=25779 introduced the new String::createUninitialized() which allows a caller to allocate a string with an inline buffer and write directly into it. Many of the callers of String::adopt() would do better to call this method.

[Dave Moore]

Created attachment 30642 [details]
Fx

This patch changes many of the uses of adopt() to createUninitialized(). Most
are just straight-forward wins. A couple are places where we walk through a
list of nodes and extract the strings from them. These used to build up the
string incrementally, appending to a buffer. Now I walk through the list twice,
the first to discover the total length and the second to write the string into
the inline buffer. I think it's worth the second navigation to keep everything
in one malloc node.

In places where the logic was somewhat complicated as to the creation of the to
be adopted buffer I left it as it was.

[Maciej Stachowiak]

Yay thanks for filing as a separate bug!

[Darin Adler]

Comment on attachment 30642 [details]
Fx

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.

[Dave Moore]

Created attachment 30660 [details]
Fix

Resolved objections

[Darin Adler]

Comment on attachment 30660 [details]
Fix

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 44135)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> [phone]  David Moore  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).

There's no comment here. Need to cite the bug number and URL and say what you're doing and why. Event better is to have a per-function comment explaining what was done to each function.

> +        int dataLength = data.length();

Should be unsigned.

r=me

[Dave Moore]

Created attachment 30737 [details]
Fix

Fixed minor objections

[Darin Adler]

Comment on attachment 30737 [details]
Fix

> +        if (c->nodeType() == Node::TEXT_NODE || c->nodeType() == Node::CDATA_SECTION_NODE || c->nodeType() == Node::COMMENT_NODE) {

Since nodeType() is a virtual function, it's too bad we're calling it three times on each non-textual node.

> +            int nodeLength = nodeValue.length();

Should be unsigned, not int.

> +        int dataLength = data.length();

Should be unsigned, not int.

r=me

[Dave Moore]

Created attachment 30762 [details]
Fix

Hopefully this resolves the remaining nits

[David Levin]

Assign to levin for landing.

[David Levin]

Committed in http://trac.webkit.org/changeset/44279.
and then fixed the windows build failure in http://trac.webkit.org/changeset/44281.