RESOLVED FIXED 95508
Remove all-but-one use of WTF::String::operator+= from WebCore
https://bugs.webkit.org/show_bug.cgi?id=95508
Summary Remove all-but-one use of WTF::String::operator+= from WebCore
Adam Barth
Reported 2012-08-30 16:11:49 PDT
Remove all-but-one use of WTF::String::operator+= from WebCore
Attachments
Patch (18.62 KB, patch)
2012-08-30 16:20 PDT, Adam Barth
no flags
Adam Barth
Comment 1 2012-08-30 16:20:08 PDT
Eric Seidel (no email)
Comment 2 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).
Adam Barth
Comment 3 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.
Benjamin Poulain
Comment 4 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); > - } :)
Adam Barth
Comment 5 2012-08-30 18:37:05 PDT
Thanks for the review. I will uglify the code before landing.
Adam Barth
Comment 6 2012-08-30 18:37:13 PDT
:)
Darin Adler
Comment 7 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.
Adam Barth
Comment 8 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.
Adam Barth
Comment 9 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>
Adam Barth
Comment 10 2012-09-01 01:19:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.