Bug 38407 - SVG hkern implementation incomplete
: SVG hkern implementation incomplete
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-30 15:21 PST by
Modified: 2010-05-02 09:30 PST (History)


Attachments
Patch (301.20 KB, patch)
2010-04-30 15:38 PST, Dirk Schulze
zimmermann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-30 15:21:01 PST
The hkern implementation is incomplete, nad partly wrong.
------- Comment #1 From 2010-04-30 15:38:13 PST -------
Created an attachment (id=54828) [details]
Patch
------- Comment #2 From 2010-04-30 15:40:00 PST -------
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.
------- Comment #3 From 2010-04-30 21:34:42 PST -------
(In reply to comment #2)
> Attachment 54828 [details] [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 #4 From 2010-05-01 10:09:05 PST -------
(From update of attachment 54828 [details])
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
------- Comment #5 From 2010-05-02 09:30:46 PST -------
Committed r58656: <http://trac.webkit.org/changeset/58656>