Closed Bug 1039796 Opened 5 years ago Closed 5 years ago

Make nsLayoutUtils arithmetic consistent for computing dimension from intrinsic-ratio & other dimension

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

Just noticed that nsLayoutUtils uses two different techniques for converting e.g. width & intrinsic-ratio to a height  (and height & ratio to a width):

FLOAT MATH VERSION:
===================
nsLayoutUtils::IntrinsicForContainer() has:
> 3789           result =
> 3790             NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));
[...]
> 3796           nscoord maxWidth =
> 3797             NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));
[...]
> 3805           nscoord minWidth =
> 3806             NSToCoordRound(h * (float(ratio.width) / float(ratio.height)));
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=d159f3b81265#3789
...which are from bug 364066.

INT64 MATH VERSION:
===================
nsLayoutUtils::ComputeSizeWithIntrinsicDimensions() and nsLayoutUtils::ComputeAutoSizeWithIntrinsicDimensions() and 
> 4262         tentWidth = MULDIV(intrinsicHeight, aIntrinsicRatio.width, aIntrinsicRatio.height);
[...]
> 4273         tentHeight = MULDIV(tentWidth, aIntrinsicRatio.height, aIntrinsicRatio.width);
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=d159f3b81265#4262

...where MULDIV is defined as:
> 4086 #define MULDIV(a,b,c) (nscoord(int64_t(a) * int64_t(b) / int64_t(c)))
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=d159f3b81265#4085

This MULDIV stuff is from bug 370629.


We should make this arithmetic consistent. I'm guessing the MULDIV version is faster (particularly on 64-bit platforms), since it doesn't involve floating-point math.
Attached patch fix v1: standardize on MULDIV (obsolete) — Splinter Review
Will request review if Try run goes well:
  https://tbpl.mozilla.org/?tree=Try&rev=c17072c4f182
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Comment on attachment 8457615 [details] [diff] [review]
fix v1: standardize on MULDIV

Try run is good. Requesting review.
Attachment #8457615 - Flags: review?(dbaron)
Flags: needinfo?(dholbert)
Comment on attachment 8457615 [details] [diff] [review]
fix v1: standardize on MULDIV

I think switching to floor instead of round should be fine given that we're dealing with nscoords, i.e., 1/60ths of a CSS pixel.

r=dbaron
Attachment #8457615 - Flags: review?(dbaron) → review+
(Yup; agreed on floor vs. round not making a big difference.)
Tree's closed --> using checkin-needed. (Try run in comment 1.)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea66bf9982ff
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.