[webkit-reviews] review granted: [Bug 21853] text drawing on WX port on OSX is a few pixels off : [Attachment 24639] Use CGFont API for better metrics and fix y offset of text

[bugzilla-daemon at webkit.org]

Kevin Ollivier <kevino at theolliviers.com> has granted Kevin Watters
<kevinwatters at gmail.com>'s request for review:
Bug 21853: text drawing on WX port on OSX is a few pixels off
https://bugs.webkit.org/show_bug.cgi?id=21853

Attachment 24639: Use CGFont API for better metrics and fix y offset of text
https://bugs.webkit.org/attachment.cgi?id=24639&action=edit

------- Additional Comments from Kevin Ollivier <kevino at theolliviers.com>
> Index: WebCore/platform/wx/wxcode/mac/carbon/non-kerned-drawing.cpp
> ===================================================================
> --- WebCore/platform/wx/wxcode/mac/carbon/non-kerned-drawing.cpp     
(revision 37839)
> +++ WebCore/platform/wx/wxcode/mac/carbon/non-kerned-drawing.cpp     
(working copy)
> @@ -57,11 +57,10 @@
>	   offset += glyphBuffer.advanceAt(from + i);
>      }
>      
> -    // the y point is actually the bottom point of the text, turn it into
the top
> -    float height = font->ascent() - font->descent();
> -    wxCoord ypoint = (wxCoord) (point.y() - height);
> -	
> -    dc->DrawText(text, (wxCoord)point.x(), ypoint);
> +    // NOTE: The wx API actually adds the ascent to the y value internally,
> +    // so we have to subtract it from the y point here so that the ascent
> +    // isn't added twice. 
> +    dc->DrawText(text, (wxCoord)point.x(), int(point.y() - font->ascent()));

>  }
>  
>  }
> Index: WebCore/platform/wx/wxcode/mac/carbon/fontprops.cpp
> ===================================================================
> --- WebCore/platform/wx/wxcode/mac/carbon/fontprops.cpp	(revision
37839)
> +++ WebCore/platform/wx/wxcode/mac/carbon/fontprops.cpp	(working copy)
> @@ -23,9 +23,7 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#if __WXMAC__
>  #include <ApplicationServices/ApplicationServices.h>
> -#endif
>  
>  #include <wx/defs.h>
>  #include <wx/gdicmn.h>
> @@ -39,23 +37,38 @@
>  m_ascent(0), m_descent(0), m_lineGap(0), m_lineSpacing(0), m_xHeight(0)
>  {
>      ATSFontRef fontRef;
> +    CGFontRef cgFont;
>      
>      fontRef = FMGetATSFontRefFromFont(font->MacGetATSUFontID());
> -    if (fontRef){
> -	   ATSFontMetrics vMetrics;
> -	   OSStatus err;
> +    
> +    if (fontRef)
> +	   cgFont = CGFontCreateWithPlatformFont((void*)&fontRef);
>	   
> -	   int height = font->GetPointSize(); //.GetHeight();
> -	   err = ATSFontGetHorizontalMetrics(fontRef, 0, &vMetrics);
> -	   if (err != noErr)
> -	       fprintf(stderr, "Error number is %d\n", err);
> -	   m_ascent = lroundf(vMetrics.ascent * height);
> -	   m_descent = lroundf(vMetrics.descent * height);
> -	   m_lineGap = lroundf(vMetrics.leading * height);
> +    if (cgFont) {
> +	   int iAscent;
> +	   int iDescent;
> +	   int iLineGap;
> +	   float unitsPerEm;
> +#ifdef BUILDING_ON_TIGER
> +	   wkGetFontMetrics(cgFont, &iAscent, &iDescent, &iLineGap,
&unitsPerEm);
> +#else
> +	   iAscent = CGFontGetAscent(cgFont);
> +	   iDescent = CGFontGetDescent(cgFont);
> +	   iLineGap = CGFontGetLeading(cgFont);
> +	   unitsPerEm = CGFontGetUnitsPerEm(cgFont);
> +#endif
> +	   float pointSize = font->GetPointSize();
> +	   float fAscent = scaleEmToUnits(iAscent, unitsPerEm) * pointSize;
> +	   float fDescent = -scaleEmToUnits(iDescent, unitsPerEm) * pointSize;
> +	   float fLineGap = scaleEmToUnits(iLineGap, unitsPerEm) * pointSize;
> +
> +	   m_ascent = lroundf(fAscent);
> +	   m_descent = lroundf(fDescent);
> +	   m_lineGap = lroundf(fLineGap);
>	   wxCoord xHeight = 0;
>	   GetTextExtent(*font, wxT("x"), NULL, &xHeight, NULL, NULL);
>	   m_xHeight = lroundf(xHeight);
> -	   m_lineSpacing = m_ascent - m_descent + m_lineGap;
> +	   m_lineSpacing = m_ascent + m_descent + m_lineGap;
>  
>      }
>  
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 37840)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,16 @@
> +2008-10-23  Kevin Watters  <kevinwatters at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Use the CGFont metrics APIs for more accurate measurements, and
tweak the y
> +	   value the text is drawn at (it was a couple pixels off before
because wx
> +	   internally adds to the y value.
> +
> +	   * platform/wx/wxcode/mac/carbon/fontprops.cpp:
> +	   (wxFontProperties::wxFontProperties):
> +	   * platform/wx/wxcode/mac/carbon/non-kerned-drawing.cpp:
> +	   (WebCore::drawTextWithSpacing):
> +
>  2008-10-23  Greg Bolsinga  <bolsinga at apple.com>
>  
>	   Reviewed by Sam Weinig.