Bug 237507 - Empty float rects should remain empty after integrally enclosing them.
Summary: Empty float rects should remain empty after integrally enclosing them.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Woodrow
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-06 16:04 PST by Matt Woodrow
Modified: 2022-03-09 15:07 PST (History)
20 users (show)

See Also:


Attachments
Patch (6.00 KB, patch)
2022-03-06 16:10 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (10.59 KB, patch)
2022-03-06 20:25 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (13.86 KB, patch)
2022-03-06 21:14 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff
Patch (13.84 KB, patch)
2022-03-07 12:40 PST, Matt Woodrow
zalan: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (15.43 KB, patch)
2022-03-08 16:22 PST, Matt Woodrow
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (18.55 KB, patch)
2022-03-08 21:36 PST, Matt Woodrow
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (15.41 KB, patch)
2022-03-09 11:50 PST, Matt Woodrow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Woodrow 2022-03-06 16:04:22 PST
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.
Comment 1 Radar WebKit Bug Importer 2022-03-06 16:04:32 PST
<rdar://problem/89880101>
Comment 2 Matt Woodrow 2022-03-06 16:07:02 PST
<rdar://88539634>
Comment 3 Matt Woodrow 2022-03-06 16:10:59 PST
Created attachment 453935 [details]
Patch
Comment 4 Matt Woodrow 2022-03-06 20:25:56 PST
Created attachment 453941 [details]
Patch
Comment 5 Matt Woodrow 2022-03-06 21:14:12 PST
Created attachment 453943 [details]
Patch
Comment 6 Matt Woodrow 2022-03-07 12:40:25 PST
Created attachment 454014 [details]
Patch
Comment 7 zalan 2022-03-07 18:59:04 PST
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.
Comment 8 Matt Woodrow 2022-03-07 20:03:06 PST
(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!
Comment 9 zalan 2022-03-07 20:24:16 PST
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.
Comment 10 Matt Woodrow 2022-03-08 16:22:18 PST
Created attachment 454170 [details]
Patch for landing
Comment 11 Matt Woodrow 2022-03-08 21:36:19 PST
Created attachment 454191 [details]
Patch for landing
Comment 12 EWS 2022-03-09 11:47:10 PST
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 13 Matt Woodrow 2022-03-09 11:50:30 PST
Created attachment 454274 [details]
Patch for landing
Comment 14 EWS 2022-03-09 15:07:30 PST
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].