Changeset 127366 in webkit

Timestamp:

Sep 1, 2012 1:20:01 AM (12 years ago)

Author:

abarth@webkit.org

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• inspector/InspectorStyleTextEditor.cpp (modified) (1 diff)
• inspector/NetworkResourcesData.cpp (modified) (1 diff)
• loader/cache/CachedCSSStyleSheet.cpp (modified) (2 diffs)
• loader/cache/CachedFont.cpp (modified) (1 diff)
• loader/cache/CachedScript.cpp (modified) (1 diff)
• loader/cache/CachedXSLStyleSheet.cpp (modified) (1 diff)
• page/FrameTree.cpp (modified) (2 diffs)
• page/Page.cpp (modified) (1 diff)
• platform/chromium/support/WebHTTPLoadInfo.cpp (modified) (1 diff)
• platform/chromium/support/WebURLResponse.cpp (modified) (1 diff)
• xml/XMLHttpRequest.cpp (modified) (2 diffs)
• xml/XPathFunctions.cpp (modified) (2 diffs)
• xml/parser/XMLDocumentParser.cpp (modified) (2 diffs)
• xml/parser/XMLDocumentParser.h (modified) (1 diff)
• xml/parser/XMLDocumentParserLibxml2.cpp (modified) (1 diff)
• xml/parser/XMLDocumentParserQt.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/inspector/InspectorStyleTextEditor.cpp

@@ -105 +105 @@
         }
         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);

trunk/Source/WebCore/inspector/NetworkResourcesData.cpp

@@ -114 +114 @@
     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;

trunk/Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp

@@ -91 +91 @@
     // 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;
 }
@@ -105 +105 @@
     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);

trunk/Source/WebCore/loader/cache/CachedFont.cpp

@@ -138 +138 @@
         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);

trunk/Source/WebCore/loader/cache/CachedScript.cpp

@@ -72 +72 @@
     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());
     }

trunk/Source/WebCore/loader/cache/CachedXSLStyleSheet.cpp

@@ -70 +70 @@
         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);

trunk/Source/WebCore/page/FrameTree.cpp

@@ -31 +31 @@
 #include <wtf/Vector.h>
 #include <wtf/text/CString.h>
+#include <wtf/text/StringBuilder.h>
 
 using std::swap;
@@ -151 +152 @@
         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();
-    }
-
-    // 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;
-
-    return AtomicString(name);
+        name.append('/');
+        name.append(frame->tree()->uniqueName());
+    }
+
+    name.appendLiteral("/<!--frame");
+    name.append(String::number(childCount()));
+    name.appendLiteral("-->-->");
+
+    return name.toAtomicString();
 }
 

trunk/Source/WebCore/page/Page.cpp

@@ -838 +838 @@
     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;

trunk/Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp

@@ -107 +107 @@
     // 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));
 }
 

trunk/Source/WebCore/platform/chromium/support/WebURLResponse.cpp

@@ -262 +262 @@
         const_cast<HTTPHeaderMap*>(&map)->add(name, valueStr);
     if (!result.isNewEntry)
-        result.iterator->second += ", " + valueStr;
+        result.iterator->second.append(", " + valueStr);
 }
 

trunk/Source/WebCore/xml/XMLHttpRequest.cpp

@@ -645 +645 @@
         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);
         }
@@ -921 +920 @@
     HTTPHeaderMap::AddResult result = m_requestHeaders.add(name, value);
     if (!result.isNewEntry)
-        result.iterator->second += ", " + value;
+        result.iterator->second.append(", " + value);
 }
 

trunk/Source/WebCore/xml/XPathFunctions.cpp

@@ -551 +551 @@
     String s2 = arg(1)->evaluate().toString();
     String s3 = arg(2)->evaluate().toString();
-    String newString;
-
-    // FIXME: Building a String a character at a time is quite slow.
+    StringBuilder result;
+
     for (unsigned i1 = 0; i1 < s1.length(); ++i1) {
         UChar ch = s1[i1];
@@ -559 +558 @@
         
         if (i2 == notFound)
-            newString += String(&ch, 1);
-        else if (i2 < s3.length()) {
-            UChar c2 = s3[i2];
-            newString += String(&c2, 1);
-        }
-    }
-
-    return newString;
+            result.append(ch);
+        else if (i2 < s3.length())
+            result.append(s3[i2]);
+    }
+
+    return result.toString();
 }
 

trunk/Source/WebCore/xml/parser/XMLDocumentParser.cpp

@@ -113 +113 @@
 }
 
-void XMLDocumentParser::append(const SegmentedString& s)
-{
-    String parseString = s.toString();
-
+void XMLDocumentParser::append(const SegmentedString& source)
+{
     if (m_sawXSLTransform || !m_sawFirstElement)
-        m_originalSourceForTransform += parseString;
+        m_originalSourceForTransform.append(source);
 
     if (isStopped() || m_sawXSLTransform)
@@ -124 +122 @@
 
     if (m_parserPaused) {
-        m_pendingSrc.append(s);
-        return;
-    }
-
-    doWrite(parseString);
+        m_pendingSrc.append(source);
+        return;
+    }
+
+    doWrite(source.toString());
 
     // After parsing, go ahead and dispatch image beforeload events.

trunk/Source/WebCore/xml/parser/XMLDocumentParser.h

@@ -178 +178 @@
         FrameView* m_view;
 
-        String m_originalSourceForTransform;
+        SegmentedString m_originalSourceForTransform;
 
 #if USE(QXMLSTREAM)

trunk/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp

@@ -1333 +1333 @@
 
     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)));
 

trunk/Source/WebCore/xml/parser/XMLDocumentParserQt.cpp

@@ -202 +202 @@
 #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);