Remove all-but-one use of WTF::String::operator+= from WebCore
Created attachment 161579 [details] Patch
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).
(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 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); > - } :)
Thanks for the review. I will uglify the code before landing.
:)
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.
Ok. I'm going to land with the prettier idiom for now because these are fairly rare cases.
Comment on attachment 161579 [details] Patch Clearing flags on attachment: 161579 Committed r127366: <http://trac.webkit.org/changeset/127366>
All reviewed patches have been landed. Closing bug.