Bug 221671

Created attachment 419838 [details] Patch

Created attachment 419854 [details] Patch

Created attachment 420278 [details] Patch

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 420441 [details] Patch

<rdar://problem/74431153>

Created attachment 420643 [details] Patch

Comment on attachment 420643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420643&action=review > Source/WebCore/rendering/RenderReplaced.cpp:579 > + BoxSizing boxSizing = BoxSizing::ContentBox; > + if (style().hasAspectRatio()) > + boxSizing = style().boxSizingForAspectRatio(); Maybe this should be a function on RenderStyle, call boxSizingForAspectRatio()?

Created attachment 421212 [details] Patch

Created attachment 421298 [details] Patch

Comment on attachment 420643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420643&action=review >> Source/WebCore/rendering/RenderReplaced.cpp:579 >> + boxSizing = style().boxSizingForAspectRatio(); > > Maybe this should be a function on RenderStyle, call boxSizingForAspectRatio()? We already have boxSizingForAspectRatio on RenderStyle. The code may be a bit misleading, the idea is for non CSS aspect-ratio to keep doing what we did before, i.e. ignore box-sizing here, but when CSS aspect-ratio is applied, pass the box-sizing in use to resolveWidthForRatio/resolveHeightForRatio since the passed intrinsicRatio is relative to either the border-box or content-box, depeding on boxSizingForAspectRatio.

Comment on attachment 421298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421298&action=review > Source/WebCore/rendering/RenderReplaced.cpp:539 > + if (boxSizing == BoxSizing::BorderBox) > + return roundToInt(round((logicalHeight + borderAndPaddingLogicalHeight) * aspectRatio)) - borderAndPaddingLogicalWidth; > + return roundToInt(round(logicalHeight * aspectRatio)); Do we know why this code does roundToInt()?

Comment on attachment 421298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421298&action=review >> Source/WebCore/rendering/RenderReplaced.cpp:539 >> + return roundToInt(round(logicalHeight * aspectRatio)); > > Do we know why this code does roundToInt()? Both roundToInt and round are suspect. Neither of those snaps to a device pixel, so what is the goal?

(In reply to Darin Adler from comment #13) > Comment on attachment 421298 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=421298&action=review > > >> Source/WebCore/rendering/RenderReplaced.cpp:539 > >> + return roundToInt(round(logicalHeight * aspectRatio)); > > > > Do we know why this code does roundToInt()? > > Both roundToInt and round are suspect. Neither of those snaps to a device > pixel, so what is the goal? This is layout code, so we shouldn't be snapping to device pixels at this point. I think historically line layout has rounded to integral values, and this is part of that.

Comment on attachment 421298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421298&action=review >>>> Source/WebCore/rendering/RenderReplaced.cpp:539 >>>> + return roundToInt(round(logicalHeight * aspectRatio)); >>> >>> Do we know why this code does roundToInt()? >> >> Both roundToInt and round are suspect. Neither of those snaps to a device pixel, so what is the goal? > > This is layout code, so we shouldn't be snapping to device pixels at this point. > > I think historically line layout has rounded to integral values, and this is part of that. Possibly it can go, r112229 added this: // FIXME: Remove unnecessary round/roundToInt calls from this method when layout is off ints: webkit.org/b/63656

Created attachment 421613 [details] Patch

Comment on attachment 421613 [details] Patch Looks like we do not need roundToInt.

Comment on attachment 421613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421613&action=review > Source/WebCore/rendering/RenderReplaced.cpp:540 > + if (boxSizing == BoxSizing::BorderBox) > + return LayoutUnit(round((logicalHeight + borderAndPaddingLogicalHeight) * aspectRatio)) - borderAndPaddingLogicalWidth; > + return LayoutUnit(round(logicalHeight * aspectRatio)); > +} fromFloatRound()?

Created attachment 421906 [details] Patch

Comment on attachment 421613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421613&action=review >> Source/WebCore/rendering/RenderReplaced.cpp:540 >> +} > > fromFloatRound()? Sure, done.

Created attachment 421932 [details] Patch

Comment on attachment 421613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421613&action=review >>> Source/WebCore/rendering/RenderReplaced.cpp:540 >>> +} >> >> fromFloatRound()? > > Sure, done. Using fromFloatRound caused (even visual) regressions, so I suggest we stick to using round as before, my goal here is to add CSS aspect-ratio logic and not regress the replaced aspect-ratio handling.

Committed r273753: <https://commits.webkit.org/r273753> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421932 [details].