Bug 237507 – Empty float rects should remain empty after integrally enclosing them.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Woodrow]

enclosingIntRect(const FloatRect&) will take an empty rect with a fractional x and y value and expand it into an integer rect that's 1x1.

This can happen when computing coverage for TileGrid, and result in us allocating a tile, when we don't need it.

[Radar WebKit Bug Importer]

<rdar://problem/89880101>

[Matt Woodrow]

<rdar://88539634>

[Matt Woodrow]

Created attachment 453935 [details]
Patch

[Matt Woodrow]

Created attachment 453941 [details]
Patch

[Matt Woodrow]

Created attachment 453943 [details]
Patch

[Matt Woodrow]

Created attachment 454014 [details]
Patch

[alan]

Comment on attachment 454014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454014&action=review

> Source/WebCore/platform/graphics/FloatRect.cpp:261
> +    FloatPoint location = flooredIntPoint(rect.minXMinYCorner());

Why converting the floored (integral) location value to float?

> Source/WebCore/platform/graphics/FloatRect.cpp:263
> +        return IntRect(IntPoint(location), IntSize());

nit: maybe? return { flooredIntPoint(rect.location()), { } }; 
-isEmpty() may also indicate negative width/height values so we end up converting them to 0, but probably it's ok -or alternatively you could floor them as well.

> Source/WebCore/platform/graphics/FloatRect.cpp:264
> +    FloatPoint maxPoint = ceiledIntPoint(rect.maxXMaxYCorner());

same here.

[Matt Woodrow]

(In reply to zalan from comment #7)
> Comment on attachment 454014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454014&action=review
> 
> > Source/WebCore/platform/graphics/FloatRect.cpp:261
> > +    FloatPoint location = flooredIntPoint(rect.minXMinYCorner());
> 
> Why converting the floored (integral) location value to float?

It's so that the 'maxPoint - location' calculation happens using float range/precision, not int. This is the same behaviour as the existing 'enclosingIntRect' function has (which has an API test that requires this behaviour).

> 
> > Source/WebCore/platform/graphics/FloatRect.cpp:263
> > +        return IntRect(IntPoint(location), IntSize());
> 
> nit: maybe? return { flooredIntPoint(rect.location()), { } }; 
> -isEmpty() may also indicate negative width/height values so we end up
> converting them to 0, but probably it's ok -or alternatively you could floor
> them as well.

I'm not sure there's a need to preserve negative sizes rects, but happy to make this change if you think it's worth it. Will make the {} change!

[alan]

Comment on attachment 454014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454014&action=review

>>> Source/WebCore/platform/graphics/FloatRect.cpp:261
>>> +    FloatPoint location = flooredIntPoint(rect.minXMinYCorner());
>> 
>> Why converting the floored (integral) location value to float?
> 
> It's so that the 'maxPoint - location' calculation happens using float range/precision, not int. This is the same behaviour as the existing 'enclosingIntRect' function has (which has an API test that requires this behaviour).

hah, I wrote that code and added the test case. :/ oh well.

[Matt Woodrow]

Created attachment 454170 [details]
Patch for landing

[Matt Woodrow]

Created attachment 454191 [details]
Patch for landing

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Matt Woodrow]

Created attachment 454274 [details]
Patch for landing

[EWS]

Committed r291080 (248245@main): <https://commits.webkit.org/248245@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454274 [details].