[webkit-reviews] review requested: [Bug 48110] getBoundingClientRect: loss of precision because of premature float-to-int truncation : [Attachment 72538] Fix and layout tests

[bugzilla-daemon at webkit.org]

Cosmin Truta <ctruta at chromium.org> has asked  for review:
Bug 48110: getBoundingClientRect: loss of precision because of premature
float-to-int truncation
https://bugs.webkit.org/show_bug.cgi?id=48110

Attachment 72538: Fix and layout tests
https://bugs.webkit.org/attachment.cgi?id=72538&action=review

------- Additional Comments from Cosmin Truta <ctruta at chromium.org>
In response to Niko's comment from bug 48640:

> localToAbsoluteQuad gives you a FloatRect, stored in localRect.
> The problem is the "	  IntRect result = quads[0].enclosingBoundingBox();"
usage here.
> When using boundingBox() instead of enclosingBoundingBox() you'd get a
FloatRect as result, which is really what ought to be used here.
>
> Instead of using "adjustIntRectForAbsoluteZoom(IntRect& rect, RenderObject*
renderer)" a adjustFloatRectForAbsoluteZoom should be added to RenderObject.h
(there's already a FloatQuad version and a FloatPoint version).
> That should fix the real issue.

I replaced adjustIntRect... with adjustFloatRect... from the very beginning,
but that turned out not to be sufficient.

For example, assuming the coordinate values 20, 30, 40, 50, zoomed by the
factor 1.2f (whose more precise approximation is 1.20000005), the results of
scaling 20 and 40 up and down are exact, but the results of scaling 30 and 50
up and down are not. See the following sample code:


#include <iostream>
#include <iomanip>

float scale = 1.2f;  // 1.20000005
float f24 = 24.0f;
float f36 = 36.0f;
float f48 = 48.0f;
float f60 = 60.0f;

float f20 = f24 / scale;  // 20.0 (exact)
float f30 = f36 / scale;  // 29.9999981
float f40 = f48 / scale;  // 40.0 (exact)
float f50 = f60 / scale;  // 49.9999962

int main()
{
    std::cout << std::setprecision(9) << f20 << std::endl
	      << std::setprecision(9) << f30 << std::endl
	      << std::setprecision(9) << f40 << std::endl
	      << std::setprecision(9) << f50 << std::endl;
}


For this reason, in spite of deferring the float-to-int conversion, an epsilon
is still required in enclosingIntRect.

I essentially made enclosingIntRect act as in roundForImprecise. If the
reviewer does not agree with this modification to enclosingIntRect, I will
perhaps add another function called enclosingIntRectRoundedForImprecise, in the
next iteration of this bugfix.
And an observation: the epsilon value 0.01 used in enclosingIntRect is still
too small, but making it larger won't help. The zooming tests pass with
zoomCount=2, but they start failing if zoomCount grows larger than 2.
Coordinates like svg1.top are altered from 250 to 248, even for zoomCount=3.