After r152572 new fast/regions/autoheight-correct-region-for-lines-2.html ref test fails on EFL, GTK and Qt ports.
Link to image diff: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r152572%20(52260)/fast/regions/autoheight-correct-region-for-lines-2-diffs.html
It looks like the problem is the same at all port.
Could you check it please?
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]
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]
Comment on attachment 210642 [details]
View in context: https://bugs.webkit.org/attachment.cgi?id=210642&action=review
> - 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]
We no longer have failure expectations for this bug, so I guess it is obsolete.