Remove all-but-one use of WTF::String::operator+= from WebCore
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 1 Sep 2012 08:20:01 +0000 (08:20 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 1 Sep 2012 08:20:01 +0000 (08:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=95508

Reviewed by Benjamin Poulain.

This patch removes all the uses of WTF::String::operator+= from
WebCore, except those in WorkerScriptLoader (which need a more delicate
patch). There were actually a handful of legitimate use cases for += in
this group. I've replaced them with calls to String::append rather than
+= so that we can remove += and encourage most contributors to use
more efficient string idioms.

(There are likely some more uses in WebCore hiding in port-specific
code---this patch covers only those call sites found when compiling the
chromium-mac port.)

* inspector/InspectorStyleTextEditor.cpp:
(WebCore::InspectorStyleTextEditor::insertProperty):
    - This complicated function looks really inefficient, but I didn't
      have the heart to rewrite it.
* inspector/NetworkResourcesData.cpp:
(WebCore::NetworkResourcesData::ResourceData::decodeDataToContent):
* loader/cache/CachedCSSStyleSheet.cpp:
(WebCore::CachedCSSStyleSheet::sheetText):
(WebCore::CachedCSSStyleSheet::data):
* loader/cache/CachedFont.cpp:
(WebCore::CachedFont::ensureSVGFontData):
* loader/cache/CachedScript.cpp:
(WebCore::CachedScript::script):
* loader/cache/CachedXSLStyleSheet.cpp:
(WebCore::CachedXSLStyleSheet::data):
    - This decoder/flush pattern can probably be improved by combining
      the decode and flush operations, but I didn't do that in this
      patch.
* page/FrameTree.cpp:
(WebCore::FrameTree::uniqueChildName):
    - I found this code very amusing. It's worried enough about
      efficiency to give a big speech about why snprintf is safe, but
      then it implicitly performs a large number of mallocs and memcpy
      operations.
* page/Page.cpp:
(WebCore::Page::userStyleSheet):
* platform/chromium/support/WebHTTPLoadInfo.cpp:
(WebKit::addHeader):
* platform/chromium/support/WebURLResponse.cpp:
(WebKit::WebURLResponse::addHTTPHeaderField):
    - This header-appending idiom looks like a reasonable use case for
      String::append.
* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::send):
(WebCore::XMLHttpRequest::setRequestHeaderInternal):
* xml/XPathFunctions.cpp:
(WebCore::XPath::FunTranslate::evaluate):
    - Fixes 6 year old FIXME.
* xml/parser/XMLDocumentParser.cpp:
(WebCore::XMLDocumentParser::append):
* xml/parser/XMLDocumentParser.h:
(XMLDocumentParser):
* xml/parser/XMLDocumentParserLibxml2.cpp:
(WebCore::XMLDocumentParser::doEnd):
* xml/parser/XMLDocumentParserQt.cpp:
(WebCore::XMLDocumentParser::doEnd):
    - Changed m_originalSourceForTransform to a SegmentedString so that
      we don't need to malloc and copy the entire document every time
      we get more data from the network.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@127366 268f45cc-cd09-0410-ab3c-d52691b4dbfc

17 files changed:
Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorStyleTextEditor.cpp
Source/WebCore/inspector/NetworkResourcesData.cpp
Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp
Source/WebCore/loader/cache/CachedFont.cpp
Source/WebCore/loader/cache/CachedScript.cpp
Source/WebCore/loader/cache/CachedXSLStyleSheet.cpp
Source/WebCore/page/FrameTree.cpp
Source/WebCore/page/Page.cpp
Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp
Source/WebCore/platform/chromium/support/WebURLResponse.cpp
Source/WebCore/xml/XMLHttpRequest.cpp
Source/WebCore/xml/XPathFunctions.cpp
Source/WebCore/xml/parser/XMLDocumentParser.cpp
Source/WebCore/xml/parser/XMLDocumentParser.h
Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp
Source/WebCore/xml/parser/XMLDocumentParserQt.cpp

index 43e0c9d..0553d4e 100644 (file)
@@ -1,3 +1,71 @@
+2012-09-01  Adam Barth  <abarth@webkit.org>
+
+        Remove all-but-one use of WTF::String::operator+= from WebCore
+        https://bugs.webkit.org/show_bug.cgi?id=95508
+
+        Reviewed by Benjamin Poulain.
+
+        This patch removes all the uses of WTF::String::operator+= from
+        WebCore, except those in WorkerScriptLoader (which need a more delicate
+        patch). There were actually a handful of legitimate use cases for += in
+        this group. I've replaced them with calls to String::append rather than
+        += so that we can remove += and encourage most contributors to use
+        more efficient string idioms.
+
+        (There are likely some more uses in WebCore hiding in port-specific
+        code---this patch covers only those call sites found when compiling the
+        chromium-mac port.)
+
+        * inspector/InspectorStyleTextEditor.cpp:
+        (WebCore::InspectorStyleTextEditor::insertProperty):
+            - This complicated function looks really inefficient, but I didn't
+              have the heart to rewrite it.
+        * inspector/NetworkResourcesData.cpp:
+        (WebCore::NetworkResourcesData::ResourceData::decodeDataToContent):
+        * loader/cache/CachedCSSStyleSheet.cpp:
+        (WebCore::CachedCSSStyleSheet::sheetText):
+        (WebCore::CachedCSSStyleSheet::data):
+        * loader/cache/CachedFont.cpp:
+        (WebCore::CachedFont::ensureSVGFontData):
+        * loader/cache/CachedScript.cpp:
+        (WebCore::CachedScript::script):
+        * loader/cache/CachedXSLStyleSheet.cpp:
+        (WebCore::CachedXSLStyleSheet::data):
+            - This decoder/flush pattern can probably be improved by combining
+              the decode and flush operations, but I didn't do that in this
+              patch.
+        * page/FrameTree.cpp:
+        (WebCore::FrameTree::uniqueChildName):
+            - I found this code very amusing. It's worried enough about
+              efficiency to give a big speech about why snprintf is safe, but
+              then it implicitly performs a large number of mallocs and memcpy
+              operations.
+        * page/Page.cpp:
+        (WebCore::Page::userStyleSheet):
+        * platform/chromium/support/WebHTTPLoadInfo.cpp:
+        (WebKit::addHeader):
+        * platform/chromium/support/WebURLResponse.cpp:
+        (WebKit::WebURLResponse::addHTTPHeaderField):
+            - This header-appending idiom looks like a reasonable use case for
+              String::append.
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::send):
+        (WebCore::XMLHttpRequest::setRequestHeaderInternal):
+        * xml/XPathFunctions.cpp:
+        (WebCore::XPath::FunTranslate::evaluate):
+            - Fixes 6 year old FIXME.
+        * xml/parser/XMLDocumentParser.cpp:
+        (WebCore::XMLDocumentParser::append):
+        * xml/parser/XMLDocumentParser.h:
+        (XMLDocumentParser):
+        * xml/parser/XMLDocumentParserLibxml2.cpp:
+        (WebCore::XMLDocumentParser::doEnd):
+        * xml/parser/XMLDocumentParserQt.cpp:
+        (WebCore::XMLDocumentParser::doEnd):
+            - Changed m_originalSourceForTransform to a SegmentedString so that
+              we don't need to malloc and copy the entire document every time
+              we get more data from the network.
+
 2012-09-01  Tommy Widenflycht  <tommyw@google.com>
 
         MediaStream API: Add MediaStream management to RTCPeerConnection
