[webkit-reviews] review granted: [Bug 38407] SVG hkern implementation incomplete : [Attachment 54828] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 38407: SVG hkern implementation incomplete
https://bugs.webkit.org/show_bug.cgi?id=38407

Attachment 54828: Patch
https://bugs.webkit.org/attachment.cgi?id=54828&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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