Bug 56829 - REGRESSION (r81625): fast/css/percentage-non-integer.html fails on Windows
Summary: REGRESSION (r81625): fast/css/percentage-non-integer.html fails on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL: http://build.webkit.org/results/Windo...
Keywords: InRadar, LayoutTestFailure, PlatformOnly, Regression
Depends on:
Blocks:
 
Reported: 2011-03-22 07:01 PDT by Adam Roben (:aroben)
Modified: 2011-03-25 09:33 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 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.
Comment 1 Adam Roben (:aroben) 2011-03-22 07:01:45 PDT
Rik, could you please look into this?
Comment 2 Adam Roben (:aroben) 2011-03-22 07:11:33 PDT
Added expected failure results in r81666.
Comment 3 Adam Roben (:aroben) 2011-03-22 07:55:01 PDT
<rdar://problem/9168471>
Comment 4 Rik Cabanier 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.
Comment 5 Rik Cabanier 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.
Comment 6 Adam Roben (:aroben) 2011-03-22 12:13:40 PDT
Is there some way we can coerce MSVC into giving us better rounding behavior?
Comment 7 Rik Cabanier 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...
Comment 8 Adam Roben (:aroben) 2011-03-22 14:49:51 PDT
Why are we truncating in the last step instead of rounding?
Comment 9 Rik Cabanier 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.
Comment 10 Rik Cabanier 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.
Comment 11 Rik Cabanier 2011-03-22 16:12:01 PDT
Created attachment 86532 [details]
Fix rounding issue
Comment 12 Rik Cabanier 2011-03-22 16:25:22 PDT
Created attachment 86537 [details]
Fixed style
Comment 13 Rik Cabanier 2011-03-22 16:41:15 PDT
Created attachment 86539 [details]
Fixed style
Comment 14 Adam Roben (:aroben) 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?
Comment 15 Rik Cabanier 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.
Comment 16 Rik Cabanier 2011-03-23 10:06:12 PDT
Created attachment 86637 [details]
updated comment
Comment 17 Adam Roben (:aroben) 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?
Comment 18 Rik Cabanier 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.
Comment 19 Adam Roben (:aroben) 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.)
Comment 20 Rik Cabanier 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?
Comment 21 Adam Roben (:aroben) 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).
Comment 22 Rik Cabanier 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?
Comment 23 Adam Roben (:aroben) 2011-03-23 13:44:17 PDT
Use "svn rm" to delete the files.
Comment 24 Rik Cabanier 2011-03-23 13:51:40 PDT
Created attachment 86679 [details]
removed obsolete layout results
Comment 25 Rik Cabanier 2011-03-23 13:53:31 PDT
Created attachment 86680 [details]
removed obsolete layout results
Comment 26 Rik Cabanier 2011-03-23 14:00:56 PDT
Created attachment 86681 [details]
removed obsolete layout results
Comment 27 Rik Cabanier 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?
Comment 28 Adam Roben (:aroben) 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?).
Comment 29 Rik Cabanier 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
Comment 30 Adam Roben (:aroben) 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
Comment 31 Rik Cabanier 2011-03-23 14:50:58 PDT
Created attachment 86692 [details]
updated layout tests
Comment 32 Rik Cabanier 2011-03-23 14:56:29 PDT
Created attachment 86695 [details]
updated layout tests
Comment 33 Rik Cabanier 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.
Comment 34 Rik Cabanier 2011-03-24 14:06:18 PDT
Created attachment 86831 [details]
changed to simple cast
Comment 35 Rik Cabanier 2011-03-24 14:07:56 PDT
Created attachment 86832 [details]
changed to simple cast
Comment 36 Rik Cabanier 2011-03-24 14:23:42 PDT
Created attachment 86836 [details]
Fixed patch
Comment 37 Dave Hyatt 2011-03-24 15:36:50 PDT
Comment on attachment 86836 [details]
Fixed patch

r=me
Comment 38 WebKit Commit Bot 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>
Comment 39 WebKit Commit Bot 2011-03-24 18:09:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 WebKit Review Bot 2011-03-24 20:43:28 PDT
http://trac.webkit.org/changeset/81925 might have broken Windows XP Debug (Tests)
Comment 41 Adam Roben (:aroben) 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.
Comment 42 Rik Cabanier 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...