fast/css/percentage-non-integer.html has been failing on Windows since r81625. See the URL for the failure diff. It looks like the rounding on Windows is worse than on Mac. On Mac, the table cells increase in width by 2px each time. On Windows, they sometimes increase by other amounts.
Rik, could you please look into this?
Added expected failure results in r81666.
<rdar://problem/9168471>
(In reply to comment #1) > Rik, could you please look into this? I will sync the code down on windows and take a look.
(In reply to comment #0) > fast/css/percentage-non-integer.html has been failing on Windows since r81625. See the URL for the failure diff. > > It looks like the rounding on Windows is worse than on Mac. On Mac, the table cells increase in width by 2px each time. On Windows, they sometimes increase by other amounts. This probably has to do with how floats and doubles interact with different compilers.
Is there some way we can coerce MSVC into giving us better rounding behavior?
(In reply to comment #6) > Is there some way we can coerce MSVC into giving us better rounding behavior? This is a 32bit intel rounding issue. The FPU will always convert a 32 bit float of 80bit. This means that a value of 1.4 becomes 1.3999999997. In this case the logic becomes: 1000*1.4f/100.0f -> 10000.0*1.3999997/100.0 -> 13.999997 -> (cast to int) 13 64 bit or MMX doesn't suffer this problem. Setting/resetting the fpu precision should work around this issue but would result in slow code. Adding a fudge factor would fix this issue as well as enforcing MMX...
Why are we truncating in the last step instead of rounding?
(In reply to comment #8) > Why are we truncating in the last step instead of rounding? Because that is what the old class did as well.
(In reply to comment #1) > Rik, could you please look into this? I have a fix for the issue and will submit a review shortly.
Created attachment 86532 [details] Fix rounding issue
Created attachment 86537 [details] Fixed style
Created attachment 86539 [details] Fixed style
Comment on attachment 86539 [details] Fixed style View in context: https://bugs.webkit.org/attachment.cgi?id=86539&action=review > Source/WebCore/platform/Length.h:143 > + // do not try to optimize this. It will cause incorrect rounding on 32-bit intel machines that use the FPU > + float retVal = maxValue * percent() / 100.0f; > + return static_cast<int>(retVal); I don't think this comment is clear enough. "optimize" could have many different interpretations. Explicitly saying "Don't remove the local float variable" would be more helpful. The first word of the comment should be capitalized, as should "Intel". It is very surprising that a float local fixes the problem. I assume appending "f" to 100.0 wasn't enough?
(In reply to comment #14) > (From update of attachment 86539 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86539&action=review > > > Source/WebCore/platform/Length.h:143 > > + // do not try to optimize this. It will cause incorrect rounding on 32-bit intel machines that use the FPU > > + float retVal = maxValue * percent() / 100.0f; > > + return static_cast<int>(retVal); > > I don't think this comment is clear enough. "optimize" could have many different interpretations. Explicitly saying "Don't remove the local float variable" would be more helpful. > > The first word of the comment should be capitalized, as should "Intel". > > It is very surprising that a float local fixes the problem. I assume appending "f" to 100.0 wasn't enough? Adding 'f' to the number didn't make a difference on Intel 32 bit since all calculations are done with 85bit precision. Because the floats don't have that much precision and the cast happened on the 85bit register, we got imprecise results. Going through a float causes correct rounding. On other platform that don't have this problem, the 'f' behind the number is good practise so the processor does all calculations in float precision. I will fix the comment and update the patch.
Created attachment 86637 [details] updated comment
I'm hoping one of our layout folks can review this. Rik, don't we need to do something about the expected failure results I checked in in r81666?
(In reply to comment #17) > I'm hoping one of our layout folks can review this. > > Rik, don't we need to do something about the expected failure results I checked in in r81666? Not sure what r81666 is. Is it different from what is checked in? I know that the pixel test would need to be updated for some platforms.
As explained in comment 1, r81625 made this test fail on Windows. In r81666 I checked in new results for Windows that matched the current (incorrect) behavior of Windows on that test. Since you're now giving Windows the correct behavior, presumably those results I added in r81666 should be removed, which will cause Windows to fall back to the Mac results. (You'll need to test that Windows actually matches the Mac results.)
(In reply to comment #19) > As explained in comment 1, r81625 made this test fail on Windows. In r81666 I checked in new results for Windows that matched the current (incorrect) behavior of Windows on that test. > > Since you're now giving Windows the correct behavior, presumably those results I added in r81666 should be removed, which will cause Windows to fall back to the Mac results. (You'll need to test that Windows actually matches the Mac results.) yes, that code should be removed. The test has to render the same on all platforms. Do I need to do this? (I have never done this before) Or, can we stage it?
It is standard practice to update test results in the same checkin as the code that changes the test's behavior. If you don't do that then we will end up with red bots (even if only temporarily).
(In reply to comment #21) > It is standard practice to update test results in the same checkin as the code that changes the test's behavior. If you don't do that then we will end up with red bots (even if only temporarily). How do I remove the layout tests you added? Do I just delete the files from my drive and create a patch?
Use "svn rm" to delete the files.
Created attachment 86679 [details] removed obsolete layout results
Created attachment 86680 [details] removed obsolete layout results
Created attachment 86681 [details] removed obsolete layout results
(In reply to comment #23) > Use "svn rm" to delete the files. can you check if I did it right?
The file removal looks right, though I don't think you've verified that Windows actually matches the Mac results now (have you?).
(In reply to comment #28) > The file removal looks right, though I don't think you've verified that Windows actually matches the Mac results now (have you?). true :-) Where is it pulling the expected results from? I can't tell what it is using as the expected output
Windows looks in these locations (in this order): platform/win-xp (only if you're running Windows XP) platform/win platform/mac-snowleopard platform/mac top level
Created attachment 86692 [details] updated layout tests
Created attachment 86695 [details] updated layout tests
(In reply to comment #30) > Windows looks in these locations (in this order): > > platform/win-xp (only if you're running Windows XP) > platform/win > platform/mac-snowleopard > platform/mac > top level OK. Now it should be OK. I just updated the windows layout tests.
Created attachment 86831 [details] changed to simple cast
Created attachment 86832 [details] changed to simple cast
Created attachment 86836 [details] Fixed patch
Comment on attachment 86836 [details] Fixed patch r=me
Comment on attachment 86836 [details] Fixed patch Clearing flags on attachment: 86836 Committed r81925: <http://trac.webkit.org/changeset/81925>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/81925 might have broken Windows XP Debug (Tests)
The new Windows results were generated using the wrong fonts. Since we match Mac now, we don't need Windows-specific results at all. I removed them in r81958.
(In reply to comment #41) > The new Windows results were generated using the wrong fonts. Since we match Mac now, we don't need Windows-specific results at all. I removed them in r81958. I was wondering about that. Can you tell me where I can find the windows fonts? I tried to follow the instructions, but couldn't get it to work...