Bug 237507

Summary: Empty float rects should remain empty after integrally enclosing them.
Product: WebKit Reporter: Matt Woodrow <mattwoodrow>
Component: Layout and RenderingAssignee: Matt Woodrow <mattwoodrow>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jfernandez, kondapallykalyan, mmaxfield, pdr, rego, sabouhallawa, schenney, sergio, simon.fraser, svillar, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=132753
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
zalan: review+, ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing
ews-feeder: commit-queue-
Patch for landing none

Matt Woodrow
Reported 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.
Attachments
Patch (6.00 KB, patch)
2022-03-06 16:10 PST, Matt Woodrow 		no flags
DetailsFormatted DiffDiff
Patch (10.59 KB, patch)
2022-03-06 20:25 PST, Matt Woodrow 		no flags
DetailsFormatted DiffDiff
Patch (13.86 KB, patch)
2022-03-06 21:14 PST, Matt Woodrow 		no flags
DetailsFormatted DiffDiff
Patch (13.84 KB, patch)
2022-03-07 12:40 PST, Matt Woodrow 		zalan: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (15.43 KB, patch)
2022-03-08 16:22 PST, Matt Woodrow 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (18.55 KB, patch)
2022-03-08 21:36 PST, Matt Woodrow 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (15.41 KB, patch)
2022-03-09 11:50 PST, Matt Woodrow 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-03-06 16:04:32 PST
<rdar://problem/89880101>
Matt Woodrow
Comment 2 2022-03-06 16:07:02 PST
<rdar://88539634>
Matt Woodrow
Comment 3 2022-03-06 16:10:59 PST
Created attachment 453935 [details] Patch
Matt Woodrow
Comment 4 2022-03-06 20:25:56 PST
Created attachment 453941 [details] Patch
Matt Woodrow
Comment 5 2022-03-06 21:14:12 PST
Created attachment 453943 [details] Patch
Matt Woodrow
Comment 6 2022-03-07 12:40:25 PST
Created attachment 454014 [details] Patch
alan
Comment 7 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.
Matt Woodrow
Comment 8 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!
alan
Comment 9 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.
Matt Woodrow
Comment 10 2022-03-08 16:22:18 PST
Created attachment 454170 [details] Patch for landing
Matt Woodrow
Comment 11 2022-03-08 21:36:19 PST
Created attachment 454191 [details] Patch for landing
EWS
Comment 12 2022-03-09 11:47:10 PST
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Matt Woodrow
Comment 13 2022-03-09 11:50:30 PST
Created attachment 454274 [details] Patch for landing
EWS
Comment 14 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].
Note You need to log in before you can comment on or make changes to this bug.