[webkit-reviews] review denied: [Bug 48640] Add layout test that check getBoundingClientRect w/ zooming : [Attachment 72341] Zooming tests for getBoundingClientRect

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Cosmin Truta
<ctruta at chromium.org>'s request for review:
Bug 48640: Add layout test that check getBoundingClientRect w/ zooming
https://bugs.webkit.org/show_bug.cgi?id=48640

Attachment 72341: Zooming tests for getBoundingClientRect
https://bugs.webkit.org/attachment.cgi?id=72341&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72341&action=review

Hi Cosmin,

good job, though in the wrong direction I fear. I was confused what you mean
with rounding problems. I thought you wanted to solve it in the code, rather
than in the tests :-)

> LayoutTests/svg/custom/getBoundingClientRect.xhtml:7
> +<div style="width:500px;height=100px;">

This can't work :-) s/=/:/

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:53
> +    shouldBeApprox('div1.left', '0', '1');

The last argument doesn't need to be a string, but it's okay, you don't need to
change it everywhere.

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:54
> +    shouldBeApprox('div1.top', '0', '1');

I don't get the whole story behind shouldBeApprox.
CSSOM defines getBoundingClientRect to return a ClientRect() object, containing
'float' coordinates.
See http://dev.w3.org/csswg/cssom-view/#clientrect.



	FloatRect localRect;
	if (svgElement->boundingBox(localRect))
	    quads.append(renderer()->localToAbsoluteQuad(localRect));

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.

Of course when testing, you might get results like 50.000001 or sth else when
zooming (due the rounding error accumulation).
You can solve this by using shouldBe("div1.width.toFixed(2)", "50.00")....
(toFixed is the key here)

Does this help to solve the underlying problem?