Replace uses of WTF::String::operator+= with StringBuilder
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Aug 2012 08:20:03 +0000 (08:20 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Aug 2012 08:20:03 +0000 (08:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=95416

Reviewed by Benjamin Poulain.

WTF::String::operator+= appears to be a sandtrap for contributors. The
vast majority of the callers are using very inefficient string
patterns. This patch removes the use of operator+= in favor of
StringBuilder. Eventually, I'd like to remove operator+= so that more
code doesn't fall into this trap.

* Modules/websockets/WebSocketHandshake.cpp:
(WebCore::resourceName):
* html/HTMLAnchorElement.cpp:
(WebCore::appendServerMapMousePosition):
(WebCore::HTMLAnchorElement::handleClick):
* html/canvas/CanvasRenderingContext2D.cpp:
(WebCore::CanvasRenderingContext2D::font):
* platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::addHTTPHeaderField):
* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::nameForLayer):
* rendering/RenderTreeAsText.cpp:
(WebCore::RenderTreeAsText::writeRenderObject):
(WebCore::nodePosition):
* rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::setContent):

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/websockets/WebSocketHandshake.cpp
Source/WebCore/html/HTMLAnchorElement.cpp
Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp
Source/WebCore/platform/network/ResourceRequestBase.cpp
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderTreeAsText.cpp
Source/WebCore/rendering/style/RenderStyle.cpp

index 3d8266b..84c3fdf 100644 (file)
@@ -1,3 +1,33 @@
+2012-08-30  Adam Barth  <abarth@webkit.org>
+
+        Replace uses of WTF::String::operator+= with StringBuilder
+        https://bugs.webkit.org/show_bug.cgi?id=95416
+
+        Reviewed by Benjamin Poulain.
+
+        WTF::String::operator+= appears to be a sandtrap for contributors. The
+        vast majority of the callers are using very inefficient string
+        patterns. This patch removes the use of operator+= in favor of
+        StringBuilder. Eventually, I'd like to remove operator+= so that more
+        code doesn't fall into this trap.
+
+        * Modules/websockets/WebSocketHandshake.cpp:
+        (WebCore::resourceName):
+        * html/HTMLAnchorElement.cpp:
+        (WebCore::appendServerMapMousePosition):
+        (WebCore::HTMLAnchorElement::handleClick):
+        * html/canvas/CanvasRenderingContext2D.cpp:
+        (WebCore::CanvasRenderingContext2D::font):
+        * platform/network/ResourceRequestBase.cpp:
+        (WebCore::ResourceRequestBase::addHTTPHeaderField):
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::nameForLayer):
+        * rendering/RenderTreeAsText.cpp:
+        (WebCore::RenderTreeAsText::writeRenderObject):
+        (WebCore::nodePosition):
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::setContent):
+
 2012-08-30  Shinya Kawanaka  <shinyak@chromium.org>
 
         AuthorShadowDOM support for textarea element.
index caddb9b..1ba15fb 100644 (file)
@@ -64,14 +64,18 @@ static const char randomCharacterInSecWebSocketKey[] = "!\"#$%&'()*+,-./:;<=>?@A
 
 static String resourceName(const KURL& url)
 {
-    String name = url.path();
+    StringBuilder name;
+    name.append(url.path());
     if (name.isEmpty())
-        name = "/";
-    if (!url.query().isNull())
-        name += "?" + url.query();
-    ASSERT(!name.isEmpty());
-    ASSERT(!name.contains(' '));
-    return name;
+        name.append('/');
+    if (!url.query().isNull()) {
+        name.append('?');
+        name.append(url.query());
+    }
+    String result = name.toString();
+    ASSERT(!result.isEmpty());
+    ASSERT(!result.contains(' '));
+    return result;
 }
 
 static String hostName(const KURL& url, bool secure)
index 6477558..c3299fc 100644 (file)
@@ -41,6 +41,7 @@
 #include "SecurityOrigin.h"
 #include "SecurityPolicy.h"
 #include "Settings.h"
+#include <wtf/text/StringBuilder.h>
 
 namespace WebCore {
 
@@ -120,7 +121,7 @@ bool HTMLAnchorElement::isKeyboardFocusable(KeyboardEvent* event) const
     return hasNonEmptyBoundingBox();
 }
 
