Summary: | enclosingIntRect doesn't handle negative offsets | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brett Wilson (Google) <brettw> | ||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brett Wilson (Google)
2008-10-01 19:44:28 PDT
Also, as per the FIXME in that function, the maximum size is actually rounded to nearest when positive, and will be totally wrong when the right or bottom is negative. Maybe this function should just be renamed "randomlyRoundRectangle"? I think other parts of the WebCore code assume non-negative rects. We should probably just change this to ASSERT its' a positive rectangle. We could also just fix it to do something "expected", I guess. :) Rects can be negative. This function needs to work. This fixme says they do this because there's no ceilf on Windows. But it works fine for me in VS 2005. Maybe they had a problem with an older compiler? I propose undoing this 3 year old change: http://trac.webkit.org/changeset/12530 Created attachment 24020 [details]
Patch
Comment on attachment 24020 [details]
Patch
Looks good. I am curious why it was removed in the first place though. Perhaps hyatt will share.
+ incorrect. I am not having problems building it on Windows, which was
+ the reason for that change.
There is no reason to include this personal detail.
r=me
Hyatt doesn't remember (chatted with him on IRC last night). I think he didn't include <math.h> but it worked on Mac because of some other includes. Windows doesn't seem to need that include now either, but I added it just in case. ceilf is definitely supported in Visual Studio. Created attachment 24024 [details]
New patch with rebaselined layout tests
This ended up changing a bunch of layout test results. I checked a bunch of the pages and there was no visible difference.
In most cases, the change is obvious looking at the text dump. Most of the problems are when negative coordinates are involved, and the rectangles can get either bigger or smaller depending on the input.
Comment on attachment 24024 [details]
New patch with rebaselined layout tests
Hyatt and I agree, this looks fine. Fix the EMAIL_ADDRESS and land away.
Fixed in r37202. |