[Webkit-unassigned] [Bug 6274] text repainting does not account for glyphs which draw outside the typographic bounds of the font

[bugzilla-daemon at webkit.org]

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





--- Comment #33 from Enrica Casucci <enrica at apple.com>  2010-04-06 15:38:45 PST ---
(In reply to comment #30)
> (From update of attachment 52670 [details])
> > -const float cGlyphWidthUnknown = -1;
> > +const float cGlyphSizeUnknown = -1;
>
Ok. 
> NaN might be a better choice here.
> 
> >  #include "GlyphPageTreeNode.h"
> > -#include "GlyphWidthMap.h"
> > +#include "GlyphMetricsMap.h"
> 
> These are not in ASCII order now.
> 
Already fixed.

> > +    if ((metricsMode == GlyphWidthOnly && metrics.horizontalAdvance != cGlyphSizeUnknown) || (metricsMode == GlyphBoundingBox && metrics.boundingBox.width() != cGlyphSizeUnknown && metrics.boundingBox.height() != cGlyphSizeUnknown))
> > +        return metrics;
> 
> Checking bot width() and height() seems like an overkill.
>
Checking only for width.

> > -            width = fontData->widthForGlyph(glyph);
> > +            width = fontData->metricsForGlyph(glyph, GlyphWidthOnly).horizontalAdvance;
> 
> This change seems unnecessary. widthForGlyph does just what you want.
> 
> > +    if (metricsMode == GlyphBoundingBox) {
I don't see the advantage here. We still need to have two return statements.

> 
> I would write this as an early return.
> 
> > +    if (metricsMode == GlyphBoundingBox) {
> 
> Ditto.
> 
> The rendering part of the code needs to be reviewed by someone other than
> myself, since I wrote it.

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