Bug 95294 – Deploy emptyString() in dom, css, and editing

• Component: New Bugs
• Status: RESOLVED LATER

[Adam Barth]

Deploy emptyString() in dom, css, and editing

[Adam Barth]

Created attachment 161147 [details]
Patch

[Benjamin Poulain]

Comment on attachment 161147 [details]
Patch

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.

[Adam Barth]

> 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().

Sure.  I'm in no rush.  Do you want me to split out the non-emptyString parts into a separate patch?

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

Yes.  It's needed to make the type system happy.

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

I didn't look up the type of noBreakSpace, but StringOperations didn't like it.

[Benjamin Poulain]

(In reply to comment #3)
> > 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().
> 
> Sure.  I'm in no rush.  Do you want me to split out the non-emptyString parts into a separate patch?

Splitting is a good idea, the other parts are pretty straightforward.

> > Do you need?
> > String(ASCIILiteral("<span class=\"" AppleConvertedSpace "\">"))?
> 
> Yes.  It's needed to make the type system happy.
> 
> > If
> >     "<span class=\"" AppleConvertedSpace "\">" + noBreakSpace + "</span>"
> > does not work, that may be a bug in StringOperators.
> 
> I didn't look up the type of noBreakSpace, but StringOperations didn't like it.

Your change is already an improvement over the existing code. Let's keep it.

I opened https://bugs.webkit.org/show_bug.cgi?id=95301 for myself to check why operator+ does not mach.

[Adam Barth]

The obvious stuff is happening in https://bugs.webkit.org/show_bug.cgi?id=95304