-static void appendServerMapMousePosition(String& url, Event* event)
+static void appendServerMapMousePosition(StringBuilder& url, Event* event)
 {
     if (!event->isMouseEvent())
         return;
@@ -143,10 +144,10 @@ static void appendServerMapMousePosition(String& url, Event* event)
     FloatPoint absolutePosition = renderer->absoluteToLocal(FloatPoint(static_cast<MouseEvent*>(event)->pageX(), static_cast<MouseEvent*>(event)->pageY()));
     int x = absolutePosition.x();
     int y = absolutePosition.y();
-    url += "?";
-    url += String::number(x);
-    url += ",";
-    url += String::number(y);
+    url.append('?');
+    url.append(String::number(x));
+    url.append(',');
+    url.append(String::number(y));
 }
 
 void HTMLAnchorElement::defaultEventHandler(Event* event)
@@ -507,9 +508,10 @@ void HTMLAnchorElement::handleClick(Event* event)
     if (!frame)
         return;
 
-    String url = stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr));
+    StringBuilder url;
+    url.append(stripLeadingAndTrailingHTMLSpaces(fastGetAttribute(hrefAttr)));
     appendServerMapMousePosition(url, event);
-    KURL kurl = document()->completeURL(url);
+    KURL kurl = document()->completeURL(url.toString());
 
 #if ENABLE(DOWNLOAD_ATTRIBUTE)
     if (hasAttribute(downloadAttr)) {
index c6b7732..a960489 100644 (file)
@@ -72,6 +72,7 @@
 #include <wtf/OwnPtr.h>
 #include <wtf/Uint8ClampedArray.h>
 #include <wtf/UnusedParam.h>
+#include <wtf/text/StringBuilder.h>
 
 #if USE(CG)
 #include <ApplicationServices/ApplicationServices.h>
@@ -1992,31 +1993,34 @@ String CanvasRenderingContext2D::font() const
     if (!state().m_realizedFont)
         return defaultFont;
 
-    String serializedFont;
+    StringBuilder serializedFont;
     const FontDescription& fontDescription = state().m_font.fontDescription();
 
     if (fontDescription.italic())
-        serializedFont += "italic ";
+        serializedFont.appendLiteral("italic ");
     if (fontDescription.smallCaps() == FontSmallCapsOn)
-        serializedFont += "small-caps ";
+        serializedFont.appendLiteral("small-caps ");
 
-    serializedFont += String::number(fontDescription.computedPixelSize()) + "px";
+    serializedFont.append(String::number(fontDescription.computedPixelSize()));
+    serializedFont.appendLiteral("px");
 
     const FontFamily& firstFontFamily = fontDescription.family();
     for (const FontFamily* fontFamily = &firstFontFamily; fontFamily; fontFamily = fontFamily->next()) {
         if (fontFamily != &firstFontFamily)
-            serializedFont += ",";
+            serializedFont.append(',');
 
+        // FIXME: We should append family directly to serializedFont rather than building a temporary string.
         String family = fontFamily->family();
         if (family.startsWith("-webkit-"))
             family = family.substring(8);
         if (family.contains(' '))
             family = makeString('"', family, '"');
 
-        serializedFont += " " + family;
+        serializedFont.append(' ');
+        serializedFont.append(family);
     }
 
-    return serializedFont;
+    return serializedFont.toString();
 }
 
 void CanvasRenderingContext2D::setFont(const String& newFont)
