Bug 26001 - Change callers of String::adopt() to String::createUninitialized()
Summary: Change callers of String::adopt() to String::createUninitialized()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-24 23:42 PDT by Dave Moore
Modified: 2009-05-29 15:44 PDT (History)
2 users (show)

See Also:


Attachments
Fx (22.49 KB, patch)
2009-05-24 23:47 PDT, Dave Moore
darin: review-
Details | Formatted Diff | Diff
Fix (22.36 KB, patch)
2009-05-25 15:28 PDT, Dave Moore
darin: review+
Details | Formatted Diff | Diff
Fix (22.97 KB, patch)
2009-05-28 07:50 PDT, Dave Moore
darin: review+
Details | Formatted Diff | Diff
Fix (23.18 KB, patch)
2009-05-28 19:11 PDT, Dave Moore
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Moore 2009-05-24 23:42:07 PDT
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.
Comment 1 Dave Moore 2009-05-24 23:47:19 PDT
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.
Comment 2 Maciej Stachowiak 2009-05-25 00:42:39 PDT
Yay thanks for filing as a separate bug!
Comment 3 Darin Adler 2009-05-25 13:33:05 PDT
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.
Comment 4 Dave Moore 2009-05-25 15:28:48 PDT
Created attachment 30660 [details]
Fix

Resolved objections
Comment 5 Darin Adler 2009-05-26 09:12:30 PDT
Comment on attachment 30660 [details]
Fix

> Index: ChangeLog
> ===================================================================
> --- ChangeLog	(revision 44135)
> +++ ChangeLog	(working copy)
> @@ -1,3 +1,31 @@
> +2009-05-25  David Moore  <davemoore@chromium.org>
> +
> +        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
Comment 6 Dave Moore 2009-05-28 07:50:15 PDT
Created attachment 30737 [details]
Fix

Fixed minor objections
Comment 7 Darin Adler 2009-05-28 09:18:19 PDT
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
Comment 8 Dave Moore 2009-05-28 19:11:57 PDT
Created attachment 30762 [details]
Fix

Hopefully this resolves the remaining nits
Comment 9 David Levin 2009-05-29 11:02:28 PDT
Assign to levin for landing.
Comment 10 David Levin 2009-05-29 15:44:51 PDT
Committed in http://trac.webkit.org/changeset/44279.
and then fixed the windows build failure in http://trac.webkit.org/changeset/44281.