To repro, go to LayoutTests/svg/W3C-SVG-1.1/fonts-glyphs-02-t.svg. Note big ugly blank rectangles where none should exist. Simplified repro case is: <?xml version="1.0" encoding="UTF-8"?> <svg xmlns="http://www.w3.org/2000/svg" > <defs> <font horiz-adv-x="500"> <font-face font-family="SVGFont" /> <missing-glyph d="M0 500L250 750L500 500"/> <glyph unicode="ښ" arabic-form="initial" d="M0 500L250 0L500 500"/> <glyph unicode="ښ" arabic-form="medial" d="M0 500L250 0L500 500"/> <glyph unicode="ښ" arabic-form="terminal" d="M0 500L250 0L500 500"/> </font> </defs> <g font-family="SVGFont" font-size="80"> <text font-face="SVGFont" font-size="80" x="100" y="100">ښښښ</text> </g> </svg> This should output three downward-pointing triangles (one for each character in the <text> element) but it outputs two downward triangles with an upward triangle between them. Glyphs with arabic-form "medial" don't work. Here's the problem. SVGGlyphElement.h: struct SVGGlyphIdentifier { ... // SVG Font depends on exactly this order. enum ArabicForm { None = 0, Isolated, Terminal, Initial, Medial }; ... ArabicForm arabicForm : 3; ArabicForm has values 0 to 4 inclusive, so it looks like a 3-bit bitfield should be more than sufficient. But enumerations are signed types, so the bitfield is also signed, so rather than ranging from 0..7, it ranges from -4..3. You can verify this: void main() { enum foo { ZERO, ONE, TWO, THREE, FOUR }; struct bar { foo f : 3; } b; b.f = FOUR; cout << "FOUR = " << FOUR << endl; cout << "b.f = " << b.f << endl; cout << "FOUR == b.f = " << (FOUR==b.f) << endl; } ...which outputs: FOUR = 4 b.f = -4 FOUR == b.f = 0 The simplest solution is to bump the size of the bitfield up by one. This should have zero impact on performance, since the next data member should be byte-aligned (at least) anyway. Sure would be nice if the compiler warned of the potential integer overflow.
Created attachment 21233 [details] Dirt-simple patch
Comment on attachment 21233 [details] Dirt-simple patch Needs a test case. Otherwise this looks great!
Comment on attachment 21233 [details] Dirt-simple patch Oh, and actually, this is the wrong fix. There is a way to force unsigned enums. We have to do that for windows compilers. there is an rtl flag on RenderStyle which we had to do this for iirc. (Random Trivia: When the very very first versions of WebKit ran on windows the first time all the text was RTL due to this bug.)
Huh... learn something new every day. There is a way to force an enum to be unsigned, and the syntax is: enum foo : unsigned { one, two, three }; I had no idea. Will update patch. Will also start thinking of a test case, but the current SVG tree dumper doesn't dump anything wrong with the way things are, so it'll require changes (and hence changes to a lot of expected output.)
Alas, while enum something : unsigned { } is kind of neat, it appears to be a nonstandard language extension. Other places in the code just use unsigned as the bitfield type, guess that's what I'll do here.
Created attachment 21243 [details] Updated patch Rather than increasing the size of the bitfield, declared it as unsigned instead of ArabicForm. This matches how other enumerations in WebKit are placed in bitfields. As for a testcase, well, you can verify the bug's existence and fix manually by visiting LayoutTests/svg/W3C-SVG-1.1/fonts-glyph-03-t.svg. A testcase that fails automated tests without the patch would be a lot tougher... the SVG render tree dumper doesn't dump nearly enough information to cause a test case to fail, and would need to be changed substantially to make it do so.
Comment on attachment 21243 [details] Updated patch Ok. r=me.
Patch fails to compile as-is with a warning about signed/unsigned comparison. I'll fix it as I check it in.
Committed revision 34445.