[EFL][GTK][Qt] New test: fast/regions/autoheight-correct-region-for-lines-2.html fails after r152572.

[Gábor Ábrahám]

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?

[Gábor Ábrahám]

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

[Gábor Ábrahám]

Created attachment 206538 [details]
The actual picture

[Gábor Ábrahám]

Created attachment 206541 [details]
Actual

I will skip this test on Qt now, and upload the actual picture here, finally.

[Andrei Bucur]

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

[Javier Fernandez]

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

[Javier Fernandez]

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.

[Javier Fernandez]

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.

[Javier Fernandez]

Created attachment 210642 [details]
Patch

[Ryosuke Niwa]

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.

[Javier Fernandez]

(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.

[Ryosuke Niwa]

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

[Javier Fernandez]

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

[Michael Catanzaro]

Comment on attachment 210642 [details]
Patch

We no longer have failure expectations for this bug, so I guess it is obsolete.