Now that operator+ is fast for multiple string concats, deploy its usage everywhere. Previously I switched several places to using makeString() directly, revert that.
Created attachment 93290 [details] Patch With that patch, there's only one direct user of StringConcatenate.h remaining in WebCore: history/PageCache.cpp:#define PCLOG(...) pageCacheLog(pageCacheLogPrefix(indentLevel), makeString(__VA_ARGS__)) Everything else uses the new String append operator.
Comment on attachment 93290 [details] Patch Attachment 93290 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8690659
Comment on attachment 93290 [details] Patch Attachment 93290 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8662011
Comment on attachment 93290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93290&action=review Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c)); Two problems here. First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic. Second of all, if it did work, we’d be trading an optimized code path to append two character to a general purpose code path that has to call strlen just to get the length of a string literal. > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139 > - builder.append(makeString('-', toASCIILower(c))); > + builder.append("-" + toASCIILower(c)); Same problem as above. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), '#', String::number(profile->uid()), "\" finished."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + '#' + String::number(profile->uid()) + "\" finished."; The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code. Changing the '/' into a "/", adds an unneeded call to strlen into the mix. > Source/WebCore/inspector/InspectorProfilerAgent.cpp:113 > - String message = makeString("Profile \"webkit-profile://", CPUProfileType, '/', encodeWithURLEscapeSequences(title), "#0\" started."); > + String message = String("Profile \"webkit-profile://") + CPUProfileType + "/" + encodeWithURLEscapeSequences(title) + "#0\" started."; Same problem of allocating an extra string. We don’t want to convert things to String just so we can concatenate them and make the final result into a string. We make sure we preserve a way of doing it that does not require extra memory allocation. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > + String message; > + if (m_document->url().isNull()) > + message = "Unsafe attempt to load URL " + url.string() + '.'; > + else > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n"; The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if?
Comment on attachment 93290 [details] Patch Attachment 93290 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8685928
(In reply to comment #4) > (From update of attachment 93290 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93290&action=review > > Generally looks good. I suggest landing a patch that covers the simpler obviously-correct cases first, and then we can slowly consider the ones that require a little more thought. You are right. > > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:120 > First, this code won’t work. It will take the address of the "-" string, and then move forward by a number of characters equal the the lowercase character code. Pointer arithmetic. Ay ay ay, great catch. > > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:139 I'll leave out both files from the next patch. > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:105 > The new code wastefully converts the first literal into a String just so we can concatenate. This is less efficient than the old code. Right, I left it out for now. > > > Source/WebCore/inspector/InspectorProfilerAgent.cpp:113 Ditto. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:496 > > + String message; > > + if (m_document->url().isNull()) > > + message = "Unsafe attempt to load URL " + url.string() + '.'; > > + else > > + message = "Unsafe attempt to load URL " + url.string() + " from frame with URL " + m_document->url().string() + ". Domains, protocols and ports must match.\n"; > > The ? : avoids creating an empty object and then using the assignment operator, which is less efficient than just initializing. Was it necessary to change from ? : to if? If I want to use the ? operator, I have to assure both sides are Strings, so I have to use String("Unsafe...") + url.string()... Compared to that, I think the new solution is better, no?
Created attachment 93411 [details] Patch v2
Comment on attachment 93411 [details] Patch v2 Attachment 93411 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8690936 New failing tests: svg/dom/SVGTransformList-basics.xhtml
Created attachment 93414 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 93552 [details] Patch v3 Fix typo in patch v2, leading to cr-linux tests problem: + case SVG_TRANSFORM_SCALE: { - DEFINE_STATIC_LOCAL(String, scaleString, ("matrix(")); + DEFINE_STATIC_LOCAL(String, scaleString, ("scale("));
Comment on attachment 93552 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=93552&action=review r=me with some notes. > Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber); not ':'? > Source/WebCore/page/DOMWindow.cpp:1702 > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n"; don't we use indention with 4 spaces here? I like your style more, but we should be consistent. > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > + String key = "F" + String::number(i); 'F' instead of "F"? > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > + String key = "F" + String::number(i); Ditto.
(In reply to comment #11) > (From update of attachment 93552 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93552&action=review > > r=me with some notes. > > > Source/WebCore/bindings/js/ScriptDebugServer.cpp:83 > > + return sourceID + ":" + String::number(scriptBreakpoint.lineNumber); > > not ':'? > > > Source/WebCore/page/DOMWindow.cpp:1702 > > + " from frame with URL " + activeWindowURL.string() + ". Domains, protocols and ports must match.\n"; > > don't we use indention with 4 spaces here? I like your style more, but we should be consistent. Fixed. > > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:56 > > + String key = "F" + String::number(i); > > 'F' instead of "F"? > > > Source/WebCore/platform/efl/PlatformKeyboardEventEfl.cpp:164 > > + String key = "F" + String::number(i); > > Ditto. That doesn't build see efl build output from patch v1.
Landed in r86542 and r86543 (forgot one file).
This introduced a subtle issue, filed at: <http://webkit.org/b/69011> FTPDirectoryDocument Shows Garbled String for Last Modified Date