Bug 118595

Actual image: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r152572%20(52260)/fast/regions/autoheight-correct-region-for-lines-2-actual.png

Created attachment 206538 [details] The actual picture

Created attachment 206541 [details] Actual I will skip this test on Qt now, and upload the actual picture here, finally.

Thanks for the report, we'll take a look at it.

I can work on this bug, if nobody tells me otherwise.

After looking into this issue for a while, I've found out that the issue is related to an arithmetic overflow when computing the logic height. The maxHeight property is defined in the test as a huge value, which is handled as a Length instance based on a float type. I've found out that the flag SATURATED_LAYOUT_ARITHMETIC is not enabled by default in the WebKitGtk+ port, so when calling to the LayoutUnit(float), it does not apply the clampTo<float> template function. Just enabling the SATURATED_LAYOUT_ARITHMETIC flag makes the test to pass, at least for the WebkitGtk+ port, but I'm still doing some further investigations to understand why this is happening, whether is a Region related issue or a more general issue, Besides, I want to understand how the combination of both, SATURATED_LAYOUT_ARITHMETIC and SUBPIXEL_LAYOUT actually works and which cases each of them handle.

This is clearly a bug not related to Regions at all, since the actual issue was reported in the bug #119273. A simple fix would be just to set a lower value for the max-height property (like 20000000px, big enough, I would say). Of course it's also possible to wait for the bug #119273 fix, but notice that the max-height value has noting to do with the purpose of the test, besides allowing the region to adjust its auto-height properly.

Created attachment 210642 [details] Patch

Comment on attachment 210642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=210642&action=review > LayoutTests/fast/regions/autoheight-correct-region-for-lines-2.html:20 > - max-height: 2000000000px; > + max-height: 20000000px; I'm not sure if it's such a good idea to workaround this bug in the test. This test was valuable in that it caught one case in which enabling subpixel layout without saturated arithmetic would result in a bug. Do we have a test in LayoutTests/fast/sub-pixel that catches the same bug? If not, please add one if you decide to "fix" the test.

(In reply to comment #9) > (From update of attachment 210642 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210642&action=review > > > LayoutTests/fast/regions/autoheight-correct-region-for-lines-2.html:20 > > - max-height: 2000000000px; > > + max-height: 20000000px; > > I'm not sure if it's such a good idea to workaround this bug in the test. > This test was valuable in that it caught one case in which enabling subpixel layout without saturated arithmetic would result in a bug. > Do we have a test in LayoutTests/fast/sub-pixel that catches the same bug? If not, please add one if you decide to "fix" the test. I share the concerns as well, but on the other hand, that was not the purpose of the tests. I've filed a new one specifically for this issue at bug #119273, which also provides the corresponding test. My idea would be to close this bug and track the arithmetic overflow issue in the specific bug. Besides, the fix for bug #119273 would imply, as discussed in the mailing list, merging the 2 flags so that SATURATED_LAYOUT_ARITHMETIC would be enabled whenever SUBPIXEL_LAYOUT is. Anyway, I've been talking about this with Mihnea Ovidenie so perhaps we should wait to get some feedback from him. Also, we could wait until bug #119273 is fixed to take the final decision.

I'm simply concerned about losing the test coverage even for temporarily.

This bug should be fixed now, at least in the WebKitGtk+ port, now that the SATURATED_LAYOUT_ARITHMETIC is enabled (bug 119273).

Comment on attachment 210642 [details] Patch We no longer have failure expectations for this bug, so I guess it is obsolete.