Bug 32710 – Newlines are drawn as missing-gyphs when using SVG fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Tor Arne Vestbø]

Due to not being replaced with spaces in the SVG font drawing path.

[Tor Arne Vestbø]

Created attachment 45148 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

[WebKit Review Bot]

Attachment 45148 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/Font.cpp:271:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1

[Tor Arne Vestbø]

Comment on attachment 45148 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

Clearing review on suspicion of the two updated tests being due to my setup.

[Tor Arne Vestbø]

Created attachment 45322 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

[Tor Arne Vestbø]

Suspicion was correct, my system has a font that the Safari buildbot does not, so the two tests had different actual results. The updated results are no longer part of the patch.

[WebKit Review Bot]

Attachment 45322 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/Font.cpp:288:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1

[Tor Arne Vestbø]

Created attachment 45323 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45323 [details] without any errors.

[Darin Adler]

Comment on attachment 45323 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

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

[Tor Arne Vestbø]

Created attachment 45383 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45383 [details] without any errors.

[Tor Arne Vestbø]

(In reply to comment #9)
> (From update of attachment 45323 [details])

Thank you for the thorough review and great feedback Darin, I really appreciate it! :)

Patch has been updated:

  - Remove const from local variables and helper method signatures
  - Rename helpers to normalizeSpaces(...)
  - Removed function-call-style cast for UChar32 to UChar conversion
  - Inlined the UChar version of normalizeSpaces, to match previous use in FontFastPath.cpp
  - Rewrote the String version of normalizeSpaces to use a Vector<UChar, 256> buffer
  
The number 256 was picked after surfing around a few sites and watching the typical length of these strings. If you feel the number is too low or too high, let me know and I'll update the patch. 

Thanks!

[Darin Adler]

Comment on attachment 45383 [details]
Fix for newlines turning into missing-gyphs when using SVG fonts

> +        if (character != space && treatAsSpace(character))
> +            return space;

The character != space check here seems to be redundant and adds no value. Even for spaces themselves, I don't think this is faster than just calling treatAsSpace, which checks for space already. And the most common case is non-spaces, which get slower due to this check.

> +        if (character != zeroWidthSpace && treatAsZeroWidthSpace(character))
> +            return zeroWidthSpace;

Same with the character != zeroWidthSpace check here.

r=me, but I think you should consider removing that extra code

[Tor Arne Vestbø]

Landed in r52525