Last Comment Bug 774968 - line-height percentage value not computed accurately
: line-height percentage value not computed accurately
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Cameron McCormack (:heycam)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-17 19:50 PDT by Cameron McCormack (:heycam)
Modified: 2012-07-20 06:44 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test (190 bytes, text/html)
2012-07-17 19:50 PDT, Cameron McCormack (:heycam)
no flags Details
patch (3.14 KB, patch)
2012-07-18 12:44 PDT, Cameron McCormack (:heycam)
dbaron: review+
Details | Diff | Review

Description Cameron McCormack (:heycam) 2012-07-17 19:50:11 PDT
Created attachment 643238 [details]
test

Consider the attached document.  I'd expect it to print "20.5px" but instead it prints "20.4833px", one app unit off.
Comment 1 Boris Zbarsky [:bz] 2012-07-17 21:05:42 PDT
I get 20.5, for what it's worth.  At least on a Mac.
Comment 2 Cameron McCormack (:heycam) 2012-07-18 03:39:04 PDT
Oh yeah, forgot to mention: this is on 32 bit Linux.  (And I think it also happens on Windows, based on try server failures of a non-reduced version of this test.)
Comment 3 Cameron McCormack (:heycam) 2012-07-18 05:33:43 PDT
I think the issue is due to 0.411 not being exactly representable as a float.  The computation is done with:

    text->mLineHeight.SetCoordValue(
        nscoord(float(aContext->GetStyleFont()->mFont.size) *
                lineHeightValue->GetPercentValue()));

I assume different compilers are doing the floating point calculations differently, and because we're rounding down when finally converting to a nscoord and not rounding to nearest, we end up losing that one app unit.
Comment 4 Cameron McCormack (:heycam) 2012-07-18 05:35:38 PDT
Is it reasonable to change it to use NSToCoordRound here?
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-18 06:08:35 PDT
(In reply to Cameron McCormack (:heycam) from comment #4)
> Is it reasonable to change it to use NSToCoordRound here?

yes
Comment 6 Cameron McCormack (:heycam) 2012-07-18 12:44:59 PDT
Created attachment 643525 [details] [diff] [review]
patch
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-19 13:57:40 PDT
Comment on attachment 643525 [details] [diff] [review]
patch

r=dbaron

(What adds the funny number at the end of the file?)
Comment 8 Cameron McCormack (:heycam) 2012-07-19 13:58:40 PDT
Ah that's git-format-patch that inserts that (and the script I have to convert the git patch to an hg patch seems to retain it).  It's the version of git that produced the patch.
Comment 9 Cameron McCormack (:heycam) 2012-07-19 14:05:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c92185fa438
Comment 10 Ed Morley [:emorley] 2012-07-20 06:44:05 PDT
https://hg.mozilla.org/mozilla-central/rev/8c92185fa438

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