Bug 21292

Summary: enclosingIntRect doesn't handle negative offsets
Product: WebKit Reporter: Brett Wilson (Google) <brettw>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
sam: review+
New patch with rebaselined layout tests eric: review+

Description Brett Wilson (Google) 2008-10-01 19:44:28 PDT
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.
Comment 1 Brett Wilson (Google) 2008-10-01 19:47:21 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"?
Comment 2 Eric Seidel (no email) 2008-10-01 19:59:20 PDT
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. :)
Comment 3 Dave Hyatt 2008-10-01 20:06:38 PDT
Rects can be negative.  This function needs to work.

Comment 4 Brett Wilson (Google) 2008-10-01 20:09:23 PDT
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

Comment 5 Brett Wilson (Google) 2008-10-02 08:35:12 PDT
Created attachment 24020 [details]
Patch
Comment 6 Sam Weinig 2008-10-02 08:44:52 PDT
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
Comment 7 Brett Wilson (Google) 2008-10-02 10:28:50 PDT
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.
Comment 8 Brett Wilson (Google) 2008-10-02 11:36:47 PDT
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 9 Eric Seidel (no email) 2008-10-02 11:50:39 PDT
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.
Comment 10 Brett Wilson (Google) 2008-10-02 12:34:17 PDT
Fixed in r37202.