Deploy emptyString() in dom, css, and editing
Created attachment 161147 [details] Patch
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, 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.
(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.
The obvious stuff is happening in https://bugs.webkit.org/show_bug.cgi?id=95304