[webkit-reviews] review denied: [Bug 32710] Newlines are drawn as missing-gyphs when using SVG fonts : [Attachment 45323] Fix for newlines turning into missing-gyphs when using SVG fonts

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tor Arne Vestbø <vestbo at webkit.org>'s
request for review:
Bug 32710: Newlines are drawn as missing-gyphs when using SVG fonts
https://bugs.webkit.org/show_bug.cgi?id=32710

Attachment 45323: Fix for newlines turning into missing-gyphs when using SVG
fonts
https://bugs.webkit.org/attachment.cgi?id=45323&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Seems like a good idea to refactor this so we can share code.

> +const UChar Font::sanitizeSpaces(UChar character)

The const here in the type of the return value is not needed or helpful.

> +const String Font::sanitizeSpaces(const String& string)

Ditto.

I think I'd call this "normalization" rather than "sanitization". Or perhaps
something else.

> -	   UChar c16 = c;
> -	   if (Font::treatAsSpace(c16))
> -	       codeUnits[0] = ' ';
> -	   else if (Font::treatAsZeroWidthSpace(c16))
> -	       codeUnits[0] = zeroWidthSpace;
> -	   else
> -	       codeUnits[0] = c16;
> +	   codeUnits[0] = Font::sanitizeSpaces(UChar(c));
>	   codeUnitsLength = 1;

The old code here in the body of this loop was inlined. The new code involves a
function call. Could this have an undesirable performance cost? Or maybe not
inlining will make things faster?

The old code used assignment to truncate the UChar32 and change it to a UChar.
The new code uses a C++ function-call-style cast. We try to avoid those. I'd
like to see this either 1) use a local variable as before, 2) use no cast at
all if the compilers don't warn or give errors, or 3) use static_cast if a type
conversion is required.

> +	   const UChar originalCharacter = string[i];

We normally do not use const for local variables. There are many that could be
marked const, but we don't feel the extra keyword adds much clarity. All it
means is that this particular variable doesn't get assigned a new value.

> +	   const UChar possibleReplacement = sanitizeSpaces(originalCharacter);


Ditto.

> +	   if (possibleReplacement != originalCharacter)
> +	       possiblyDetatched.replace(i, 1, String(&possibleReplacement,
1)); // Detach

This is a quite inefficient way to change a character. Allocating a new string
just so you can use the replace function is poor. And also, if you end up
replacing many characters you'll end up allocating a new StringImpl every time.
If the string has a lot of characters that need to be changed, then you could
end up with an extremely slow algorithm with tons of allocations.

The right way to do this is to create some kind of temporary buffer, either a
Vector<UChar> or a Vector<UChar, 1000> where 1000 is the amount of stack space
you want to use. Then you do the replacement in that buffer and create a string
once. This keeps the number of allocations small. If you use Vector<UChar> then
you can use the String::adopt function to create the new string. If you use a
Vector<UChar, 1000> then you can avoid any allocation at all if the string
happens to be small enough to fit into the stack buffer, and the only
allocation done will be the StringImpl which will be large enough to hold all
the characters in the string.

A function that uses this idiom is deprecatedParseURL in
WebCore/css/CSSHelper.cpp, although that function does not have a special case
for the case where you can use the existing string as-is, and it should.

I think the name "possiblyDetached" is a confusing one. I understand that
you're trying to put an emphasis on the fact that we might end up with a new
string, but to me it seems like an oblique name for this local variable. But if
you rewrite this to be efficient then it probably won't exist any more.

> +	   const String text = Font::sanitizeSpaces(String(run.data(from),
run.length()));

Same comment as above about const in declaration of local variables.

review- because of the extremely slow algorithm in the String version of the
sanitizeSpaces function