index 7976a4a..059a650 100644 (file)
@@ -104,11 +104,11 @@ void InspectorStyleTextEditor::insertProperty(unsigned index, const String& prop
                 textToSet.insert(formatLineFeed, formattingPrependOffset);
         }
         if (!isHTMLLineBreak(m_styleText[propertyStart]))
-            textToSet += formatLineFeed;
+            textToSet.append(formatLineFeed);
     } else {
         String fullPrefix = formatLineFeed + formatPropertyPrefix;
         long fullPrefixLength = fullPrefix.length();
-        textToSet += fullPrefix;
+        textToSet.append(fullPrefix);
         if (insertFirstInSource && (propertyStart < fullPrefixLength || m_styleText.substring(propertyStart - fullPrefixLength, fullPrefixLength) != fullPrefix))
             textToSet.insert(fullPrefix, formattingPrependOffset);
     }
index e05d13d..4416adf 100644 (file)
@@ -113,7 +113,7 @@ size_t NetworkResourcesData::ResourceData::decodeDataToContent()
     ASSERT(!hasContent());
     size_t dataLength = m_dataBuffer->size();
     m_content = m_decoder->decode(m_dataBuffer->data(), m_dataBuffer->size());
-    m_content += m_decoder->flush();
+    m_content.append(m_decoder->flush());
     m_dataBuffer = nullptr;
     return contentSizeInBytes(m_content) - dataLength;
 }
index b2596cd..29da628 100644 (file)
@@ -90,7 +90,7 @@ const String CachedCSSStyleSheet::sheetText(bool enforceMIMEType, bool* hasValid
     
     // Don't cache the decoded text, regenerating is cheap and it can use quite a bit of memory
     String sheetText = m_decoder->decode(m_data->data(), m_data->size());
-    sheetText += m_decoder->flush();
+    sheetText.append(m_decoder->flush());
     return sheetText;
 }
 
