The hkern implementation is incomplete, nad partly wrong.
Created attachment 54828 [details] Patch
Attachment 54828 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/rendering/SVGRootInlineBox.cpp:359: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/rendering/SVGRootInlineBox.cpp:1346: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 54828 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" > exit_code: 1 > WebCore/rendering/SVGRootInlineBox.cpp:359: Tests for true/false, > null/non-null, and zero/non-zero should all be done without equality > comparisons. [readability/comparison_to_zero] [5] > WebCore/rendering/SVGRootInlineBox.cpp:1346: Tests for true/false, > null/non-null, and zero/non-zero should all be done without equality > comparisons. [readability/comparison_to_zero] [5] > Total errors found: 2 in 20 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. The explicite null check is needed here.
Comment on attachment 54828 [details] Patch Hi Dirk, patch looks great! Some comments: > > - if (info.nextDrawnSeperated || spacing != 0.0f) { > + // FIXME: SVG Kerning get's not applied on texts on path. "doesn't get" not "get's" ;-) > + UnicodeRanges::const_iterator end = ranges.end(); Make it const, to emphazise you're not changing it. I'd always use that construct "const .. const_iterator end = foo.end();". > -bool SVGFontElement::getHorizontalKerningPairForStringsAndGlyphs(const String& u1, const String& g1, const String& u2, const String& g2, SVGHorizontalKerningPair& kerningPair) const > +float SVGFontElement::getHorizontalKerningPairForStringsAndGlyphs(const String& u1, const String& g1, const String& u2, const String& g2) const > + KerningPairVector::iterator it = m_kerningPairs.end() - 1; > + KerningPairVector::iterator begin = m_kerningPairs.begin() - 1; > + for (; it != begin; --it) { > + if (matches(u1, g1, u2, g2, *it)) > + return it->kerning; matches takes a "const SVGHorizontalKerningPair&", so you should use const_iterators here as well. a "const KerningPairVector::const_iterator begin = m_kerningPars.begin() - 1;" etc.. > + parseGlyphName(getAttribute(g1Attr), kerningPair.glyphName1); > + parseGlyphName(getAttribute(g2Attr), kerningPair.glyphName2); > + parseKerningUnicodeString(getAttribute(u1Attr), kerningPair.unicodeRange1, kerningPair.unicodeName1); > + parseKerningUnicodeString(getAttribute(u2Attr), kerningPair.unicodeRange2, kerningPair.unicodeName2); > + kerningPair.kerning = getAttribute(kAttr).string().toFloat(); We need to think about error handling here, please add a FIXME here! That needs to be adressed in a follow-up patch... I don't think we want to create kerning pairs, if there's an error in the parsing above. The parser changes look great to me! I think it's safe to land, after you've fixed the issues above. Have a nice weekend, Niko
Committed r58656: <http://trac.webkit.org/changeset/58656>