Bug 21292 - enclosingIntRect doesn't handle negative offsets
Summary: enclosingIntRect doesn't handle negative offsets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-01 19:44 PDT by Brett Wilson (Google)
Modified: 2008-10-02 12:34 PDT (History)
0 users

See Also:


Attachments
Patch (1.85 KB, patch)
2008-10-02 08:35 PDT, Brett Wilson (Google)
sam: review+
Details | Formatted Diff | Diff
New patch with rebaselined layout tests (506.38 KB, patch)
2008-10-02 11:36 PDT, Brett Wilson (Google)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.