Text laid out with the SVG -> OTF font converter does not have the same metrics as with the SVG font code path
Created attachment 238287 [details] Patch
Comment on attachment 238287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238287&action=review > Source/WebCore/svg/SVGToOTFFontConversion.cpp:221 > + // If the font parser sees an ascent or descent of 0, it thinks that something > + // bad happened and takes a different path. We want it to believe us. > + if (!ascent) > + ascent = 1; > + if (!descent) > + descent = 1; Nothing in this comment makes it clear why “1” is a suitable value to use instead of 0. Also “want it to believe us” is a vague comment. It would be more useful to say something brief and clear about how specifying a 1 FUnit value gives us the behavior we want. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:226 > + // WebKit's SVG codepath hardcodes the line gap to be 1/10th of the font size (see SVGFontData.cpp). We should match that. This comment is missing the “why”. Why should we match that? > Source/WebCore/svg/SVGToOTFFontConversion.cpp:683 > + if (!advanceAttribute.isEmpty()) { Why the special case for the empty string? The code would do the right thing without a special case and I’d like this code better without a local variable for advanceAttribute. > Source/WebCore/svg/SVGToOTFFontConversion.cpp:702 > auto& advanceAttribute = glyph.fastGetAttribute(SVGNames::horiz_adv_xAttr); This code has the same issues as the copied/pasted version of it that I commented on above.
Comment on attachment 238287 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238287&action=review >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:221 >> + descent = 1; > > Nothing in this comment makes it clear why “1” is a suitable value to use instead of 0. Also “want it to believe us” is a vague comment. It would be more useful to say something brief and clear about how specifying a 1 FUnit value gives us the behavior we want. Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:226 >> + // WebKit's SVG codepath hardcodes the line gap to be 1/10th of the font size (see SVGFontData.cpp). We should match that. > > This comment is missing the “why”. Why should we match that? Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:683 >> + if (!advanceAttribute.isEmpty()) { > > Why the special case for the empty string? The code would do the right thing without a special case and I’d like this code better without a local variable for advanceAttribute. Done. >> Source/WebCore/svg/SVGToOTFFontConversion.cpp:702 >> auto& advanceAttribute = glyph.fastGetAttribute(SVGNames::horiz_adv_xAttr); > > This code has the same issues as the copied/pasted version of it that I commented on above. Done.
http://trac.webkit.org/changeset/173739