[webkit-reviews] review denied: [Bug 60700] Replace direct StringConcatenate usage, by using operator+ (again) : [Attachment 93290] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 60700: Replace direct StringConcatenate usage, by using operator+ (again)
https://bugs.webkit.org/show_bug.cgi?id=60700

Attachment 93290: Patch
https://bugs.webkit.org/attachment.cgi?id=93290&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?