On 21/03/14 18:14, Achal Aggarwal wrote:
This change is not right.
First, you're introducing a multiply 1.3, which is a floating point
operation. The rest of that calculation is using fixed point to
avoid using floating point.
Second you're doing setup->text.size/FONT_SIZE___SCALE which will
lose any fractional part of the original font size. E.g. your
change would make 12.8pt get treated as if it were 12pt. Third, I'm
not sure why you're modifying that calculation. I don't see
anything wrong with it. Have I missed something?
earlier FONT_SIZE_SCALE was treated as a fixed point which is actually
an integer
Just think of FONT_SIZE_SCALE as a multiplier.
> and multiplied with another fixed point ( F_72 ) as
FONT_SIZE_SCALE * F_72
instead of using FMUL
Which is fine. We know there won't be an arithmetic overflow here.
I changed a bit, now it is
ret->line_height = FIXTOINT(FDIV(FMUL(nscss_screen_dpi,
FLTTOFIX((setup->text.size * 1.3) / FONT_SIZE_SCALE)), F_72));
incorporating 1.3 and not loosing precision of text.size. I checked that
when I set font-size:100px I got line_height:100 and when I set 75pt, I
got 100px equivalent to 75pt.
1. That doesn't address my first point, that you're introducing floating
point arithmetic.
2. It doesn't address my second point, that you're dividing the font
size by the font scale, and thus losing any fractions of point sizes.
The original calc is essentially:
(1.3 * dpi * font_size) / (font_scale * 72)
Effectively converting pt to dots (pixels) before dividing
You're doing:
(dpi * ((font_size * 1.3) / font_scale)) / 72
That now has 2 divides. More importantly, it is doing the division
before converting pts to pixels:
(font_size * 1.3) / font_scale
So the fractions of pt sizes are lost. The *1.3 doesn't offset the
problem that much.
3. It doesn't address my third point, which was why are you modifying
this? What fault are you trying to fix?
diff --git a/render/font.c b/render/font.c
index 03c5a36..f594678 100644
--- a/render/font.c
+++ b/render/font.c
@@ -45,8 +45,7 @@ void font_plot_style_from_css(const
css_computed_style *css,
css_computed_font_family(css, &families));
css_computed_font_size(css, &length, &unit);
- fstyle->size = FIXTOINT(FMUL(nscss_len2pt(__length, unit),
- INTTOFIX(FONT_SIZE_SCALE)));
+ fstyle->size = FIXTOINT(nscss_len2pt(length, unit)) *
FONT_SIZE_SCALE;
Again, this change isn't relevant to the bug as far as I can see.
I guess we don't need to convert FONT_SIZE_SCALE to fixed point.
checkout
https://github.com/Achal-Aggarwal/netsurf/blob/master/render/textplain.c#L1323
size is point scaled using FONT_SIZE_SCALE.
Your change here is wrong. You're converting from a fixed point pt size
to an int before applying the FONT_SIZE_SCALE multiplier. This means
that any fractions of a pt size are lost. e.g. 11.9pt becomes 11pt.
The original code preserved this by applying the FONT_SIZE_SCALE
multiplier before converting from fixed point to integer.
Again, this code is not relevant to the textarea issue baseline offset
issue you were attempting to fix. It is in the generic text handling code.
