|Summary:||[EFL][GTK][Qt] New test: fast/regions/autoheight-correct-region-for-lines-2.html fails after r152572.|
|Product:||WebKit||Reporter:||Gábor Ábrahám <abrhm>|
|Component:||Tools / Tests||Assignee:||Javier Fernandez <jfernandez>|
|Severity:||Normal||CC:||abucur, dino, jfernandez, kadam, mcatanzaro, mihnea, mrobinson, ossy, rniwa, simon.fraser, spenap, thorton, WebkitBugTracker, zan, zarvai|
|Version:||528+ (Nightly build)|
|Bug Depends on:||119273|
|Bug Blocks:||63344, 87008, 118531|
Description Gábor Ábrahám 2013-07-12 04:33:52 PDT
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?
Comment 1 Gábor Ábrahám 2013-07-12 06:36:27 PDT
Comment 2 Gábor Ábrahám 2013-07-12 06:47:57 PDT
Created attachment 206538 [details] The actual picture
Comment 3 Gábor Ábrahám 2013-07-12 06:59:57 PDT
Created attachment 206541 [details] Actual I will skip this test on Qt now, and upload the actual picture here, finally.
Comment 4 Andrei Bucur 2013-07-15 01:32:15 PDT
Thanks for the report, we'll take a look at it.
Comment 5 Javier Fernandez 2013-07-16 02:13:18 PDT
I can work on this bug, if nobody tells me otherwise.
Comment 6 Javier Fernandez 2013-07-29 11:32:45 PDT
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.
Comment 7 Javier Fernandez 2013-07-30 14:25:19 PDT
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.
Comment 9 Ryosuke Niwa 2013-09-09 11:50:51 PDT
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.
Comment 10 Javier Fernandez 2013-09-10 01:13:37 PDT
(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.
Comment 11 Ryosuke Niwa 2013-09-10 09:58:50 PDT
I'm simply concerned about losing the test coverage even for temporarily.
Comment 12 Javier Fernandez 2014-02-03 12:00:07 PST
This bug should be fixed now, at least in the WebKitGtk+ port, now that the SATURATED_LAYOUT_ARITHMETIC is enabled (bug 119273).