Bug 21292 – enclosingIntRect doesn't handle negative offsets

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brett Wilson (Google)]

enclosingIntRect in FloatRect.cpp looks like this:

IntRect enclosingIntRect(const FloatRect& rect)
{
    int l = static_cast<int>(rect.x());
    int t = static_cast<int>(rect.y());

l and t will be incorrect (for normal Intel CPU modes as far as I can tell) if the rectangle has negative coordinates. The static cast will truncated, not round to negative infinity, and the resulting rectangle will not end up enclosing the input.

[Brett Wilson (Google)]

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

[Eric Seidel (no email)]

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

[Dave Hyatt]

Rects can be negative.  This function needs to work.

[Brett Wilson (Google)]

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

[Brett Wilson (Google)]

Created attachment 24020 [details]
Patch

[Sam Weinig]

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

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Eric Seidel (no email)]

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.

[Brett Wilson (Google)]

Fixed in r37202.