720 - Font size rounding bug

Status: VERIFIED FIXED

(MozillaClassic Graveyard :: FontLib, defect, P1)

Description

[harishd]

Peter Linss wrote:

In working to make NGLayout's font handling match that of Nav's, I've uncovered
a rounding error in the Win libfont code:

modules/libfont/producers/win/src/winfp.cpp: line 875

// for passing to Windows, convert pointsize to pixel size and negative.
 if( pointSize > 0 )
  pixelSize = (int) - pointSize * pPrmFont->YPixelPerInch / 72.0;

pointSize is the input size (a double), the rounding error is introduced in the
conversion to pixel size.
The problem is a classic lack of parentheses, the int conversion is happening
before the divide by 72.0.
To do what was meant, the line should read:
  pixelSize = (int) - ((pointSize * pPrmFont->YPixelPerInch) / 72.0);
However, this is also wrong. The proper (ie: Windows) way is to round to the
rearest integer, not the smallest. According to the
Windows docs the code should read:
  pixelSize = - MulDiv((int)pointSize, pPrmFont->YPixelPerInch, 72);
That would give the correct rounding.

This bug results in Nav being unable to render certain font sizes, ie: 8 point
text comes out significantly smaller (more like 7
point).

For more information about the way it looks checkout the above URL.

Comment 1

[rubydoo123]

dp, since this may require a code review to verify the fix, who would be the
appropriate engineer to look at the code.

Comment 2

[rubydoo123]

assigning Fenella as QA Assigned to

Comment 3

[chris hofmann]

tell me if this is a regression from 4.05...

Comment 4

[Suresh Duddi (gone)]

This isn't a regression. We are still evaluating whether we need to fix it ASAP.
Data is pretty convoluted betn. 3.0,4.0,IE4.0,IE5.0 and word.

We should revisit this for 4.5

Comment 5

[leger]

Putting on b2 radar.  Changing from P0 to P1.

Comment 6

[Jean-Charles Mourey]

This is not a regression nor a crash, nor data loss, nor an escalation,
therefore this bug doesn't meet the 4.5 criteria
(http://webgroup/novabugs.html).  Marking TFV 5.0.

Comment 7

[leger]

Setting all current Open/Normal to M4.

Comment 8

[leger]

Old bug.  beppe, couldyou assign QA to Verify this one please.

Comment 9

[rubydoo123]

asking dp who is the best person to review this within fontlib.

Comment 10

[rubydoo123]

I talked with Harish about how we could verify this bug and there really isn't
any way that we can do this visually. This would really need to be done through
assessing what the code pushes out. Maybe Peter Linss has a suggestion on how to
 verify this.

Comment 11

[rubydoo123]

I talked with Harish about how we could verify this bug and there really isn't
any way that we can do this visually. This would really need to be done through
assessing what the code pushes out. Maybe Peter Linss has a suggestion on how to
 verify this.

Comment 12

[rubydoo123]

per Peters assessment, marking this as verified