[Webkit-unassigned] [Bug 38407] SVG hkern implementation incomplete

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38407


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54828|review?                     |review+
               Flag|                            |




--- Comment #4 from Nikolas Zimmermann <zimmermann at kde.org>  2010-05-01 10:09:05 PST ---
(From update of attachment 54828)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.