@@ -104,7 +104,7 @@ void CachedCSSStyleSheet::data(PassRefPtr<SharedBuffer> data, bool allDataReceiv
     // Decode the data to find out the encoding and keep the sheet text around during checkNotify()
     if (m_data) {
         m_decodedSheetText = m_decoder->decode(m_data->data(), m_data->size());
-        m_decodedSheetText += m_decoder->flush();
+        m_decodedSheetText.append(m_decoder->flush());
     }
     setLoading(false);
     checkNotify();
index 27ffd95..72c7ca7 100644 (file)
@@ -137,7 +137,7 @@ bool CachedFont::ensureSVGFontData()
 
         RefPtr<TextResourceDecoder> decoder = TextResourceDecoder::create("application/xml");
         String svgSource = decoder->decode(m_data->data(), m_data->size());
-        svgSource += decoder->flush();
+        svgSource.append(decoder->flush());
         
         m_externalSVGDocument->setContent(svgSource);
         
index 12c9fe5..72264fa 100644 (file)
@@ -71,7 +71,7 @@ const String& CachedScript::script()
 
     if (!m_script && m_data) {
         m_script = m_decoder->decode(m_data->data(), encodedSize());
-        m_script += m_decoder->flush();
+        m_script.append(m_decoder->flush());
         setDecodedSize(m_script.sizeInBytes());
     }
     m_decodedDataDeletionTimer.startOneShot(0);
index 0e77161..77c51e8 100644 (file)
@@ -69,11 +69,11 @@ void CachedXSLStyleSheet::data(PassRefPtr<SharedBuffer> data, bool allDataReceiv
     if (!allDataReceived)
         return;
 
-    m_data = data;     
+    m_data = data;
     setEncodedSize(m_data.get() ? m_data->size() : 0);
     if (m_data.get()) {
-        m_sheet = String(m_decoder->decode(m_data->data(), encodedSize()));
-        m_sheet += m_decoder->flush();
+        m_sheet = m_decoder->decode(m_data->data(), encodedSize());
+        m_sheet.append(m_decoder->flush());
     }
     setLoading(false);
     checkNotify();
index 6378f3f..d624579 100644 (file)
@@ -30,6 +30,7 @@
 #include <wtf/StringExtras.h>
 #include <wtf/Vector.h>
 #include <wtf/text/CString.h>
+#include <wtf/text/StringBuilder.h>
 
 using std::swap;
 
@@ -150,29 +151,23 @@ AtomicString FrameTree::uniqueChildName(const AtomicString& requestedName) const
             break;
         chain.append(frame);
     }
-    String name;
-    name += framePathPrefix;
-    if (frame)
-        name += frame->tree()->uniqueName().string().substring(framePathPrefixLength,
-            frame->tree()->uniqueName().length() - framePathPrefixLength - framePathSuffixLength);
+    StringBuilder name;
+    name.append(framePathPrefix);
+    if (frame) {
+        name.append(frame->tree()->uniqueName().string().substring(framePathPrefixLength,
+            frame->tree()->uniqueName().length() - framePathPrefixLength - framePathSuffixLength));
+    }
     for (int i = chain.size() - 1; i >= 0; --i) {
         frame = chain[i];
-        name += "/";
-        name += frame->tree()->uniqueName();
+        name.append('/');
+        name.append(frame->tree()->uniqueName());
     }
 
-    // Suffix buffer has more than enough space for:
-    //     10 characters before the number
-    //     a number (20 digits for the largest 64-bit integer)
-    //     6 characters after the number
-    //     trailing null byte
-    // But we still use snprintf just to be extra-safe.
-    char suffix[40];
-    snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount());
-
-    name += suffix;
+    name.appendLiteral("/<!--frame");
+    name.append(String::number(childCount()));
+    name.appendLiteral("-->-->");
 
-    return AtomicString(name);
+    return name.toAtomicString();
 }
 
 inline Frame* FrameTree::scopedChild(unsigned index, TreeScope* scope) const
index b8b8356..2983808 100644 (file)
@@ -837,7 +837,7 @@ const String& Page::userStyleSheet() const
 
     RefPtr<TextResourceDecoder> decoder = TextResourceDecoder::create("text/css");
     m_userStyleSheet = decoder->decode(data->data(), data->size());
-    m_userStyleSheet += decoder->flush();
+    m_userStyleSheet.append(decoder->flush());
 
     return m_userStyleSheet;
 }
index 7762fd8..33eace1 100644 (file)
@@ -106,7 +106,7 @@ static void addHeader(HTTPHeaderMap* map, const WebString& name, const WebString
     HTTPHeaderMap::AddResult result = map->add(name, value);
     // It is important that values are separated by '\n', not comma, otherwise Set-Cookie header is not parseable.
     if (!result.isNewEntry)
-        result.iterator->second += "\n" + String(value);
+        result.iterator->second.append("\n" + String(value));
 }
 
 void WebHTTPLoadInfo::addRequestHeader(const WebString& name, const WebString& value)
