[webkit-reviews] review canceled: [Bug 95294] Deploy emptyString() in dom, css, and editing : [Attachment 161147] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has canceled Adam Barth
<abarth at webkit.org>'s request for review:
Bug 95294: Deploy emptyString() in dom, css, and editing
https://bugs.webkit.org/show_bug.cgi?id=95294

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

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


Adam, do you mind keeping this one on hold of a day of two? I would like to
make sure we are doing the best thing for emptyString().

> Source/WebCore/ChangeLog:9
> +	   emptyString() is more efficient than String("") because it saves
calls
> +	   to malloc. There are tons of uses of String("") in WebCore. This
patch

We actually have early return in StringImpl to make sure we do not allocate
anything in those cases.

Something to check is whether or not emptyString() generate less instructions
than String(""). 
String("") is likely around 64bits for the pointer + 1 function call.
emptyString() is one function call + the inline ref.

It is possible emptyString() is less compact than String(""). In which case we
should consider having an "empty string constructor".

> Source/WebCore/editing/HTMLInterchange.cpp:40
> +    DEFINE_STATIC_LOCAL(String, convertedSpaceString,
(String(ASCIILiteral("<span class=\"" AppleConvertedSpace "\">")) +
noBreakSpace + "</span>"));

Do you need?
String(ASCIILiteral("<span class=\"" AppleConvertedSpace "\">"))?

If
    "<span class=\"" AppleConvertedSpace "\">" + noBreakSpace + "</span>"
does not work, that may be a bug in StringOperators.