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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1: standardize on MULDIV

Will request review if Try run goes well:
  https://tbpl.mozilla.org/?tree=Try&rev=c17072c4f182

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8457615 [details] [diff] [review]
fix v1: standardize on MULDIV

Try run is good. Requesting review.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 4

[Daniel Holbert [:dholbert]]

(Yup; agreed on floor vs. round not making a big difference.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2, for landing (same patch; just noted r=dbaron in commit message]

Comment 6

[Daniel Holbert [:dholbert]]

Tree's closed --> using checkin-needed. (Try run in comment 1.)

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea66bf9982ff

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ea66bf9982ff