index 64ebc18..26cb80c 100644 (file)
@@ -374,7 +374,7 @@ void ResourceRequestBase::addHTTPHeaderField(const AtomicString& name, const Str
     updateResourceRequest();
     HTTPHeaderMap::AddResult result = m_httpHeaderFields.add(name, value);
     if (!result.isNewEntry)
-        result.iterator->second += "," + value;
+        result.iterator->second = result.iterator->second + ',' + value;
 
     if (url().protocolIsInHTTPFamily())
         m_platformRequestUpdated = false;
index d26eef1..518cc29 100644 (file)
@@ -55,8 +55,8 @@
 #include "Settings.h"
 #include "StyleResolver.h"
 #include "TiledBacking.h"
-
 #include <wtf/CurrentTime.h>
+#include <wtf/text/StringBuilder.h>
 
 #if ENABLE(CSS_FILTERS)
 #include "FilterEffectRenderer.h"
@@ -1710,29 +1710,35 @@ AnimatedPropertyID RenderLayerBacking::cssToGraphicsLayerProperty(CSSPropertyID
 
 String RenderLayerBacking::nameForLayer() const
 {
-    String name = renderer()->renderName();
+    StringBuilder name;
+    name.append(renderer()->renderName());
     if (Node* node = renderer()->node()) {
-        if (node->isElementNode())
-            name += " " + static_cast<Element*>(node)->tagName();
-        if (node->hasID())
-            name += " id=\'" + static_cast<Element*>(node)->getIdAttribute() + "\'";
+        if (node->isElementNode()) {
+            name.append(' ');
+            name.append(static_cast<Element*>(node)->tagName());
+        }
+        if (node->hasID()) {
+            name.appendLiteral(" id=\'");
+            name.append(static_cast<Element*>(node)->getIdAttribute());
+            name.append('\'');
+        }
 
         if (node->hasClass()) {
+            name.appendLiteral(" class=\'");
             StyledElement* styledElement = static_cast<StyledElement*>(node);
-            String classes;
             for (size_t i = 0; i < styledElement->classNames().size(); ++i) {
                 if (i > 0)
-                    classes += " ";
-                classes += styledElement->classNames()[i];
+                    name.append(' ');
+                name.append(styledElement->classNames()[i]);
             }
-            name += " class=\'" + classes + "\'";
+            name.append('\'');
         }
     }
 
     if (m_owningLayer->isReflection())
-        name += " (reflection)";
+        name.appendLiteral(" (reflection)");
 
-    return name;
+    return name.toString();
 }
 
 CompositingLayerType RenderLayerBacking::compositingLayerType() const
index c1585b1..b243872 100644 (file)
@@ -445,14 +445,14 @@ void RenderTreeAsText::writeRenderObject(TextStream& ts, const RenderObject& o,
                 ts << " id=\"" + static_cast<Element*>(node)->getIdAttribute() + "\"";
 
             if (node->hasClass()) {
+                ts << " class=\"";
                 StyledElement* styledElement = static_cast<StyledElement*>(node);
-                String classes;
                 for (size_t i = 0; i < styledElement->classNames().size(); ++i) {
                     if (i > 0)
-                        classes += " ";
-                    classes += styledElement->classNames()[i];
+                        ts << " ";
+                    ts << styledElement->classNames()[i];
                 }
-                ts << " class=\"" + classes + "\"";
+                ts << "\"";
             }
         }
     }
@@ -789,29 +789,36 @@ static void writeLayers(TextStream& ts, const RenderLayer* rootLayer, RenderLaye
 
 static String nodePosition(Node* node)
 {
-    String result;
+    StringBuilder result;
 
     Element* body = node->document()->body();
     Node* parent;
     for (Node* n = node; n; n = parent) {
         parent = n->parentOrHostNode();
         if (n != node)
-            result += " of ";
+            result.appendLiteral(" of ");
         if (parent) {
             if (body && n == body) {
                 // We don't care what offset body may be in the document.
-                result += "body";
+                result.appendLiteral("body");
                 break;
             }
-            if (n->isShadowRoot())
-                result += "{" + getTagName(n) + "}";
-            else
-                result += "child " + String::number(n->nodeIndex()) + " {" + getTagName(n) + "}";
+            if (n->isShadowRoot()) {
+                result.append('{');
+                result.append(getTagName(n));
+                result.append('}');
+            } else {
+                result.appendLiteral("child ");
+                result.append(String::number(n->nodeIndex()));
+                result.appendLiteral(" {");
+                result.append(getTagName(n));
+                result.append('}');
+            }
         } else
-            result += "document";
+            result.appendLiteral("document");
     }
 
-    return result;
+    return result.toString();
 }
 
 static void writeSelection(TextStream& ts, const RenderObject* o)
index a3cd02e..62343ca 100644 (file)
@@ -778,9 +778,7 @@ void RenderStyle::setContent(const String& string, bool add)
             // We attempt to merge with the last ContentData if possible.
             if (lastContent->isText()) {
                 TextContentData* textContent = static_cast<TextContentData*>(lastContent);
-                String text = textContent->text();
-                text += string;
-                textContent->setText(text);
+                textContent->setText(textContent->text() + string);
             } else
                 lastContent->setNext(ContentData::create(string));