[webkit-reviews] review granted: [Bug 95291] Deploy ASCIILiteral and StringBuilder in more places in WebCore : [Attachment 161140] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 95291: Deploy ASCIILiteral and StringBuilder in more places in WebCore
https://bugs.webkit.org/show_bug.cgi?id=95291

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=161140&action=review


Looks great!

How did you fix all the string? Manually or you have made a smart script?

I have a few comments but they are nitpicking.

> Source/WebCore/css/CSSCanvasValue.cpp:47
> +    result.appendLiteral(")");

append(')') is faster (it is mostly inline IIRC)

You can also use the string operators (operator+). If you do not have a branch,
they do a better job than StringBuilder.

> Source/WebCore/css/CSSCrossfadeValue.cpp:96
> +    result.appendLiteral(")");

append(')')

> Source/WebCore/css/CSSPageRule.cpp:64
> +	       text.appendLiteral(" ");

append(' ')

> Source/WebCore/css/CSSSelector.cpp:527
> +	       str.append(prefix.string());

This is unfortunate. If that is useful elsewhere, we add
StringBuilder::append(AtomicString).

> Source/WebCore/css/CSSSelector.cpp:631
> +	       return tagHistoryText + " " + str.toString();

' '

> Source/WebCore/html/HTMLTitleElement.cpp:79
>      for (Node *n = firstChild(); n; n = n->nextSibling()) {
>	   if (n->isTextNode())
> -	       val += toText(n)->data();
> +	       result.append(toText(n)->data());
>      }

Wow, that was pretty bad.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:96
> +    if (!SQLiteStatement(*this, ASCIILiteral("PRAGMA temp_store =
MEMORY;")).executeCommand())

I have looked at our use of SQLiteStatement and I think we could make a
malloc-free version for many operations.
But that would be stupid not to use ASCIILiteral in the meantime. :)