[webkit-reviews] review denied: [Bug 48110] getBoundingClientRect: Do not truncate the coordinates to integers : [Attachment 74442] Fix and layout tests

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Cosmin Truta
<ctruta at chromium.org>'s request for review:
Bug 48110: getBoundingClientRect: Do not truncate the coordinates to integers
https://bugs.webkit.org/show_bug.cgi?id=48110

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

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

Patch is still perfect, I"m tempted to r+ it, but there are still some minor
issues with the LayoutTests:

> LayoutTests/css3/zoom-coords.xhtml:-87
> -shouldBe('text1.top >= 75', 'true');
> +shouldBe('text1.top >= 250', 'true');
>  shouldBe('text1.width > 0', 'true');
>  shouldBe('text1.height > 0', 'true');
>  shouldBe('text1.right > 100', 'true');
> -shouldBe('text1.bottom > 75', 'true');

There's no way you could check for exact coordinate matches here??

> LayoutTests/css3/zoom-coords.xhtml:136
> -shouldBe('text2.left == 800', 'true');
> -shouldBe('text2.top >= 400', 'true');
> +shouldBe('text2.left == 175', 'true');
> +shouldBe('text2.top >= 100', 'true');

Ditto.

> LayoutTests/css3/zoom-coords.xhtml:141
>  shouldBe('text2.width > 0', 'true');
>  shouldBe('text2.height > 0', 'true');
> -shouldBe('text2.right > 800', 'true');
> -shouldBe('text2.bottom > 400', 'true');
> +shouldBe('text2.right > 175', 'true');
> +shouldBe('text2.bottom > 100', 'true');
> +debug("");

Ditto.

> LayoutTests/css3/zoom-coords.xhtml:170
> +shouldBe('text3.left == 1100', 'true');
> +shouldBe('text3.top >= 500', 'true');
> +shouldBe('text3.width > 0', 'true');
> +shouldBe('text3.height > 0', 'true');
> +shouldBe('text3.right > 1100', 'true');
> +shouldBe('text3.bottom > 500', 'true');

Ditto.

> LayoutTests/fast/dom/Element/getBoundingClientRect-expected.txt:8
> +PASS rect.left.toFixed(3) is "8.000"
> +PASS rect.top.toFixed(3) is "40.000"
> +PASS rect.width.toFixed(3) is "300.000"
> +PASS rect.height.toFixed(3) is "100.000"
> +PASS rect.right is rect.left + rect.width
> +PASS rect.bottom is rect.top + rect.height
> +

Excellent!

> LayoutTests/svg/zoom/page/zoom-getBoundingClientRect-expected.txt:6
> +PASS parseFloat(r1.left.toFixed(2)) is 20

You could have used:
shouldBeEqualToString("r1.left.toFixed(2)", "20.00")

This is cleaner, then using parseFloat on a toFixed returned string.

> LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:11
> +</svg><svg xmlns="http://www.w3.org/2000/svg" version="1.1"

Can you add a newline between </svg> and <svg>.

> LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:15
> +</svg><svg xmlns="http://www.w3.org/2000/svg" version="1.1"

Ditto.

> LayoutTests/svg/zoom/page/zoom-getBoundingClientRect.xhtml:34
> +    shouldBe('parseFloat(r1.left.toFixed(2))', '20');
> +    shouldBe('parseFloat(r1.top.toFixed(2))', '30');

As stated above, shouldBeEqualToString should be sufficient.

> LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:86
> +PASS parseFloat(text2.top.toFixed(2)) >= 100 is true
> +PASS parseFloat(text2.width.toFixed(2)) > 0 is true
> +PASS parseFloat(text2.height.toFixed(2)) > 0 is true
> +PASS parseFloat(text2.right.toFixed(2)) > 175 is true
> +PASS parseFloat(text2.bottom.toFixed(2)) > 100 is true
> +

No way to use equality here?

> LayoutTests/svg/zoom/page/zoom-zoom-coords-expected.txt:110
> +PASS parseFloat(text3.top.toFixed(2)) >= 500 is true
> +PASS parseFloat(text3.width.toFixed(2)) > 0 is true
> +PASS parseFloat(text3.height.toFixed(2)) > 0 is true
> +PASS parseFloat(text3.right.toFixed(2)) > 1100 is true
> +PASS parseFloat(text3.bottom.toFixed(2)) > 500 is true

Ditto.

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:22
> +</svg><svg id="svg2" xmlns="http://www.w3.org/2000/svg"

Please add a newline between </svg> and <svg>.

> LayoutTests/svg/zoom/page/zoom-zoom-coords.xhtml:28
> +</svg><svg id="svg3" xmlns="http://www.w3.org/2000/svg"

Ditto.