Bug 95508 - Remove all-but-one use of WTF::String::operator+= from WebCore
Summary: Remove all-but-one use of WTF::String::operator+= from WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-30 16:11 PDT by Adam Barth
Modified: 2012-09-01 01:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (18.62 KB, patch)
2012-08-30 16:20 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-08-30 16:11:49 PDT
Remove all-but-one use of WTF::String::operator+= from WebCore
Comment 1 Adam Barth 2012-08-30 16:20:08 PDT
Created attachment 161579 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-08-30 16:47:27 PDT
Comment on attachment 161579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161579&action=review

> Source/WebCore/inspector/InspectorStyleTextEditor.cpp:107
> +            textToSet.append(formatLineFeed);

It seems this function wants a StringBuilder.  Maybe needs a fixme.

> Source/WebCore/inspector/NetworkResourcesData.cpp:116
>      m_content = m_decoder->decode(m_dataBuffer->data(), m_dataBuffer->size());
> -    m_content += m_decoder->flush();
> +    m_content.append(m_decoder->flush());

It seems decoder wants a decodeAndFlush command. :)  Or a ShouldFlush enum (like I think it used to ahve).
Comment 3 Adam Barth 2012-08-30 16:52:56 PDT
(In reply to comment #2)
> (From update of attachment 161579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161579&action=review
> 
> > Source/WebCore/inspector/InspectorStyleTextEditor.cpp:107
> > +            textToSet.append(formatLineFeed);
> 
> It seems this function wants a StringBuilder.  Maybe needs a fixme.

Yeah, the problem is that it's doing a bunch of complex insert operations as well.  I can add a FIXME.
Comment 4 Benjamin Poulain 2012-08-30 18:32:20 PDT
Comment on attachment 161579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161579&action=review

I am only half convinced we should have String::append() at all.

This is an improvement in any case.

> Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp:109
> -        result.iterator->second += "\n" + String(value);
> +        result.iterator->second.append("\n" + String(value));

One of those ugly case where the verbose way is better:
    result.iterator->second.append = result.iterator->second.append + '\n' + String(value);

> Source/WebCore/platform/chromium/support/WebURLResponse.cpp:264
> -        result.iterator->second += ", " + valueStr;
> +        result.iterator->second.append(", " + valueStr);

The ugly case.

> Source/WebCore/xml/XMLHttpRequest.cpp:922
> -        result.iterator->second += ", " + value;
> +        result.iterator->second.append(", " + value);

Another case for
    result.iterator->second.append = result.iterator->second.append + ", " + value);

> Source/WebCore/xml/XPathFunctions.cpp:-565
> -    // FIXME: Building a String a character at a time is quite slow.
>      for (unsigned i1 = 0; i1 < s1.length(); ++i1) {
>          UChar ch = s1[i1];
>          size_t i2 = s2.find(ch);
>          
>          if (i2 == notFound)
> -            newString += String(&ch, 1);
> -        else if (i2 < s3.length()) {
> -            UChar c2 = s3[i2];
> -            newString += String(&c2, 1);
> -        }

:)
Comment 5 Adam Barth 2012-08-30 18:37:05 PDT
Thanks for the review.  I will uglify the code before landing.
Comment 6 Adam Barth 2012-08-30 18:37:13 PDT
:)
Comment 7 Darin Adler 2012-08-31 11:57:25 PDT
Comment on attachment 161579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161579&action=review

>> Source/WebCore/inspector/NetworkResourcesData.cpp:116
>> +    m_content.append(m_decoder->flush());
> 
> It seems decoder wants a decodeAndFlush command. :)  Or a ShouldFlush enum (like I think it used to ahve).

It would be good to have a “decode all at once” function that does the decode plus the flush. Simplistic uses of the decoder all want that. I don’t think adding an argument to decode is the way to do it, though. I’d prefer the new function.

Since flush almost always returns an empty string, there is not a significant performance issue caused by using append in the implementation, whether at these call sites or inside the helper function. Basically, the flush function will only return characters if there is a trailing partial character sequence. Not really that common.

>> Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp:109
>> +        result.iterator->second.append("\n" + String(value));
> 
> One of those ugly case where the verbose way is better:
>     result.iterator->second.append = result.iterator->second.append + '\n' + String(value);

With a some work, we could make the append function call style get the same performance as the verbose way. More template meta-programming work, but not too terribly difficult.
Comment 8 Adam Barth 2012-09-01 01:18:00 PDT
Ok.  I'm going to land with the prettier idiom for now because these are fairly rare cases.
Comment 9 Adam Barth 2012-09-01 01:19:27 PDT
Comment on attachment 161579 [details]
Patch

Clearing flags on attachment: 161579

Committed r127366: <http://trac.webkit.org/changeset/127366>
Comment 10 Adam Barth 2012-09-01 01:19:30 PDT
All reviewed patches have been landed.  Closing bug.