Bug 95508 – Remove all-but-one use of WTF::String::operator+= from WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Remove all-but-one use of WTF::String::operator+= from WebCore

[Adam Barth]

Created attachment 161579 [details]
Patch

[Eric Seidel (no email)]

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).

[Adam Barth]

(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.

[Benjamin Poulain]

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);
> -        }

:)

[Adam Barth]

Thanks for the review.  I will uglify the code before landing.

[Adam Barth]

:)

[Darin Adler]

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.

[Adam Barth]

Ok.  I'm going to land with the prettier idiom for now because these are fairly rare cases.

[Adam Barth]

Comment on attachment 161579 [details]
Patch

Clearing flags on attachment: 161579

Committed r127366: <http://trac.webkit.org/changeset/127366>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.