Bug 56829

Summary:

REGRESSION (r81625): fast/css/percentage-non-integer.html fails on Windows

Product:

WebKit

Reporter:

Adam Roben (:aroben) <aroben>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cabanier, commit-queue, eric, hyatt, mitz, simon.fraser, webkit.review.bot

Priority:

Keywords:

InRadar, LayoutTestFailure, PlatformOnly, Regression

Version:

528+ (Nightly build)

Hardware:

OS:

Windows 7

URL:

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r81625%20(10623)/fast/css/percentage-non-integer-pretty-diff.html

Attachments:

Description	Flags
Fix rounding issue	none
Fixed style	none
Fixed style	none
updated comment	none
removed obsolete layout results	none
removed obsolete layout results	none
removed obsolete layout results	none
updated layout tests	none
updated layout tests	none
changed to simple cast	none
changed to simple cast	none
Fixed patch	none

Adam Roben (:aroben)

Reported 2011-03-22 07:01:25 PDT

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.

Attachments
Fix rounding issue (2.48 KB, patch) 2011-03-22 16:12 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
Fixed style (2.45 KB, patch) 2011-03-22 16:25 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
Fixed style (2.46 KB, patch) 2011-03-22 16:41 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
updated comment (2.48 KB, patch) 2011-03-23 10:06 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
removed obsolete layout results (13.41 KB, patch) 2011-03-23 13:51 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
removed obsolete layout results (13.41 KB, patch) 2011-03-23 13:53 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
removed obsolete layout results (13.00 KB, patch) 2011-03-23 14:00 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
updated layout tests (38.05 KB, patch) 2011-03-23 14:50 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
updated layout tests (30.32 KB, patch) 2011-03-23 14:56 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
changed to simple cast (30.29 KB, patch) 2011-03-24 14:06 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
changed to simple cast (30.25 KB, patch) 2011-03-24 14:07 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
Fixed patch (30.25 KB, patch) 2011-03-24 14:23 PDT, Rik Cabanier	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Adam Roben (:aroben)

Comment 1 2011-03-22 07:01:45 PDT

Rik, could you please look into this?

Adam Roben (:aroben)

Comment 2 2011-03-22 07:11:33 PDT

Added expected failure results in r81666.

Adam Roben (:aroben)

Comment 3 2011-03-22 07:55:01 PDT

<rdar://problem/9168471>

Rik Cabanier

Comment 4 2011-03-22 09:55:43 PDT

(In reply to comment #1) > Rik, could you please look into this? I will sync the code down on windows and take a look.

Rik Cabanier

Comment 5 2011-03-22 12:09:36 PDT

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

Adam Roben (:aroben)

Comment 6 2011-03-22 12:13:40 PDT

Is there some way we can coerce MSVC into giving us better rounding behavior?

Rik Cabanier

Comment 7 2011-03-22 14:47:02 PDT

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

Adam Roben (:aroben)

Comment 8 2011-03-22 14:49:51 PDT

Why are we truncating in the last step instead of rounding?

Rik Cabanier

Comment 9 2011-03-22 15:18:56 PDT

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

Rik Cabanier

Comment 10 2011-03-22 15:29:04 PDT

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

Rik Cabanier

Comment 11 2011-03-22 16:12:01 PDT

Created attachment 86532 [details] Fix rounding issue

Rik Cabanier

Comment 12 2011-03-22 16:25:22 PDT

Created attachment 86537 [details] Fixed style

Rik Cabanier

Comment 13 2011-03-22 16:41:15 PDT

Created attachment 86539 [details] Fixed style

Adam Roben (:aroben)

Comment 14 2011-03-23 08:28:16 PDT

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?

Rik Cabanier

Comment 15 2011-03-23 09:53:17 PDT

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

Rik Cabanier

Comment 16 2011-03-23 10:06:12 PDT

Created attachment 86637 [details] updated comment

Adam Roben (:aroben)

Comment 17 2011-03-23 10:14:28 PDT

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?

Rik Cabanier

Comment 18 2011-03-23 10:34:28 PDT

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

Adam Roben (:aroben)

Comment 19 2011-03-23 11:46:11 PDT

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

Rik Cabanier

Comment 20 2011-03-23 13:19:56 PDT

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

Adam Roben (:aroben)

Comment 21 2011-03-23 13:34:56 PDT

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

Rik Cabanier

Comment 22 2011-03-23 13:38:19 PDT

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

Adam Roben (:aroben)

Comment 23 2011-03-23 13:44:17 PDT

Use "svn rm" to delete the files.

Rik Cabanier

Comment 24 2011-03-23 13:51:40 PDT

Created attachment 86679 [details] removed obsolete layout results

Rik Cabanier

Comment 25 2011-03-23 13:53:31 PDT

Created attachment 86680 [details] removed obsolete layout results

Rik Cabanier

Comment 26 2011-03-23 14:00:56 PDT

Created attachment 86681 [details] removed obsolete layout results

Rik Cabanier

Comment 27 2011-03-23 14:01:55 PDT

(In reply to comment #23) > Use "svn rm" to delete the files. can you check if I did it right?

Adam Roben (:aroben)

Comment 28 2011-03-23 14:15:59 PDT

The file removal looks right, though I don't think you've verified that Windows actually matches the Mac results now (have you?).

Rik Cabanier

Comment 29 2011-03-23 14:26:21 PDT

(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

Adam Roben (:aroben)

Comment 30 2011-03-23 14:27:19 PDT

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

Rik Cabanier

Comment 31 2011-03-23 14:50:58 PDT

Created attachment 86692 [details] updated layout tests

Rik Cabanier

Comment 32 2011-03-23 14:56:29 PDT

Created attachment 86695 [details] updated layout tests

Rik Cabanier

Comment 33 2011-03-23 14:57:38 PDT

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

Rik Cabanier

Comment 34 2011-03-24 14:06:18 PDT

Created attachment 86831 [details] changed to simple cast

Rik Cabanier

Comment 35 2011-03-24 14:07:56 PDT

Created attachment 86832 [details] changed to simple cast

Rik Cabanier

Comment 36 2011-03-24 14:23:42 PDT

Created attachment 86836 [details] Fixed patch

Dave Hyatt

Comment 37 2011-03-24 15:36:50 PDT

Comment on attachment 86836 [details] Fixed patch r=me

WebKit Commit Bot

Comment 38 2011-03-24 18:09:46 PDT

Comment on attachment 86836 [details] Fixed patch Clearing flags on attachment: 86836 Committed r81925: <http://trac.webkit.org/changeset/81925>

WebKit Commit Bot

Comment 39 2011-03-24 18:09:52 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 40 2011-03-24 20:43:28 PDT

http://trac.webkit.org/changeset/81925 might have broken Windows XP Debug (Tests)

Adam Roben (:aroben)

Comment 41 2011-03-25 06:59:16 PDT

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.

Rik Cabanier

Comment 42 2011-03-25 09:33:23 PDT

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

Note You need to log in before you can comment on or make changes to this bug.