index 1f9b968..9de3c01 100644 (file)
@@ -261,7 +261,7 @@ void WebURLResponse::addHTTPHeaderField(const WebString& name, const WebString&
     HTTPHeaderMap::AddResult result =
         const_cast<HTTPHeaderMap*>(&map)->add(name, valueStr);
     if (!result.isNewEntry)
-        result.iterator->second += ", " + valueStr;
+        result.iterator->second.append(", " + valueStr);
 }
 
 void WebURLResponse::clearHTTPHeaderField(const WebString& name)
index 111a649..8fb7ce0 100644 (file)
@@ -644,8 +644,7 @@ void XMLHttpRequest::send(DOMFormData* body, ExceptionCode& ec)
 
         String contentType = getRequestHeader("Content-Type");
         if (contentType.isEmpty()) {
-            contentType = "multipart/form-data; boundary=";
-            contentType += m_requestEntityBody->boundary().data();
+            contentType = makeString("multipart/form-data; boundary=", m_requestEntityBody->boundary().data());
             setRequestHeaderInternal("Content-Type", contentType);
         }
     }
@@ -920,7 +919,7 @@ void XMLHttpRequest::setRequestHeaderInternal(const AtomicString& name, const St
 {
     HTTPHeaderMap::AddResult result = m_requestHeaders.add(name, value);
     if (!result.isNewEntry)
-        result.iterator->second += ", " + value;
+        result.iterator->second.append(", " + value);
 }
 
 String XMLHttpRequest::getRequestHeader(const AtomicString& name) const
index 3c90e3b..3934eba 100644 (file)
@@ -550,22 +550,19 @@ Value FunTranslate::evaluate() const
     String s1 = arg(0)->evaluate().toString();
     String s2 = arg(1)->evaluate().toString();
     String s3 = arg(2)->evaluate().toString();
-    String newString;
+    StringBuilder result;
 
-    // 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);
-        }
+            result.append(ch);
+        else if (i2 < s3.length())
+            result.append(s3[i2]);
     }
 
-    return newString;
+    return result.toString();
 }
 
 Value FunBoolean::evaluate() const
index 5241f07..69ef6a5 100644 (file)
@@ -112,22 +112,20 @@ void XMLDocumentParser::insert(const SegmentedString&)
     ASSERT_NOT_REACHED();
 }
 
-void XMLDocumentParser::append(const SegmentedString& s)
+void XMLDocumentParser::append(const SegmentedString& source)
 {
-    String parseString = s.toString();
-
     if (m_sawXSLTransform || !m_sawFirstElement)
-        m_originalSourceForTransform += parseString;
+        m_originalSourceForTransform.append(source);
 
     if (isStopped() || m_sawXSLTransform)
         return;
 
     if (m_parserPaused) {
-        m_pendingSrc.append(s);
+        m_pendingSrc.append(source);
         return;
     }
 
-    doWrite(parseString);
+    doWrite(source.toString());
 
     // After parsing, go ahead and dispatch image beforeload events.
     ImageLoader::dispatchPendingBeforeLoadEvents();
index 2a7245e..e8d144b 100644 (file)
@@ -177,7 +177,7 @@ public:
 
         FrameView* m_view;
 
-        String m_originalSourceForTransform;
+        SegmentedString m_originalSourceForTransform;
 
 #if USE(QXMLSTREAM)
         QXmlStreamReader m_stream;
index 49a316e..208336e 100644 (file)
@@ -1332,7 +1332,7 @@ void XMLDocumentParser::doEnd()
         xmlTreeViewer.transformDocumentToTreeView();
 
     if (m_sawXSLTransform) {
-        void* doc = xmlDocPtrForString(document()->cachedResourceLoader(), m_originalSourceForTransform, document()->url().string());
+        void* doc = xmlDocPtrForString(document()->cachedResourceLoader(), m_originalSourceForTransform.toString(), document()->url().string());
         document()->setTransformSource(adoptPtr(new TransformSource(doc)));
 
         document()->setParsing(false); // Make the document think it's done, so it will apply XSL stylesheets.
index 6430acc..c0ba9ce 100644 (file)
@@ -201,7 +201,7 @@ void XMLDocumentParser::doEnd()
 {
 #if ENABLE(XSLT)
     if (m_sawXSLTransform) {
-        document()->setTransformSource(adoptPtr(new TransformSource(m_originalSourceForTransform)));
+        document()->setTransformSource(adoptPtr(new TransformSource(m_originalSourceForTransform.toString())));
         document()->setParsing(false); // Make the doc think it's done, so it will apply xsl sheets.
         document()->styleResolverChanged(RecalcStyleImmediately);
         document()->setParsing(true);